[v2] Cleanup free_pages() misuse

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

Posted by Vishal Moola (Oracle) 1 month, 1 week ago

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 7/7] virtio_balloon: Stop calling page_address() in free_pages()

Posted by David Hildenbrand 1 month, 1 week ago

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 7/7] virtio_balloon: Stop calling page_address() in free_pages()

Posted by Vishal Moola (Oracle) 1 month ago

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 :(.
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()

Posted by Michael S. Tsirkin 2 weeks, 3 days ago

On Wed, Aug 27, 2025 at 11:29:22AM -0700, Vishal Moola (Oracle) wrote:
> 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 :(.

Confused what is folded where.
If you want me to apply these things please just fold whatever
you want folded and post.
Thanks!


> >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()

Posted by Vishal Moola (Oracle) 2 weeks, 3 days ago

On Mon, Sep 15, 2025 at 06:45:51PM -0400, Michael S. Tsirkin wrote:
> On Wed, Aug 27, 2025 at 11:29:22AM -0700, Vishal Moola (Oracle) wrote:
> > 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 :(.
> 
> Confused what is folded where.
> If you want me to apply these things please just fold whatever
> you want folded and post.
> Thanks!

Hi Michael, you shouldn't need to worry about this as Andrew will take
all patches in the patchset through the mm tree. See v3 for the updated
version that will be merged:
https://lore.kernel.org/linux-mm/20250903185921.1785167-8-vishal.moola@gmail.com/

I get why you might've gotten confused. The 'fold' patch I was referencing
was included as an attachment rather than inline (but I've figured out a
way to inline things for the future).

> 
> > >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()

Posted by Matthew Wilcox 1 month ago

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()

Posted by Michael S. Tsirkin 1 month ago

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 7/7] virtio_balloon: Stop calling page_address() in free_pages()

Posted by Stephen Smalley 1 month ago

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.