Re: [PATCH v4] mm/vmalloc: make vm_struct.nr_pages an unsigned long
Uladzislau Rezki <[email protected]>
| Newsgroups | gmane.linux.kernel,gmane.linux.kernel.mm |
|---|---|
| Message-ID | <anRK-y0nBUnbli_u@milan> |
On Mon, Aug 03, 2026 at 05:39:35PM -0700, Andrew Morton wrote: > On Sun, 2 Aug 2026 17:52:26 +0200 Uladzislau Rezki <[email protected]> wrote: > > > On Sat, Aug 01, 2026 at 11:52:02AM -0700, Andrew Morton wrote: > > > On Sat, 1 Aug 2026 14:49:15 +0300 Artem Lytkin <[email protected]> wrote: > > > > > > > ... > > > > > Ulad, AI review suggests that vrealloc() has an issue handling > > > __GFP_ZERO. Can you please check? > > > > > > https://sashiko.dev/#/patchset/[email protected] > > > > > I have checked. I think the AI is missing at least one point. > > AI argument which is: > > > > <snip> > > If a driver initially allocates memory using vmalloc() without __GFP_ZERO > > (leaving spare page capacity uninitialized), and then grows the allocation > > using vrealloc() with __GFP_ZERO, the caller expects the newly exposed bytes > > to be zeroed. > > <snip> > > > > In the vrealloc_node_align_noprof() header documentation there is a statement: > > > > <snip> > > * If __GFP_ZERO logic is requested, callers must ensure that, starting with the > > * initial memory allocation, every subsequent call to this API for the same > > * memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that > > * __GFP_ZERO is not fully honored by this API. > > <snip> > > > > AI argument violates the documentation, i.e. mixing __GFP_ZERO is not allowed. > > Sashiko is talking about the initial allocation not using __GFP_ZERO > but vrealloc() *does* use __GFP_ZERO. The documentation you quoted > doesn't address that case? > But this is not allowed according to doc :) <snip> ... callers must ensure that, starting with the initial memory allocation ... <snip> Also, AI describes the situation like: vmalloc() vrealloc(grow, since need more) i.e. from the description: <snip> and then grows the allocation using vrealloc() with __GFP_ZERO, the caller expects the newly exposed bytes to be zeroed. <snip> and it will be zeroed in fact, because the path would be: <snip> need_realloc: /* TODO: Grow the vm_area, i.e. allocate and map additional pages. */ n = __vmalloc_node_noprof(size, align, flags, nid, __builtin_return_address(0)); if (!n) return NULL; <snip> and not the one which AI pointed to. The real scenario is: 1. vmalloc(!GFP_ZERO) - alloc size 10 2. vrealloc(!GFP_ZERO) - realloc to size 5 3. vrealloc(GFP_ZERO) - realloc back to 10. 3 - will not zeroed. As noted in doc ZERO should be used starting from the beginning [1]. But in __most__ cases it will be zeroed anyway because we free/unmap tail pages and if: <snip> /* * Free tail pages when shrink crosses a page boundary. * * Skip huge page allocations (page_order > 0) as partial * freeing would require splitting. * * Skip VM_FLUSH_RESET_PERMS, as direct-map permissions must * be reset before pages are returned to the allocator. * * Skip VM_USERMAP, as remap_vmalloc_range_partial() validates * mapping requests against the unchanged vm->size; freeing * tail pages would cause vmalloc_to_page() to return NULL for * the unmapped range. * * Skip if either GFP_NOFS or GFP_NOIO are used. * kmemleak_free_part() internally allocates with * GFP_KERNEL, which could trigger a recursive deadlock * if we are under filesystem or I/O reclaim. */ if (new_nr_pages < vm->nr_pages && !vm_area_page_order(vm) && !(vm->flags & (VM_FLUSH_RESET_PERMS | VM_USERMAP)) && gfp_has_io_fs(flags)) { <snip> is true the next grow with GFP_ZERO will be zeroed. For huge alloc it will not be zeroed and for other conditions. > Also, developers don't read documentation ;) What happens if some > caller *does* use vmalloc(!__GFP_ZERO) then vrealloc(__GFP_ZERO)? > Silent misbehavior would be bad - it would be good if vrealloc() were > to drop a WARN() then ignore the __GFP_ZERO. > > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -4294,11 +4294,6 @@ EXPORT_SYMBOL(vzalloc_node_noprof); > > * __GFP_THISNODE flag should be set, otherwise the function will try to avoid > > * reallocation and possibly disregard the specified @nid. > > * > > - * If __GFP_ZERO logic is requested, callers must ensure that, starting with the > > - * initial memory allocation, every subsequent call to this API for the same > > - * memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that > > - * __GFP_ZERO is not fully honored by this API. > > - * > > * Requesting an alignment that is bigger than the alignment of the existing > > * allocation will fail. > > * > > @@ -4415,13 +4410,12 @@ void *vrealloc_node_align_noprof(const void *p, size_t size, unsigned long align > > * We already have the bytes available in the allocation; use them. > > */ > > if (size <= vm->nr_pages << PAGE_SHIFT) { > > - /* > > - * No need to zero memory here, as unused memory will have > > - * already been zeroed at initial allocation time or during > > - * realloc shrink time. > > - */ > > - vm->requested_size = size; > > kasan_vrealloc(p, old_size, size); > > + > > + if (want_init_on_alloc(flags)) > > + memset((void *)p + old_size, 0, size - old_size); > > + > > + vm->requested_size = size; > > return (void *)p; > > } > > OK, thanks, I'll assume you'll prepare this for real when convenient. > OK. I will prepare something and send out the patch after testing. -- Uladzislau Rezki