Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Uladzislau Rezki <urezki@gmail.com>
To: Artem Lytkin <iprintercanon@gmail.com>
Cc: linux-mm@kvack.org, akpm@linux-foundation.org, urezki@gmail.com,
	shivamkalra98@zohomail.in, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/vmalloc: make vm_struct.nr_pages an unsigned long
Date: Thu, 30 Jul 2026 15:04:24 +0200	[thread overview]
Message-ID: <amtL2Md-vl_gt-pV@milan> (raw)
In-Reply-To: <20260729175708.7074-1-iprintercanon@gmail.com>

On Wed, Jul 29, 2026 at 08:57:08PM +0300, Artem Lytkin wrote:
> A vmalloc area can hold more than 2^32 pages, but the page counts
> tracking it are 32 bit. That has produced the same truncation bug twice
> already, once in vread_iter() and once in the vrealloc() grow-in-place
> check, and it keeps the casts that hide it scattered around the file.
> 
> Widen the field and everything that feeds or consumes it, so the byte
> counts derived from it are computed in 64 bit arithmetic without a cast
> at each site:
> 
>   - vm_area_alloc_pages() takes and returns a page count, and the result
>     is stored into nr_pages, so its parameter, return type and the
>     nr_allocated and nr_remaining accumulators widen too. The 100 page
>     cap on a bulk request stays narrow, only its literal becomes 100UL.
> 
>   - nr_small_pages in __vmalloc_area_node() is derived from a 64 bit
>     size and compared against nr_pages, so it widens as well. Before
>     this the store truncated for a size of 16 TiB or more, which sized
>     area->pages from the wrapped count while __vmap_pages_range() still
>     walked the full size. The cast on array_size next to it was already
>     redundant and goes away with it.
> 
>   - new_nr_pages and old_nr_pages in vrealloc_node_align_noprof() widen,
>     which drops the four casts on them, and the casts added by the two
>     preceding patches in vrealloc_node_align_noprof() and vread_iter()
>     are no longer needed either.
> 
>   - vm_area_free_pages() takes a page index range.
> 
> Three loop counters that index area->pages were plain int and would have
> overflowed at 2^31 pages: in set_area_direct_map(), in vm_reset_perms()
> and in the bulk allocation loop, where the index is seeded from
> nr_allocated. They become unsigned long.
> 
> Printing needed fixing in two places. vmalloc_dump_obj() used %u, and
> vmalloc_info_show() printed the unsigned field with %d, which would have
> shown a negative page count for an area of 8 TiB or more.
> show_numa_info() used a single unsigned int as both the page index and
> the node id, so the page loop gets its own unsigned long counter.
> 
> The page table walkers are not covered here. vmap_pages_pte_range() and
> the levels above it carry the page cursor as int *nr, so a mapping is
> still capped independently of the counters this patch widens. Widening
> that cursor touches the whole walker and is separate work.
> 
> Nothing outside mm/vmalloc.c needs a change to build or to behave as
> before. Those users either index the pages array or multiply the count
> by PAGE_SIZE, which is unsigned long already. kexec_handover stores the
> count into a 32 bit field of its own ABI, which caps that interface
> exactly where it did before.
> 
> On x86-64 this leaves sizeof(struct vm_struct) at 72 bytes when
> CONFIG_HAVE_ARCH_HUGE_VMALLOC=n, since the existing padding before
> phys_addr absorbs the change, and takes it to 80 when the config is
> enabled. Both sizes come from the same kmalloc-96 bucket that
> __get_vm_area_node() allocates from, so the footprint per area does not
> change either way.
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Artem Lytkin <iprintercanon@gmail.com>
> ---
>  include/linux/vmalloc.h |  2 +-
>  mm/vmalloc.c            | 62 ++++++++++++++++++++---------------------
>  2 files changed, 31 insertions(+), 33 deletions(-)
> 
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index d87dc7f77f4e8..a39c20c77efcd 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -62,7 +62,7 @@ struct vm_struct {
>  #ifdef CONFIG_HAVE_ARCH_HUGE_VMALLOC
>  	unsigned int		page_order;
>  #endif
> -	unsigned int		nr_pages;
> +	unsigned long		nr_pages;
>  	phys_addr_t		phys_addr;
>  	const void		*caller;
>  	unsigned long		requested_size;
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index baf7e396e3fc7..efd5e42c64e2e 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3338,7 +3338,7 @@ struct vm_struct *remove_vm_area(const void *addr)
>  static inline void set_area_direct_map(const struct vm_struct *area,
>  				       int (*set_direct_map)(struct page *page))
>  {
> -	int i;
> +	unsigned long i;
>  
>  	/* HUGE_VMALLOC passes small pages to set_direct_map */
>  	for (i = 0; i < area->nr_pages; i++)
> @@ -3354,7 +3354,7 @@ static void vm_reset_perms(struct vm_struct *area)
>  	unsigned long start = ULONG_MAX, end = 0;
>  	unsigned int page_order = vm_area_page_order(area);
>  	int flush_dmap = 0;
> -	int i;
> +	unsigned long i;
>  
>  	/*
>  	 * Find the start and end range of the direct mappings to make sure that
> @@ -3427,10 +3427,10 @@ void vfree_atomic(const void *addr)
>   * Caller is responsible for unmapping (vunmap_range) and KASAN
>   * poisoning before calling this.
>   */
> -static void vm_area_free_pages(struct vm_struct *vm, unsigned int start_idx,
> -			       unsigned int end_idx)
> +static void vm_area_free_pages(struct vm_struct *vm, unsigned long start_idx,
> +			       unsigned long end_idx)
>  {
> -	unsigned int i;
> +	unsigned long i;
>  
>  	if (!(vm->flags & VM_MAP_PUT_PAGES)) {
>  		for (i = start_idx; i < end_idx; i++)
> @@ -3642,12 +3642,12 @@ static inline gfp_t vmalloc_gfp_adjust(gfp_t flags, const bool large)
>  	return flags;
>  }
>  
> -static inline unsigned int
> +static inline unsigned long
>  vm_area_alloc_pages(gfp_t gfp, int nid,
> -		unsigned int order, unsigned int nr_pages, struct page **pages)
> +		unsigned int order, unsigned long nr_pages, struct page **pages)
>  {
> -	unsigned int nr_allocated = 0;
> -	unsigned int nr_remaining = nr_pages;
> +	unsigned long nr_allocated = 0;
> +	unsigned long nr_remaining = nr_pages;
>  	unsigned int max_attempt_order = MAX_PAGE_ORDER;
>  	struct page *page;
>  	int i;
> @@ -3695,7 +3695,7 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  	if (!order) {
>  		while (nr_allocated < nr_pages) {
>  			unsigned int nr, nr_pages_request;
> -			int i;
> +			unsigned long i;
>  
>  			/*
>  			 * A maximum allowed request is hard-coded and is 100
> @@ -3703,7 +3703,7 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>  			 * long preemption off scenario in the bulk-allocator
>  			 * so the range is [1:100].
>  			 */
> -			nr_pages_request = min(100U, nr_pages - nr_allocated);
> +			nr_pages_request = min(100UL, nr_pages - nr_allocated);
>  
>  			/* memory allocation should consider mempolicy, we can't
>  			 * wrongly use nearest node when nid == NUMA_NO_NODE,
> @@ -3849,12 +3849,12 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	unsigned long addr = (unsigned long)area->addr;
>  	unsigned long size = get_vm_area_size(area);
>  	unsigned long array_size;
> -	unsigned int nr_small_pages = size >> PAGE_SHIFT;
> +	unsigned long nr_small_pages = size >> PAGE_SHIFT;
>  	unsigned int page_order;
>  	unsigned int flags;
>  	int ret;
>  
> -	array_size = (unsigned long)nr_small_pages * sizeof(struct page *);
> +	array_size = nr_small_pages * sizeof(struct page *);
>  
>  	/* __GFP_NOFAIL and "noblock" flags are mutually exclusive. */
>  	if (!gfpflags_allow_blocking(gfp_mask))
> @@ -4352,7 +4352,7 @@ void *vrealloc_node_align_noprof(const void *p, size_t size, unsigned long align
>  	}
>  
>  	if (size <= old_size) {
> -		unsigned int new_nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +		unsigned long new_nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>  
>  		/* Zero out "freed" memory, potentially for future realloc. */
>  		if (want_init_on_free() || want_init_on_alloc(flags))
> @@ -4381,7 +4381,7 @@ void *vrealloc_node_align_noprof(const void *p, size_t size, unsigned long align
>  		    !(vm->flags & (VM_FLUSH_RESET_PERMS | VM_USERMAP)) &&
>  		    gfp_has_io_fs(flags)) {
>  			unsigned long addr = (unsigned long)kasan_reset_tag(p);
> -			unsigned int old_nr_pages = vm->nr_pages;
> +			unsigned long old_nr_pages = vm->nr_pages;
>  
>  			/*
>  			 * Use the node lock to synchronize with concurrent
> @@ -4394,16 +4394,13 @@ void *vrealloc_node_align_noprof(const void *p, size_t size, unsigned long align
>  			spin_unlock(&vn->busy.lock);
>  
>  			/* Notify kmemleak of the reduced allocation size before unmapping. */
> -			kmemleak_free_part(
> -				(void *)addr + ((unsigned long)new_nr_pages
> -						<< PAGE_SHIFT),
> -				(unsigned long)(old_nr_pages - new_nr_pages)
> -					<< PAGE_SHIFT);
> +			kmemleak_free_part((void *)addr +
> +					   (new_nr_pages << PAGE_SHIFT),
> +					   (old_nr_pages - new_nr_pages)
> +						<< PAGE_SHIFT);
>  
> -			vunmap_range(addr + ((unsigned long)new_nr_pages
> -					     << PAGE_SHIFT),
> -				     addr + ((unsigned long)old_nr_pages
> -					     << PAGE_SHIFT));
> +			vunmap_range(addr + (new_nr_pages << PAGE_SHIFT),
> +				     addr + (old_nr_pages << PAGE_SHIFT));
>  
>  			vm_area_free_pages(vm, new_nr_pages, old_nr_pages);
>  		}
> @@ -4415,7 +4412,7 @@ 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 <= (unsigned long)vm->nr_pages << PAGE_SHIFT) {
> +	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
> @@ -4722,7 +4719,7 @@ long vread_iter(struct iov_iter *iter, const char *addr, size_t count)
>  			 * mapping types (vmap, ioremap) don't set nr_pages.
>  			 */
>  			size = (vm->flags & VM_ALLOC && vm->nr_pages) ?
> -				       ((unsigned long)vm->nr_pages << PAGE_SHIFT) :
> +				       (vm->nr_pages << PAGE_SHIFT) :
>  				       get_vm_area_size(vm);
>  		else
>  			size = va_size(va);
> @@ -5228,7 +5225,7 @@ bool vmalloc_dump_obj(void *object)
>  	struct vmap_area *va;
>  	struct vmap_node *vn;
>  	unsigned long addr;
> -	unsigned int nr_pages;
> +	unsigned long nr_pages;
>  
>  	addr = PAGE_ALIGN((unsigned long) object);
>  	vn = addr_to_node(addr);
> @@ -5248,7 +5245,7 @@ bool vmalloc_dump_obj(void *object)
>  	nr_pages = vm->nr_pages;
>  	spin_unlock(&vn->busy.lock);
>  
> -	pr_cont(" %u-page vmalloc region starting at %#lx allocated at %pS\n",
> +	pr_cont(" %lu-page vmalloc region starting at %#lx allocated at %pS\n",
>  		nr_pages, addr, caller);
>  
>  	return true;
> @@ -5266,16 +5263,17 @@ bool vmalloc_dump_obj(void *object)
>  static void show_numa_info(struct seq_file *m, struct vm_struct *v,
>  				 unsigned int *counters)
>  {
> -	unsigned int nr;
>  	unsigned int step = 1U << vm_area_page_order(v);
> +	unsigned long i;
> +	unsigned int nr;
>  
>  	if (!counters)
>  		return;
>  
>  	memset(counters, 0, nr_node_ids * sizeof(unsigned int));
>  
> -	for (nr = 0; nr < v->nr_pages; nr += step)
> -		counters[page_to_nid(v->pages[nr])] += step;
> +	for (i = 0; i < v->nr_pages; i += step)
> +		counters[page_to_nid(v->pages[i])] += step;
>  	for_each_node_state(nr, N_HIGH_MEMORY)
>  		if (counters[nr])
>  			seq_printf(m, " N%u=%u", nr, counters[nr]);
> @@ -5333,7 +5331,7 @@ static int vmalloc_info_show(struct seq_file *m, void *p)
>  				seq_printf(m, " %pS", v->caller);
>  
>  			if (v->nr_pages)
> -				seq_printf(m, " pages=%d", v->nr_pages);
> +				seq_printf(m, " pages=%lu", v->nr_pages);
>  
>  			if (v->phys_addr)
>  				seq_printf(m, " phys=%pa", &v->phys_addr);
> 
> base-commit: 248951ddc14de84de3910f9b13f51491a8cd91df
> prerequisite-patch-id: bc462c518eb9ddcaccad5e1f5fed7cc24512b117
> prerequisite-patch-id: 891688c2e93c570d1c9c7802b46aaba974002684
> -- 
> 2.43.0
> 
Makes sense to me. The commit message could be shorter describing
just an issue. I was lost when i read it first time.

Also:

<snip>
Artem Lytkin (3):
  mm/vmalloc: fix 32-bit truncation of the area size in vread_iter()
  mm/vmalloc: fix 32-bit truncation in the vrealloc() grow-in-place check
  mm/vmalloc: make vm_struct.nr_pages an unsigned long
<snip>

first two fix overlaps. The latest also do it by converting nr_pages
to unsigned long and removes (unsigned long) casting introduced by 1 and 2.

Maybe just use one commit which is last in this series?

Anyway for this

Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

--
Uladzislau Rezki


      parent reply	other threads:[~2026-07-30 13:04 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-29 17:57 [PATCH] mm/vmalloc: make vm_struct.nr_pages an unsigned long Artem Lytkin
2026-07-29 18:28 ` Matthew Wilcox
2026-07-29 21:45   ` Andrew Morton
2026-07-30  9:07   ` Artem Lytkin
2026-07-30  9:06 ` [PATCH v2 0/3] mm/vmalloc: stop truncating byte counts derived from nr_pages Artem Lytkin
2026-07-30  9:06 ` [PATCH v2 1/3] mm/vmalloc: fix 32-bit truncation of the area size in vread_iter() Artem Lytkin
2026-07-30  9:06 ` [PATCH v2 2/3] mm/vmalloc: fix 32-bit truncation in the vrealloc() grow-in-place check Artem Lytkin
2026-07-30  9:06 ` [PATCH v2 3/3] mm/vmalloc: make vm_struct.nr_pages an unsigned long Artem Lytkin
2026-07-30 13:04 ` Uladzislau Rezki [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=amtL2Md-vl_gt-pV@milan \
    --to=urezki@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=iprintercanon@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=shivamkalra98@zohomail.in \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox