Re: [GIT PULL] Performance events changes for v7.1

Re: [GIT PULL] Performance events changes for v7.1

[Namhyung Kim]

Hello,

It seems sashio removes receipients in a reply.  Added PeterZ.

On Mon, Apr 13, 2026 at 09:41:09AM +0000, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 2 potential issue(s):
> - [Critical] Pre-existing Stack Buffer Overflow in AMD IBS NMI handler on CPUs supporting `IBS_CAPS_OPDATA4`.
> - [High] Reference count leak of `perf_ctx_data` in `attach_global_ctx_data()` when racing with a concurrent non-global context creation.
> --
> 
> commit 167b4d230f2ffe8284d5afdbd849ef2f4f05ef9a
> Author: Ingo Molnar <mingo@kernel.org>
> 
> Performance events changes for v7.1
> 
> This commit pulls in various performance event changes for v7.1. It includes
> an optimization to allocate task_ctx_data quickly on large systems, updates
> and fixes for the AMD PMU driver IBS support, Intel PEBS constraint
> validation, and MSR driver changes.
> 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -5487,6 +5487,12 @@ attach_global_ctx_data(struct kmem_cache *ctx_cache)
> >  					cd = NULL;
> >  			}
> >  			if (!cd) {
> > +				/*
> > +				 * Try to allocate context quickly before
> > +				 * traversing the whole thread list again.
> > +				 */
> > +				if (!attach_task_ctx_data(p, ctx_cache, true, GFP_NOWAIT))
> > +					continue;
> >  				get_task_struct(p);
> >  				goto alloc;
> >  			}
> 
> Could the new GFP_NOWAIT fast-path in attach_global_ctx_data() leak the
> perf_ctx_data reference if it races with a concurrent non-global context
> creation?
> 
> If attach_task_ctx_data() races with a per-task event attaching a non-global
> context, it will find an existing context where old is not NULL. It increments
> old->refcount and returns 0, but intentionally ignores the global argument
> and does not set old->global to 1.
> 
> Because it returns success, attach_global_ctx_data() executes continue and
> skips to the next thread. The context for the thread is left with an
> extra reference but global remains 0.

Right, it looks like an existing problem.  It should set the old->global
like below:

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 95d7a3e57268b6b4..af773bc750495706 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5447,6 +5447,8 @@ attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache,
                }
 
                if (refcount_inc_not_zero(&old->refcount)) {
+                       if (global)
+                               old->global = 1;
                        free_perf_ctx_data(cd); /* unused */
                        return 0;
                }

> 
> Later, when the global context is detached, __detach_global_ctx_data()
> iterates over all threads and only calls detach_task_ctx_data(p) if
> cd and cd->global are true. Since cd->global is 0, the context is skipped,
> permanently leaking the perf_ctx_data allocation.
> 
> > @@ -5497,7 +5503,7 @@ attach_global_ctx_data(struct kmem_cache *ctx_cache)
> >  
> >  	return 0;
> >  alloc:
> > -	ret = attach_task_ctx_data(p, ctx_cache, true);
> > +	ret = attach_task_ctx_data(p, ctx_cache, true, GFP_KERNEL);
> >  	put_task_struct(p);
> >  	if (ret) {
> >  		__detach_global_ctx_data();
> 
> Can this result in a double-increment leak? If the same race occurs,
> attach_task_ctx_data() bumps the refcount without setting global to 1 and
> returns success. Then goto again restarts the loop.
> 
> When the loop reaches the thread again, it evaluates:
> 
> 	cd = rcu_dereference(p->perf_ctx_data);
> 	if (cd && !cd->global) {
> 		cd->global = 1;
> 		refcount_inc_not_zero(&cd->refcount);
> 	}
> 
> This bumps the refcount a second time. Since __detach_global_ctx_data()
> only decrements once, the extra reference is leaked.

Looks correct and should be fixed by the above change as well.  I'll
send a formal patch if you're ok.

Thanks,
Namhyung

Re: [GIT PULL] Performance events changes for v7.1

[Ravi Bangoria]

>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -698,6 +698,8 @@
>>  #define MSR_AMD64_IBSBRTARGET		0xc001103b
>>  #define MSR_AMD64_ICIBSEXTDCTL		0xc001103c
>>  #define MSR_AMD64_IBSOPDATA4		0xc001103d
>> +#define MSR_AMD64_IBSOPCTL2		0xc001103e
>> +#define MSR_AMD64_IBSFETCHCTL2		0xc001103f
>>  #define MSR_AMD64_IBS_REG_COUNT_MAX	8 /* includes MSR_AMD64_IBSBRTARGET */
> 
> This isn't a regression from the current patch, but looking at the adjacent
> MSR handling code, is there a potential stack buffer overflow in the IBS
> NMI handler on CPUs supporting IBS_CAPS_OPDATA4?
> 
> In arch/x86/events/amd/ibs.c, struct perf_ibs_data is allocated on the
> stack in perf_ibs_handle_irq(). Its regs array is sized using
> MSR_AMD64_IBS_REG_COUNT_MAX, which is 8.
> 
> When handling a perf_ibs_op event, the initial read loop fetches up to 7
> MSRs, advancing the pointer buf to &ibs_data.regs[7].
> 
> Subsequently, the handler reads additional MSRs depending on CPU caps:
> 
> 	if (event->attr.sample_type & PERF_SAMPLE_RAW) {
> 		if (perf_ibs == &perf_ibs_op) {
> 			if (ibs_caps & IBS_CAPS_BRNTRGT) {
> 				rdmsrq(MSR_AMD64_IBSBRTARGET, *buf++);
> 				br_target_idx = size;
> 				size++;
> 			}
> 			if (ibs_caps & IBS_CAPS_OPDATA4) {
> 				rdmsrq(MSR_AMD64_IBSOPDATA4, *buf++);
> 				size++;
> 			}
> 		}
> 
> If the CPU supports IBS_CAPS_BRNTRGT, MSR_AMD64_IBSBRTARGET is read into
> regs[7], advancing buf to &regs[8].
> 
> If the CPU also supports IBS_CAPS_OPDATA4, it reads MSR_AMD64_IBSOPDATA4 into
> the next pointer position, which translates to regs[8]. This writes one
> element past the end of the 8-element array, overwriting adjacent local
> variables on the stack.
> 
> Furthermore, ibs_data.size is incremented to 9, so
> perf_sample_save_raw_data() will subsequently read 72 bytes of regs,
> exposing the corrupted stack data to user-space in the raw perf record. Since
> unprivileged users can configure RAW IBS events, this might lead to
> unintended behavior.
> 
> Should MSR_AMD64_IBS_REG_COUNT_MAX be increased to account for
> IBS_CAPS_OPDATA4?

Thanks for the bug report.

While this seems like a genuine overflow issue, IbsOpData4 has been
deprecated from long back. I was unable to find any platform that
supports it. So it's not a real a concern.

Thanks,
Ravi