linux-kernel - Re: [PATH v3] tracing: Fix race condition in kprobe initialization causing NULL pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20251001003707.3eaf9ad062d5cad96f49b9ba@kernel.org>
Date: Wed, 1 Oct 2025 00:37:07 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: chenyuan_fl@....com, john.ogness@...utronix.de, rostedt@...dmis.org,
 bigeasy@...utronix.de, chenyuan@...inos.cn, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, mathieu.desnoyers@...icios.com,
 mhiramat@...nel.org
Subject: Re: [PATH v3] tracing: Fix race condition in kprobe initialization
 causing NULL pointer dereference

On Tue, 30 Sep 2025 10:46:45 +0200
Peter Zijlstra <peterz@...radead.org> wrote:

> On Tue, Sep 30, 2025 at 09:18:48AM +0100, chenyuan_fl@....com wrote:
> 
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index 842383fbc03b..98b838591edc 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -274,19 +274,19 @@ struct event_file_link {
> >  static inline bool trace_probe_test_flag(struct trace_probe *tp,
> >  					 unsigned int flag)
> >  {
> > -	return !!(tp->event->flags & flag);
> > +	return !!(smp_load_acquire(&tp->event->flags) & flag);
> >  }
> >  
> >  static inline void trace_probe_set_flag(struct trace_probe *tp,
> >  					unsigned int flag)
> >  {
> > -	tp->event->flags |= flag;
> > +	smp_store_release(&tp->event->flags, tp->event->flags | flag);
> >  }
> >  
> >  static inline void trace_probe_clear_flag(struct trace_probe *tp,
> >  					  unsigned int flag)
> >  {
> > -	tp->event->flags &= ~flag;
> > +	smp_store_release(&tp->event->flags, tp->event->flags & ~flag);
> >  }
> 
> 
> I _think_ the clear one is superfluous. Is there anything that cares
> about stores done before the clear when the flag is found not set?
> 
> Also, code like:
> 
> static int fentry_dispatcher(struct fprobe *fp, unsigned long entry_ip,
> 			     unsigned long ret_ip, struct ftrace_regs *fregs,
> 			     void *entry_data)
> {
> 	struct trace_fprobe *tf = container_of(fp, struct trace_fprobe, fp);
> 	int ret = 0;
> 
> 	if (trace_probe_test_flag(&tf->tp, TP_FLAG_TRACE))
> 		fentry_trace_func(tf, entry_ip, fregs);
> 
> #ifdef CONFIG_PERF_EVENTS
> 	if (trace_probe_test_flag(&tf->tp, TP_FLAG_PROFILE))
> 		ret = fentry_perf_func(tf, entry_ip, fregs);
> #endif
> 	return ret;
> }
> 
> 
> Will now have two barriers; where one would suffice, eg.
> 
> 	flags = smp_load_acquire(&tp->event->flags);
> 
> 	if (flags & TP_FLAG_TRACE)
> 		fentry_trace_func(...);
> 
> 	if (flags & TP_FLAG_PROFILE)
> 		fentry_perf_func(...);
> 
> Should be just fine afaict.

Looks good to me. We should replace trace_probe_test_flag()
with trace_probe_load_flag().

Thanks,

> 
> 
> Is this something anybody cares about?


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>