On Thu, Jul 18, 2024 at 7:48 PM Hailong Liu <hailong.liu@xxxxxxxx> wrote:
>
> On Thu, 18. Jul 11:00, Barry Song wrote:
> > From: Barry Song <v-songbaohua@xxxxxxxx>
> >
> > Overflow in this context is highly unlikely. However, allocations using
> > GFP_NOFAIL are guaranteed to succeed, so checking the return value is
> > unnecessary. One option to fix this is allowing memory allocation with
> > an overflowed size, but it seems pointless. Let's at least issue a
> > warning. Likely BUG_ON() seems better as anyway we can't fix it?
> >
> > Cc: Michal Hocko <mhocko@xxxxxxxx>
> > Cc: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> > Cc: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> > Cc: Christoph Lameter <cl@xxxxxxxxx>
> > Cc: Pekka Enberg <penberg@xxxxxxxxxx>
> > Cc: David Rientjes <rientjes@xxxxxxxxxx>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxx>
> > Cc: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> > Cc: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
> > Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
> > ---
> > include/linux/slab.h | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index a332dd2fa6cd..c6aec311864f 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -692,8 +692,10 @@ static inline __alloc_size(1, 2) void *kmalloc_array_noprof(size_t n, size_t siz
> > {
> > size_t bytes;
> >
> > - if (unlikely(check_mul_overflow(n, size, &bytes)))
> > + if (unlikely(check_mul_overflow(n, size, &bytes))) {
> > + WARN_ON(flags & __GFP_NOFAIL);
> Hi Barry:
>
> IMO, using __GFP_NOFAIL guarantees success if and only if the parameters are *correct*.
> Maybe we can add here to help callers to find the reason as in mm/page_alloc.c
no, this doesn't make any sense:
* %__GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller
* cannot handle allocation failures. The allocation could block
* indefinitely but will never return with failure. Testing for
* failure is pointless.
I believe these are two separate things at different layers.
>
> ```
> if (gfp_mask & __GFP_NOFAIL) {
> /*
> * All existing users of the __GFP_NOFAIL are blockable, so warn
> * of any new users that actually require GFP_NOWAIT
> */
> if (WARN_ON_ONCE_GFP(!can_direct_reclaim, gfp_mask))
> goto fail;
If the code calling this function doesn't have a retry mechanism, it is
a BUG that needs to be fixed. even though the above code might return
NULL, its wrapper will still retry, for example:
while (nr_allocated < nr_pages) {
if (!nofail && fatal_signal_pending(current))
break;
if (nid == NUMA_NO_NODE)
page = alloc_pages_noprof(alloc_gfp, order);
else
page = alloc_pages_node_noprof(nid, alloc_gfp, order);
if (unlikely(!page)) {
if (!nofail)
break;
/* fall back to the zero order allocations */
alloc_gfp |= __GFP_NOFAIL;
order = 0;
continue;
}
/*
* Higher order allocations must be able to be treated as
* indepdenent small pages by callers (as they can with
* small-page vmallocs). Some drivers do their own refcounting
* on vmalloc_to_page() pages, some use page->mapping,
* page->lru, etc.
*/
if (order)
split_page(page, order);
/*
* Careful, we allocate and map page-order pages, but
* tracking is done per PAGE_SIZE page so as to keep the
* vm_struct APIs independent of the physical/mapped size.
*/
for (i = 0; i < (1U << order); i++)
pages[nr_allocated + i] = page + i;
cond_resched();
nr_allocated += 1U << order;
}
>
> /*
> * PF_MEMALLOC request from this context is rather bizarre
> * because we cannot reclaim anything and only can loop waiting
> * for somebody to do a work for us
> */
> WARN_ON_ONCE_GFP(current->flags & PF_MEMALLOC, gfp_mask);
>
> /*
> * non failing costly orders are a hard requirement which we
> * are not prepared for much so let's warn about these users
> * so that we can identify them and convert them to something
> * else.
> */
> WARN_ON_ONCE_GFP(costly_order, gfp_mask);
> ```
>
> > return NULL;
> > + }
> > if (__builtin_constant_p(n) && __builtin_constant_p(size))
> > return kmalloc_noprof(bytes, flags);
> > return kmalloc_noprof(bytes, flags);
> > @@ -794,8 +796,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))) {
> > + WARN_ON(flags & __GFP_NOFAIL);
> > return NULL;
> > + }
> >
> > return kvmalloc_node_noprof(bytes, flags, node);
> > }
> > --
> > 2.34.1
> >
> >
>
> --
> help you, help me,
> Hailong.
Thanks
Barry
