Re: [PATCH v3 3/4] mm: BUG_ON to avoid NULL deference while __GFP_NOFAIL fails

[Michal Hocko <mhocko@suse.com>]

On Mon 19-08-24 14:49:55, David Hildenbrand wrote:
> On 19.08.24 14:48, Barry Song wrote:
[...]
> > > > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > > > > > index 60742d057b05..d2c37f8f8d09 100644
> > > > > > > > --- a/mm/page_alloc.c
> > > > > > > > +++ b/mm/page_alloc.c
> > > > > > > > @@ -4668,8 +4668,10 @@ struct page *__alloc_pages_noprof(gfp_t gfp, unsigned int order,
> > > > > > > >           * There are several places where we assume that the order value is sane
> > > > > > > >           * so bail out early if the request is out of bound.
> > > > > > > >           */
> > > > > > > > -     if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp))
> > > > > > > > +     if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp)) {
> > > > > > > > +             BUG_ON(gfp & __GFP_NOFAIL);
> > > > > > > >                  return NULL;
> > > > > > > > +     }
[...]
> > Returning NULL doesn't necessarily crash the caller's process, p->field,
> > *(p + offset) deference could be used by hackers to exploit the system.
> 
> See my other reply to Michal: why do we even allow to specify them
> separately and not simply let one enforce the other?

Are you replying to this patch? This is not about a combination of
flags. This is about the above (and other similar) boundary checks which
return NULL if the size is deemed incorrect. I think those are potential
problems because it could be a lack of input check which could be turned
into a potentially malicious code. Because unchecked (return value
because NOFAIL never fails, right?) return value might even not OOPs and
become a silent read/write into memory.

Whether to BUG_ON or simply loop for ever in the allocator if somebody
requests non-sleeping NOFAIL allocation is a different story.
-- 
Michal Hocko
SUSE Labs