| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aAEpryR3d4OmlFIk@pc636>
Date: Thu, 17 Apr 2025 18:17:51 +0200
From: Uladzislau Rezki <urezki@...il.com>
To: Baoquan He <bhe@...hat.com>
Cc: Uladzislau Rezki <urezki@...il.com>, linux-mm@...ck.org,
akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] mm/vmalloc.c: return explicit error value in
alloc_vmap_area()
On Thu, Apr 17, 2025 at 11:02:06AM +0800, Baoquan He wrote:
> On 04/16/25 at 04:28pm, Uladzislau Rezki wrote:
> > On Tue, Apr 15, 2025 at 10:39:52AM +0800, Baoquan He wrote:
> > > In codes of alloc_vmap_area(), it returns the upper bound 'vend' to
> > > indicate if the allocation is successful or failed. That is not very clear.
> > >
> > > Here change to return explicit error values and check them to judge if
> > > allocation is successful.
> > >
> > > IS_ERR_VALUE already uses unlikely() internally
> > >
> > > Signed-off-by: Baoquan He <bhe@...hat.com>
> > > ---
> > > mm/vmalloc.c | 34 +++++++++++++++++-----------------
> > > 1 file changed, 17 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > index 3f38a232663b..5b21cd09b2b4 100644
> > > --- a/mm/vmalloc.c
> > > +++ b/mm/vmalloc.c
> > > @@ -1715,7 +1715,7 @@ va_clip(struct rb_root *root, struct list_head *head,
> > > */
> > > lva = kmem_cache_alloc(vmap_area_cachep, GFP_NOWAIT);
> > > if (!lva)
> > > - return -1;
> > > + return -ENOMEM;
> > > }
> > >
> > > /*
> > > @@ -1729,7 +1729,7 @@ va_clip(struct rb_root *root, struct list_head *head,
> > > */
> > > va->va_start = nva_start_addr + size;
> > > } else {
> > > - return -1;
> > > + return -EINVAL;
> > > }
> > >
> > > if (type != FL_FIT_TYPE) {
> > > @@ -1758,19 +1758,19 @@ va_alloc(struct vmap_area *va,
> > >
> > > /* Check the "vend" restriction. */
> > > if (nva_start_addr + size > vend)
> > > - return vend;
> > > + return -ERANGE;
> > >
> > > /* Update the free vmap_area. */
> > > ret = va_clip(root, head, va, nva_start_addr, size);
> > > - if (WARN_ON_ONCE(ret))
> > > - return vend;
> > >
> > Not clear why you remove this WARN_ON by this patch. It should be
> > a separate patch or just keep it as is. The warning here can mean
> > that something is really wrong, especially if NOTHING_FIT. So we
> > definitely want the warning.
>
> I remember one time someone reported that the slab allocation failure
> triggered this warning which is confusing to them. But yes, it should be
> discussed in a separate post or thread, not appropriate to remove it
> silently. I will add it back in v2.
>
Thanks!
> >
> > > + if (ret)
> > > + return ret;
> > >
> > > return nva_start_addr;
> > > }
> > >
> > > /*
> > > * Returns a start address of the newly allocated area, if success.
> > > - * Otherwise a vend is returned that indicates failure.
> > > + * Otherwise an error value is returned that indicates failure.
> > > */
> > > static __always_inline unsigned long
> > > __alloc_vmap_area(struct rb_root *root, struct list_head *head,
> > > @@ -1795,14 +1795,13 @@ __alloc_vmap_area(struct rb_root *root, struct list_head *head,
> > >
> > > va = find_vmap_lowest_match(root, size, align, vstart, adjust_search_size);
> > > if (unlikely(!va))
> > > - return vend;
> > > + return -ENOENT;
> > >
> > > nva_start_addr = va_alloc(va, root, head, size, align, vstart, vend);
> > > - if (nva_start_addr == vend)
> > > - return vend;
> > >
> > > #if DEBUG_AUGMENT_LOWEST_MATCH_CHECK
> > > - find_vmap_lowest_match_check(root, head, size, align);
> > > + if (!IS_ERR_VALUE(nva_start_addr))
> > >
> > Just keep it as it was. No need to check if addr is valid or not.
>
> This is to keep consistent with the old code. Before this patch, if
> va_alloc() return vend, it returns directly, no
> find_vmap_lowest_match_check() invocation is done. I tried to keep the
> behaviour unchanged. That code is for debugging, both is fine to me.
>
Ack. Makes sense to keep same behaviour as it was/is.
> >
> > > + find_vmap_lowest_match_check(root, head, size, align);
> > > #endif
> > >
> > > return nva_start_addr;
> > > @@ -1932,7 +1931,7 @@ node_alloc(unsigned long size, unsigned long align,
> > > struct vmap_area *va;
> > >
> > > *vn_id = 0;
> > > - *addr = vend;
> > > + *addr = -EINVAL;
> > >
> > > /*
> > > * Fallback to a global heap if not vmalloc or there
> > > @@ -2012,20 +2011,20 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> > > }
> > >
> > > retry:
> > > - if (addr == vend) {
> > > + if (IS_ERR_VALUE(addr)) {
> > > preload_this_cpu_lock(&free_vmap_area_lock, gfp_mask, node);
> > > addr = __alloc_vmap_area(&free_vmap_area_root, &free_vmap_area_list,
> > > size, align, vstart, vend);
> > > spin_unlock(&free_vmap_area_lock);
> > > }
> > >
> > > - trace_alloc_vmap_area(addr, size, align, vstart, vend, addr == vend);
> > > + trace_alloc_vmap_area(addr, size, align, vstart, vend, IS_ERR_VALUE(addr));
> > >
> > > /*
> > > - * If an allocation fails, the "vend" address is
> > > + * If an allocation fails, the error value is
> > > * returned. Therefore trigger the overflow path.
> > > */
> > > - if (unlikely(addr == vend))
> > > + if (IS_ERR_VALUE(addr))
> > > goto overflow;
> > >
> > > va->va_start = addr;
> > > @@ -4753,9 +4752,10 @@ struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets,
> > >
> > > ret = va_clip(&free_vmap_area_root,
> > > &free_vmap_area_list, va, start, size);
> > > - if (WARN_ON_ONCE(unlikely(ret)))
> > > - /* It is a BUG(), but trigger recovery instead. */
> > Keep the comment.
>
> OK, will add it back.
>
Thank you.
--
Uladzislau Rezki
Powered by blists - more mailing lists