Re: [PATCH RFC] mm: warn potential return NULL for kmalloc_array and kvmalloc_array with __GFP_NOFAIL

[Vlastimil Babka <vbabka@suse.cz>]

On 7/22/24 9:23 AM, Michal Hocko wrote:
> On Sat 20-07-24 08:36:16, Barry Song wrote:
>> On Fri, Jul 19, 2024 at 9:30 PM Michal Hocko <mhocko@suse.com> wrote:
>> >
>> > On Fri 19-07-24 21:02:10, Barry Song wrote:
>> > > what about an earlier WARN_ON, for example, even before we begin to
>> > > try the allocation?
>> >
>> > Page allocator is a hot path and adding checks for something that
>> > shouldn't really even exist is not a great addition there IMHO.
>> 
>> I would argue adding checks for something that shouldn't really
>> even exist is precisely the point of having those checks. These
>> checks help ensure the integrity and robustness of the system
>> by catching unexpected conditions. I agree that the page allocator
>> is a hot path, but adding one line might not cause noticeable
>> overhead, especially considering that alloc_pages() already
>> contains a few hundred lines of code.
> 
> We do not add stuff like that into hot path.

It should be acceptable with CONFIG_DEBUG_VM. Or in __alloc_pages_slowpath()
it could be placed higher even without CONFIG_DEBUG_VM. Right now it's
rather hard to reach, espcially for GPF_ATOMIC.

> 
>> Regardless, let me try to summarize the discussions. Unless
>> anyone has better ideas, v2 series might start with the following
>> actions:
>> 
>> 1. Update the documentation for GFP_NOFAIL to explicitly state
>> that non-wait GFP_NOFAIL is not legal and should be avoided.
>> This will provide a basis for other subsystems to explicitly
>> reject anyone using GFP_ATOMIC | GFP_NOFAIL, etc.
> 
> No objection to that.
> 
>> 2. Add BUG_ON() at the existing points where we return NULL
>> for GFP_NOFAIL -  __alloc_pages_slowpath() , kmalloc, vmalloc
>> to avoid exposing security vulnerabilities.
> 
> Let's see how this goes.

In __alloc_pages_noprof() we also have

        if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp))
                return NULL;

This should BUG_ON for __GFP_NOFAIL, same as the overflowing kmalloc_array().

> 
>> 3. Raise a bug report to the vdpa maintainer, requesting that they
>> either drop GFP_ATOMIC or GFP_NOFAIL based on whether
>> their context is atomic. If GFP_NOFAIL is dropped, ask for an
>> explicit check on the return value.

You could do it right now, based on the warning we have although it's hard
to reach.

> GPF_ATOMIC is likely used because of write_lock(&domain->bounce_lock)
> vduse_domain_remove_user_bounce_pages is iself called with spin lock held
> from vduse_domain_release. So you would need some pre-allocation.