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

[Michal Hocko <mhocko@suse.com>]

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.

> 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.

> 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.

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.
-- 
Michal Hocko
SUSE Labs