On Mon, Aug 19, 2024 at 9:43 PM David Hildenbrand <[email protected]> wrote: > > On 17.08.24 08:24, Barry Song wrote: > > From: Barry Song <[email protected]> > > > > We have cases we still fail though callers might have __GFP_NOFAIL. Since > > they don't check the return, we are exposed to the security risks for NULL > > deference. > > > > Though BUG_ON() is not encouraged by Linus, this is an unrecoverable > > situation. > > > > Christoph Hellwig: > > The whole freaking point of __GFP_NOFAIL is that callers don't handle > > allocation failures. So in fact a straight BUG is the right thing > > here. > > > > Vlastimil Babka: > > It's just not a recoverable situation (WARN_ON is for recoverable > > situations). The caller cannot handle allocation failure and at the same > > time asked for an impossible allocation. BUG_ON() is a guaranteed oops > > with stracktrace etc. We don't need to hope for the later NULL pointer > > dereference (which might if really unlucky happen from a different > > context where it's no longer obvious what lead to the allocation failing). > > > > Michal Hocko: > > Linus tends to be against adding new BUG() calls unless the failure is > > absolutely unrecoverable (e.g. corrupted data structures etc.). I am > > not sure how he would look at simply incorrect memory allocator usage to > > blow up the kernel. Now the argument could be made that those failures > > could cause subtle memory corruptions or even be exploitable which might > > be a sufficient reason to stop them early. > > > > Signed-off-by: Barry Song <[email protected]> > > Reviewed-by: Christoph Hellwig <[email protected]> > > Acked-by: Vlastimil Babka <[email protected]> > > Acked-by: Michal Hocko <[email protected]> > > Cc: Uladzislau Rezki (Sony) <[email protected]> > > Cc: Lorenzo Stoakes <[email protected]> > > Cc: Christoph Lameter <[email protected]> > > Cc: Pekka Enberg <[email protected]> > > Cc: David Rientjes <[email protected]> > > Cc: Joonsoo Kim <[email protected]> > > Cc: Roman Gushchin <[email protected]> > > Cc: Hyeonggon Yoo <[email protected]> > > Cc: Linus Torvalds <[email protected]> > > Cc: Kees Cook <[email protected]> > > Cc: "Eugenio Pérez" <[email protected]> > > Cc: Hailong.Liu <[email protected]> > > Cc: Jason Wang <[email protected]> > > Cc: Maxime Coquelin <[email protected]> > > Cc: "Michael S. Tsirkin" <[email protected]> > > Cc: Xuan Zhuo <[email protected]> > > --- > > include/linux/slab.h | 4 +++- > > mm/page_alloc.c | 4 +++- > > mm/util.c | 1 + > > 3 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/slab.h b/include/linux/slab.h > > index c9cb42203183..4a4d1fdc2afe 100644 > > --- a/include/linux/slab.h > > +++ b/include/linux/slab.h > > @@ -827,8 +827,10 @@ kvmalloc_array_node_noprof(size_t n, size_t size, > > gfp_t flags, int node) > > { > > size_t bytes; > > > > - if (unlikely(check_mul_overflow(n, size, &bytes))) > > + if (unlikely(check_mul_overflow(n, size, &bytes))) { > > + BUG_ON(flags & __GFP_NOFAIL); > > return NULL; > > + } > > > > return kvmalloc_node_noprof(bytes, flags, node); > > } > > 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; > > + } > > > > gfp &= gfp_allowed_mask; > > /* > > diff --git a/mm/util.c b/mm/util.c > > index ac01925a4179..678c647b778f 100644 > > --- a/mm/util.c > > +++ b/mm/util.c > > @@ -667,6 +667,7 @@ void *__kvmalloc_node_noprof(DECL_BUCKET_PARAMS(size, > > b), gfp_t flags, int node) > > > > /* Don't even allow crazy sizes */ > > if (unlikely(size > INT_MAX)) { > > + BUG_ON(flags & __GFP_NOFAIL); > > No new BUG_ON please. WARN_ON_ONCE() + recovery code might be suitable here.
Hi David, WARN_ON_ONCE() might be fine but I don't see how it is possible to recover. > > -- > Cheers, > > David / dhildenb >
