[PATCH v2 0/7] Cleanup free_pages() misuse

[PATCH v2 0/7] Cleanup free_pages() misuse

[Vishal Moola (Oracle)]

free_pages() is supposed to be called when we only have a virtual address.
__free_pages() is supposed to be called when we have a page.

There are a number of callers that use page_address() to get a page's
virtual address then call free_pages() on it when they should just call
__free_pages() directly.

Add kernel-docs for free_pages() to help callers better understand which
function they should be calling, and replace the obvious cases of
misuse.

-----------------
Based on mm-new. I intend to have all of these taken through the mm tree.

I've split the patches into separate subsystems to make it easier to
resolve conflicts, but there aren't any functional changes.

v2:
  - Reference __get_free_pages() instead of alloc_pages() in the
  free_pages() kernel-doc
  - Get some Reviewed-by tags
  - cc the subsystem maintainers related to specific patches

Vishal Moola (Oracle) (7):
  mm/page_alloc: Add kernel-docs for free_pages()
  aoe: Stop calling page_address() in free_page()
  x86: Stop calling page_address() in free_pages()
  riscv: Stop calling page_address() in free_pages()
  powerpc: Stop calling page_address() in free_pages()
  arm64: Stop calling page_address() in free_pages()
  virtio_balloon: Stop calling page_address() in free_pages()

 arch/arm64/mm/mmu.c                      | 2 +-
 arch/powerpc/mm/book3s64/radix_pgtable.c | 2 +-
 arch/riscv/mm/init.c                     | 4 ++--
 arch/x86/mm/init_64.c                    | 2 +-
 arch/x86/platform/efi/memmap.c           | 2 +-
 drivers/block/aoe/aoecmd.c               | 2 +-
 drivers/virtio/virtio_balloon.c          | 3 +--
 mm/page_alloc.c                          | 9 +++++++++
 8 files changed, 17 insertions(+), 9 deletions(-)

-- 
2.51.0

[PATCH v2 1/7] mm/page_alloc: Add kernel-docs for free_pages()

[Vishal Moola (Oracle)]

Add kernel-docs to free_pages(). This will help callers understand when
to use it instead of __free_pages().

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/page_alloc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index baead29b3e67..f5ab7d5bcbd7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5270,6 +5270,15 @@ void free_pages_nolock(struct page *page, unsigned int order)
 	___free_pages(page, order, FPI_TRYLOCK);
 }
 
+/**
+ * free_pages - Free pages allocated with __get_free_pages().
+ * @addr: The virtual address tied to a page returned from __get_free_pages().
+ * @order: The order of the allocation.
+ *
+ * This function behaves the same as __free_pages(). Use this function
+ * to free pages when you only have a valid virtual address. If you have
+ * the page, call __free_pages() instead.
+ */
 void free_pages(unsigned long addr, unsigned int order)
 {
 	if (addr != 0) {
-- 
2.51.0

[PATCH v2 2/7] aoe: Stop calling page_address() in free_page()

[Vishal Moola (Oracle)]

free_page() should be used when we only have a virtual address. We
should call __free_page() directly on our page instead.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 drivers/block/aoe/aoecmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index 6298f8e271e3..a9affb7c264d 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -1761,6 +1761,6 @@ aoecmd_exit(void)
 	kfree(kts);
 	kfree(ktiowq);
 
-	free_page((unsigned long) page_address(empty_page));
+	__free_page(empty_page);
 	empty_page = NULL;
 }
-- 
2.51.0

[PATCH v2 3/7] x86: Stop calling page_address() in free_pages()

[Vishal Moola (Oracle)]

free_pages() should be used when we only have a virtual address. We
should call __free_pages() directly on our page instead.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 arch/x86/mm/init_64.c          | 2 +-
 arch/x86/platform/efi/memmap.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index b9426fce5f3e..0e4270e20fad 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1031,7 +1031,7 @@ static void __meminit free_pagetable(struct page *page, int order)
 		free_reserved_pages(page, nr_pages);
 #endif
 	} else {
-		free_pages((unsigned long)page_address(page), order);
+		__free_pages(page, order);
 	}
 }
 
diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c
index 061b8ecc71a1..023697c88910 100644
--- a/arch/x86/platform/efi/memmap.c
+++ b/arch/x86/platform/efi/memmap.c
@@ -42,7 +42,7 @@ void __init __efi_memmap_free(u64 phys, unsigned long size, unsigned long flags)
 		struct page *p = pfn_to_page(PHYS_PFN(phys));
 		unsigned int order = get_order(size);
 
-		free_pages((unsigned long) page_address(p), order);
+		__free_pages(p, order);
 	}
 }
 
-- 
2.51.0

[PATCH v2 4/7] riscv: Stop calling page_address() in free_pages()

[Vishal Moola (Oracle)]

free_pages() should be used when we only have a virtual address. We
should call __free_pages() directly on our page instead.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 arch/riscv/mm/init.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 15683ae13fa5..1056c11d3251 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -1624,7 +1624,7 @@ static void __meminit free_pud_table(pud_t *pud_start, p4d_t *p4d)
 	if (PageReserved(page))
 		free_reserved_page(page);
 	else
-		free_pages((unsigned long)page_address(page), 0);
+		__free_pages(page, 0);
 	p4d_clear(p4d);
 }
 
@@ -1646,7 +1646,7 @@ static void __meminit free_vmemmap_storage(struct page *page, size_t size,
 		return;
 	}
 
-	free_pages((unsigned long)page_address(page), order);
+	__free_pages(page, order);
 }
 
 static void __meminit remove_pte_mapping(pte_t *pte_base, unsigned long addr, unsigned long end,
-- 
2.51.0

[PATCH v2 5/7] powerpc: Stop calling page_address() in free_pages()

[Vishal Moola (Oracle)]

free_pages() should be used when we only have a virtual address. We
should call __free_pages() directly on our page instead.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 arch/powerpc/mm/book3s64/radix_pgtable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index be523e5fe9c5..73977dbabcf2 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -780,7 +780,7 @@ static void __meminit free_vmemmap_pages(struct page *page,
 		while (nr_pages--)
 			free_reserved_page(page++);
 	} else
-		free_pages((unsigned long)page_address(page), order);
+		__free_pages(page, order);
 }
 
 static void __meminit remove_pte_table(pte_t *pte_start, unsigned long addr,
-- 
2.51.0

[PATCH v2 6/7] arm64: Stop calling page_address() in free_pages()

[Vishal Moola (Oracle)]

free_pages() should be used when we only have a virtual address. We
should call __free_pages() directly on our page instead.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 arch/arm64/mm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 34e5d78af076..e14a75d0dbd3 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -843,7 +843,7 @@ static void free_hotplug_page_range(struct page *page, size_t size,
 		vmem_altmap_free(altmap, size >> PAGE_SHIFT);
 	} else {
 		WARN_ON(PageReserved(page));
-		free_pages((unsigned long)page_address(page), get_order(size));
+		__free_pages(page, get_order(size));
 	}
 }
 
-- 
2.51.0

[PATCH v2 7/7] virtio_balloon: Stop calling page_address() in free_pages()

[Vishal Moola (Oracle)]

free_pages() should be used when we only have a virtual address. We
should call __free_pages() directly on our page instead.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 drivers/virtio/virtio_balloon.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index eae65136cdfb..d4e6865ce355 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -488,8 +488,7 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
 		page = balloon_page_pop(&vb->free_page_list);
 		if (!page)
 			break;
-		free_pages((unsigned long)page_address(page),
-			   VIRTIO_BALLOON_HINT_BLOCK_ORDER);
+		__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
 	}
 	vb->num_free_page_blocks -= num_returned;
 	spin_unlock_irq(&vb->free_page_list_lock);
-- 
2.51.0

Re: [PATCH v2 3/7] x86: Stop calling page_address() in free_pages()

[Dave Hansen]

On 8/26/25 13:56, Vishal Moola (Oracle) wrote:
> free_pages() should be used when we only have a virtual address. We
> should call __free_pages() directly on our page instead.

Thanks for doing these, Vishal!

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

Re: [PATCH v2 7/7] virtio_balloon: Stop calling page_address() in free_pages()

[David Hildenbrand]

On 26.08.25 22:56, Vishal Moola (Oracle) wrote:
> free_pages() should be used when we only have a virtual address. We
> should call __free_pages() directly on our page instead.
> 
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> ---
>   drivers/virtio/virtio_balloon.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index eae65136cdfb..d4e6865ce355 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -488,8 +488,7 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
>   		page = balloon_page_pop(&vb->free_page_list);
>   		if (!page)
>   			break;
> -		free_pages((unsigned long)page_address(page),
> -			   VIRTIO_BALLOON_HINT_BLOCK_ORDER);
> +		__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
>   	}
>   	vb->num_free_page_blocks -= num_returned;
>   	spin_unlock_irq(&vb->free_page_list_lock);

I think you missed another nastiness of similar kind in 
get_free_page_and_send() where we do

	p = page_address(page);

Just to call

	free_pages((unsigned long)p, VIRTIO_BALLOON_HINT_BLOCK_ORDER);

-- 
Cheers

David / dhildenb

Re: [PATCH v2 6/7] arm64: Stop calling page_address() in free_pages()

[Catalin Marinas]

On Tue, Aug 26, 2025 at 01:56:16PM -0700, Vishal Moola (Oracle) wrote:
> free_pages() should be used when we only have a virtual address. We
> should call __free_pages() directly on our page instead.
> 
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v2 1/7] mm/page_alloc: Add kernel-docs for free_pages()

[SeongJae Park]

On Tue, 26 Aug 2025 13:56:11 -0700 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote:

> Add kernel-docs to free_pages(). This will help callers understand when
> to use it instead of __free_pages().
> 
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Acked-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

[...]

Re: [PATCH v2 7/7] virtio_balloon: Stop calling page_address() in free_pages()

[Vishal Moola (Oracle)]

[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]

On Wed, Aug 27, 2025 at 02:01:01PM +0200, David Hildenbrand wrote:
> On 26.08.25 22:56, Vishal Moola (Oracle) wrote:
> > free_pages() should be used when we only have a virtual address. We
> > should call __free_pages() directly on our page instead.
> > 
> > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> > ---
> >   drivers/virtio/virtio_balloon.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index eae65136cdfb..d4e6865ce355 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -488,8 +488,7 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
> >   		page = balloon_page_pop(&vb->free_page_list);
> >   		if (!page)
> >   			break;
> > -		free_pages((unsigned long)page_address(page),
> > -			   VIRTIO_BALLOON_HINT_BLOCK_ORDER);
> > +		__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
> >   	}
> >   	vb->num_free_page_blocks -= num_returned;
> >   	spin_unlock_irq(&vb->free_page_list_lock);
> 
> I think you missed another nastiness of similar kind in
> get_free_page_and_send() where we do
> 
> 	p = page_address(page);
> 
> Just to call
> 
> 	free_pages((unsigned long)p, VIRTIO_BALLOON_HINT_BLOCK_ORDER);

Thanks for catching that. Andrew can you fold the attached patch into
this one please? It looks like the page_address() call is needed for other
things, but since we're changing the file we might as well clean these
up as well.

I imagine theres more of these lingering in the kernel, but theres so
many callers and I only looked for the ones that were calling
page_address() inline :(.

[-- Attachment #2: 0001-virtio_ballon-Call-__free_pages-in-get_free_page_and.patch --]
[-- Type: text/plain, Size: 1413 bytes --]

From a7d439154c7990418da976e5864b91fce9d49d58 Mon Sep 17 00:00:00 2001
From: "Vishal Moola (Oracle)" <vishal.moola@gmail.com>
Date: Wed, 27 Aug 2025 11:10:22 -0700
Subject: [PATCH] virtio_ballon: Call __free_pages() in
 get_free_page_and_send()

free_pages() should be used when we only have a virtual address. We
should call __free_pages() directly on our page instead.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 drivers/virtio/virtio_balloon.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index d4e6865ce355..7f3fd72678eb 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -718,8 +718,7 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
 	if (vq->num_free > 1) {
 		err = virtqueue_add_inbuf(vq, &sg, 1, p, GFP_KERNEL);
 		if (unlikely(err)) {
-			free_pages((unsigned long)p,
-				   VIRTIO_BALLOON_HINT_BLOCK_ORDER);
+			__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
 			return err;
 		}
 		virtqueue_kick(vq);
@@ -732,7 +731,7 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
 		 * The vq has no available entry to add this page block, so
 		 * just free it.
 		 */
-		free_pages((unsigned long)p, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
+		__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
 	}
 
 	return 0;
-- 
2.51.0

Re: [PATCH v2 7/7] virtio_balloon: Stop calling page_address() in free_pages()

[Matthew Wilcox]

On Wed, Aug 27, 2025 at 11:29:22AM -0700, Vishal Moola (Oracle) wrote:
> I imagine theres more of these lingering in the kernel, but theres so
> many callers and I only looked for the ones that were calling
> page_address() inline :(.

There's only 841 callers of free_page() and free_pages()!

It's a bit of a disease we have, to be honest,  Almost all of
them should be using kmalloc() instead.  To pick on one at random,
sel_read_bool() in security/selinux/selinuxfs.c is the implementation
of read() for some file in selinux.  All it's trying to do is output two
numbers, so it allocates an entire page of memory, prints two numbers
to it (while being VERY CAREFUL not to overflow the buffer!) and copies
the buffer to userspace.

It should just use kmalloc.  Oh, and it should avoid leaking the buffer
if security_get_bool_value() returns an error.

Re: [PATCH v2 7/7] virtio_balloon: Stop calling page_address() in free_pages()

[Michael S. Tsirkin]

On Thu, Aug 28, 2025 at 03:53:56PM +0100, Matthew Wilcox wrote:
> On Wed, Aug 27, 2025 at 11:29:22AM -0700, Vishal Moola (Oracle) wrote:
> > I imagine theres more of these lingering in the kernel, but theres so
> > many callers and I only looked for the ones that were calling
> > page_address() inline :(.
> 
> There's only 841 callers of free_page() and free_pages()!
> 
> It's a bit of a disease we have, to be honest,  Almost all of
> them should be using kmalloc() instead.  To pick on one at random,
> sel_read_bool() in security/selinux/selinuxfs.c is the implementation
> of read() for some file in selinux.  All it's trying to do is output two
> numbers, so it allocates an entire page of memory, prints two numbers
> to it (while being VERY CAREFUL not to overflow the buffer!) and copies
> the buffer to userspace.
> 
> It should just use kmalloc.

Why even kmalloc? Why not have a small array on stack?

>  Oh, and it should avoid leaking the buffer
> if security_get_bool_value() returns an error.

Re: [PATCH v2 5/7] powerpc: Stop calling page_address() in free_pages()

[Ritesh Harjani]

Hi Vishal,

"Vishal Moola (Oracle)" <vishal.moola@gmail.com> writes:

> free_pages() should be used when we only have a virtual address. We
> should call __free_pages() directly on our page instead.
>
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> ---
>  arch/powerpc/mm/book3s64/radix_pgtable.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Please also cc the respective subsystem mailing list if the changes span
in their area. In this case that would be linuxppc-dev@lists.ozlabs.org
( I did it this time )


Thanks for doing the cleanup. Yes, it makes no sense to do page_address()
here and then free_pages() doing virt_to_page() internally.. 

The change looks good to me. Please feel free to add:

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

>
> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
> index be523e5fe9c5..73977dbabcf2 100644
> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
> @@ -780,7 +780,7 @@ static void __meminit free_vmemmap_pages(struct page *page,
>  		while (nr_pages--)
>  			free_reserved_page(page++);
>  	} else
> -		free_pages((unsigned long)page_address(page), order);
> +		__free_pages(page, order);
>  }
>  
>  static void __meminit remove_pte_table(pte_t *pte_start, unsigned long addr,
> -- 
> 2.51.0

Re: [PATCH v2 7/7] virtio_balloon: Stop calling page_address() in free_pages()

[Stephen Smalley]

On Sat, Aug 30, 2025 at 7:48 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Aug 28, 2025 at 03:53:56PM +0100, Matthew Wilcox wrote:
> > On Wed, Aug 27, 2025 at 11:29:22AM -0700, Vishal Moola (Oracle) wrote:
> > > I imagine theres more of these lingering in the kernel, but theres so
> > > many callers and I only looked for the ones that were calling
> > > page_address() inline :(.
> >
> > There's only 841 callers of free_page() and free_pages()!
> >
> > It's a bit of a disease we have, to be honest,  Almost all of
> > them should be using kmalloc() instead.  To pick on one at random,
> > sel_read_bool() in security/selinux/selinuxfs.c is the implementation
> > of read() for some file in selinux.  All it's trying to do is output two
> > numbers, so it allocates an entire page of memory, prints two numbers
> > to it (while being VERY CAREFUL not to overflow the buffer!) and copies
> > the buffer to userspace.
> >
> > It should just use kmalloc.
>
> Why even kmalloc? Why not have a small array on stack?

Patch posted at
https://lore.kernel.org/selinux/20250902131107.13509-2-stephen.smalley.work@gmail.com/T/#u

>
> >  Oh, and it should avoid leaking the buffer
> > if security_get_bool_value() returns an error.