[PATCH] KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing

[PATCH] KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing

[Vincent Donnefort]

In the very unlikely event of a failure in __map_hyp, the allocated
backing pages are leaked in hyp_trace_buffer_alloc_bpages_backing(). Fix
this by freeing the pages on error.

Fixes: 3aed038aac8d ("KVM: arm64: Add trace remote for the nVHE/pKVM hyp")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
index 2411b4c32932..5fbbfd40376f 100644
--- a/arch/arm64/kvm/hyp_trace.c
+++ b/arch/arm64/kvm/hyp_trace.c
@@ -160,6 +160,7 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
 	int nr_bpages = (PAGE_ALIGN(size) / PAGE_SIZE) + 1;
 	size_t backing_size;
 	void *start;
+	int ret;
 
 	backing_size = PAGE_ALIGN(sizeof(struct simple_buffer_page) * nr_bpages *
 				  num_possible_cpus());
@@ -171,7 +172,11 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
 	trace_buffer->desc->bpages_backing_start = (unsigned long)start;
 	trace_buffer->desc->bpages_backing_size = backing_size;
 
-	return __map_hyp(start, backing_size);
+	ret = __map_hyp(start, backing_size);
+	if (ret)
+		free_pages_exact(start, backing_size);
+
+	return ret;
 }
 
 static void hyp_trace_buffer_free_bpages_backing(struct hyp_trace_buffer *trace_buffer)

base-commit: 8cdeaa50eae8dad34885515f62559ee83e7e8dda
-- 
2.55.0.rc2.803.g1fd1e6609c-goog

Re: [PATCH] KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] trace_remote_alloc_buffer leaks page allocations on the error path if a per-CPU ring buffer allocation fails.
--

--- Patch [1]: [PATCH] KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing ---
commit 21f10293a05c7439f7a1d1be691e92ef2d36331a
Author: Vincent Donnefort <vdonnefort@google.com>

KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing

In the very unlikely event of a failure in __map_hyp, the allocated
backing pages are leaked in hyp_trace_buffer_alloc_bpages_backing(). Fix
this by freeing the pages on error.

Fixes: 3aed038aac8d ("KVM: arm64: Add trace remote for the nVHE/pKVM hyp")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

> diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> index 2411b4c32932c..5fbbfd40376f7 100644
> --- a/arch/arm64/kvm/hyp_trace.c
> +++ b/arch/arm64/kvm/hyp_trace.c
> @@ -160,6 +160,7 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
>  	int nr_bpages = (PAGE_ALIGN(size) / PAGE_SIZE) + 1;
>  	size_t backing_size;
>  	void *start;
> +	int ret;
>  
>  	backing_size = PAGE_ALIGN(sizeof(struct simple_buffer_page) * nr_bpages *
>  				  num_possible_cpus());
> @@ -171,7 +172,11 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
>  	trace_buffer->desc->bpages_backing_start = (unsigned long)start;
>  	trace_buffer->desc->bpages_backing_size = backing_size;
>  
> -	return __map_hyp(start, backing_size);
> +	ret = __map_hyp(start, backing_size);
> +	if (ret)
> +		free_pages_exact(start, backing_size);
> +
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but since this patch fixes a memory leak in the 
trace buffer allocation, I noticed a potential leak in a related error path.

In kernel/trace/trace_remote.c:trace_remote_alloc_buffer(), if an allocation
fails, the code jumps to the err label:

	rb_desc->meta_va = (unsigned long)__get_free_page(GFP_KERNEL);
	if (!rb_desc->meta_va)
		goto err;

	for (id = 0; id < nr_pages; id++) {
		rb_desc->page_va[id] = (unsigned long)__get_free_page(GFP_KERNEL);
		if (!rb_desc->page_va[id])
			goto err;

		rb_desc->nr_page_va++;
	}
	desc->nr_cpus++;

Because desc->nr_cpus is only incremented after all pages for the current CPU
are allocated, any successfully allocated pages for the failing CPU are left
out of the count.

When the error path calls trace_remote_free_buffer(), it iterates based on
desc->nr_cpus:

	for_each_ring_buffer_desc(rb_desc, cpu, desc) {
		unsigned int id;

		free_page(rb_desc->meta_va);

		for (id = 0; id < rb_desc->nr_page_va; id++)
			free_page(rb_desc->page_va[id]);
	}

Does this skip the partially initialized ring buffer for the failing CPU and 
leak its allocated pages?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707165029.3410701-1-vdonnefort@google.com?part=1

Re: [PATCH] KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing

[Fuad Tabba]

Hi Vincent,

On Tue, 7 Jul 2026 at 17:50, Vincent Donnefort <vdonnefort@google.com> wrote:
>
> In the very unlikely event of a failure in __map_hyp, the allocated
> backing pages are leaked in hyp_trace_buffer_alloc_bpages_backing(). Fix
> this by freeing the pages on error.
>
> Fixes: 3aed038aac8d ("KVM: arm64: Add trace remote for the nVHE/pKVM hyp")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
>
> diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> index 2411b4c32932..5fbbfd40376f 100644
> --- a/arch/arm64/kvm/hyp_trace.c
> +++ b/arch/arm64/kvm/hyp_trace.c
> @@ -160,6 +160,7 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
>         int nr_bpages = (PAGE_ALIGN(size) / PAGE_SIZE) + 1;
>         size_t backing_size;
>         void *start;
> +       int ret;
>
>         backing_size = PAGE_ALIGN(sizeof(struct simple_buffer_page) * nr_bpages *
>                                   num_possible_cpus());
> @@ -171,7 +172,11 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
>         trace_buffer->desc->bpages_backing_start = (unsigned long)start;
>         trace_buffer->desc->bpages_backing_size = backing_size;
>
> -       return __map_hyp(start, backing_size);
> +       ret = __map_hyp(start, backing_size);
> +       if (ret)
> +               free_pages_exact(start, backing_size);
> +
> +       return ret;
>  }

nit: would be a bit cleaner to do move the desc assignment to after
the check. Also, I think sashiko found another real bug, don't think
it's serious but worth fixing.

That said...

Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
Tested-by: Fuad Tabba < fuad.tabba@linux.dev>

Cheers,
/fuad

>
>  static void hyp_trace_buffer_free_bpages_backing(struct hyp_trace_buffer *trace_buffer)
>
> base-commit: 8cdeaa50eae8dad34885515f62559ee83e7e8dda
> --
> 2.55.0.rc2.803.g1fd1e6609c-goog
>

Re: [PATCH] KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing

[Vincent Donnefort]

On Tue, Jul 07, 2026 at 07:32:44PM +0100, Fuad Tabba wrote:
> Hi Vincent,
> 
> On Tue, 7 Jul 2026 at 17:50, Vincent Donnefort <vdonnefort@google.com> wrote:
> >
> > In the very unlikely event of a failure in __map_hyp, the allocated
> > backing pages are leaked in hyp_trace_buffer_alloc_bpages_backing(). Fix
> > this by freeing the pages on error.
> >
> > Fixes: 3aed038aac8d ("KVM: arm64: Add trace remote for the nVHE/pKVM hyp")
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> >
> > diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> > index 2411b4c32932..5fbbfd40376f 100644
> > --- a/arch/arm64/kvm/hyp_trace.c
> > +++ b/arch/arm64/kvm/hyp_trace.c
> > @@ -160,6 +160,7 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
> >         int nr_bpages = (PAGE_ALIGN(size) / PAGE_SIZE) + 1;
> >         size_t backing_size;
> >         void *start;
> > +       int ret;
> >
> >         backing_size = PAGE_ALIGN(sizeof(struct simple_buffer_page) * nr_bpages *
> >                                   num_possible_cpus());
> > @@ -171,7 +172,11 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
> >         trace_buffer->desc->bpages_backing_start = (unsigned long)start;
> >         trace_buffer->desc->bpages_backing_size = backing_size;
> >
> > -       return __map_hyp(start, backing_size);
> > +       ret = __map_hyp(start, backing_size);
> > +       if (ret)
> > +               free_pages_exact(start, backing_size);
> > +
> > +       return ret;
> >  }
> 
> nit: would be a bit cleaner to do move the desc assignment to after
> the check. Also, I think sashiko found another real bug, don't think
> it's serious but worth fixing.

Ha yes good point.

For the other bug, I saw it but that will be for the tracing ML

> 
> That said...
> 
> Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
> Tested-by: Fuad Tabba < fuad.tabba@linux.dev>

Thanks

> 
> Cheers,
> /fuad
> 
> >
> >  static void hyp_trace_buffer_free_bpages_backing(struct hyp_trace_buffer *trace_buffer)
> >
> > base-commit: 8cdeaa50eae8dad34885515f62559ee83e7e8dda
> > --
> > 2.55.0.rc2.803.g1fd1e6609c-goog
> >

Re: [PATCH] KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing

[Vincent Donnefort]

On Tue, Jul 07, 2026 at 05:02:44PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] trace_remote_alloc_buffer leaks page allocations on the error path if a per-CPU ring buffer allocation fails.
> --
> 
> --- Patch [1]: [PATCH] KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing ---
> commit 21f10293a05c7439f7a1d1be691e92ef2d36331a
> Author: Vincent Donnefort <vdonnefort@google.com>
> 
> KVM: arm64: Fix potential leak in hyp_trace_buffer_alloc_bpages_backing
> 
> In the very unlikely event of a failure in __map_hyp, the allocated
> backing pages are leaked in hyp_trace_buffer_alloc_bpages_backing(). Fix
> this by freeing the pages on error.
> 
> Fixes: 3aed038aac8d ("KVM: arm64: Add trace remote for the nVHE/pKVM hyp")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> 
> > diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> > index 2411b4c32932c..5fbbfd40376f7 100644
> > --- a/arch/arm64/kvm/hyp_trace.c
> > +++ b/arch/arm64/kvm/hyp_trace.c
> > @@ -160,6 +160,7 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
> >  	int nr_bpages = (PAGE_ALIGN(size) / PAGE_SIZE) + 1;
> >  	size_t backing_size;
> >  	void *start;
> > +	int ret;
> >  
> >  	backing_size = PAGE_ALIGN(sizeof(struct simple_buffer_page) * nr_bpages *
> >  				  num_possible_cpus());
> > @@ -171,7 +172,11 @@ static int hyp_trace_buffer_alloc_bpages_backing(struct hyp_trace_buffer *trace_
> >  	trace_buffer->desc->bpages_backing_start = (unsigned long)start;
> >  	trace_buffer->desc->bpages_backing_size = backing_size;
> >  
> > -	return __map_hyp(start, backing_size);
> > +	ret = __map_hyp(start, backing_size);
> > +	if (ret)
> > +		free_pages_exact(start, backing_size);
> > +
> > +	return ret;
> >  }
> 
> [Severity: High]
> This is a pre-existing issue, but since this patch fixes a memory leak in the 
> trace buffer allocation, I noticed a potential leak in a related error path.
> 
> In kernel/trace/trace_remote.c:trace_remote_alloc_buffer(), if an allocation
> fails, the code jumps to the err label:
> 
> 	rb_desc->meta_va = (unsigned long)__get_free_page(GFP_KERNEL);
> 	if (!rb_desc->meta_va)
> 		goto err;
> 
> 	for (id = 0; id < nr_pages; id++) {
> 		rb_desc->page_va[id] = (unsigned long)__get_free_page(GFP_KERNEL);
> 		if (!rb_desc->page_va[id])
> 			goto err;
> 
> 		rb_desc->nr_page_va++;
> 	}
> 	desc->nr_cpus++;
> 
> Because desc->nr_cpus is only incremented after all pages for the current CPU
> are allocated, any successfully allocated pages for the failing CPU are left
> out of the count.
> 
> When the error path calls trace_remote_free_buffer(), it iterates based on
> desc->nr_cpus:
> 
> 	for_each_ring_buffer_desc(rb_desc, cpu, desc) {
> 		unsigned int id;
> 
> 		free_page(rb_desc->meta_va);
> 
> 		for (id = 0; id < rb_desc->nr_page_va; id++)
> 			free_page(rb_desc->page_va[id]);
> 	}
> 
> Does this skip the partially initialized ring buffer for the failing CPU and 
> leak its allocated pages?

https://lore.kernel.org/all/20260708133201.295072-1-vdonnefort@google.com/

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260707165029.3410701-1-vdonnefort@google.com?part=1
>