On Mon, Aug 19, 2024 at 9:55 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 19.08.24 11:47, Barry Song wrote:
> > On Mon, Aug 19, 2024 at 9:43 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
> >>
> >> On 17.08.24 08:24, Barry Song wrote:
> >>> From: Barry Song <v-songbaohua@xxxxxxxx>
> >>>
> >>> 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 <v-songbaohua@xxxxxxxx>
> >>> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> >>> Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> >>> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
> >>> Cc: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> >>> Cc: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> >>> Cc: Christoph Lameter <cl@xxxxxxxxx>
> >>> Cc: Pekka Enberg <penberg@xxxxxxxxxx>
> >>> Cc: David Rientjes <rientjes@xxxxxxxxxx>
> >>> Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> >>> Cc: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> >>> Cc: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
> >>> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> >>> Cc: Kees Cook <kees@xxxxxxxxxx>
> >>> Cc: "Eugenio Pérez" <eperezma@xxxxxxxxxx>
> >>> Cc: Hailong.Liu <hailong.liu@xxxxxxxx>
> >>> Cc: Jason Wang <jasowang@xxxxxxxxxx>
> >>> Cc: Maxime Coquelin <maxime.coquelin@xxxxxxxxxx>
> >>> Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> >>> Cc: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> >>> ---
> >>> 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.
>
> Just return NULL? "shit in shit out" :) ?
Returning NULL is perfectly right if gfp doesn't include __GFP_NOFAIL,
as it's the caller's responsibility to check the return value. However, with
__GFP_NOFAIL, users will directly dereference *(p + offset) even when
p == NULL. It is how __GFP_NOFAIL is supposed to work.
>
> --
> Cheers,
>
> David / dhildenb
>
