linux-kernel - Re: [PATCH 2/3] tracing: Add bulk garbage collection of freeing event_trigger

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] [day] [month] [year] [list]

Message-ID: <20251121145150.3716a1a1@gandalf.local.home>
Date: Fri, 21 Nov 2025 14:51:50 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc: Steven Rostedt <rostedt@...nel.org>, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, Mark Rutland <mark.rutland@....com>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Andrew Morton
 <akpm@...ux-foundation.org>, Tom Zanussi <zanussi@...nel.org>
Subject: Re: [PATCH 2/3] tracing: Add bulk garbage collection of freeing
 event_trigger_data

On Sat, 22 Nov 2025 00:12:06 +0900
Masami Hiramatsu (Google) <mhiramat@...nel.org> wrote:

> >  /* Avoid typos */
> > diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> > index e5dcfcbb2cd5..16e3449f3cfe 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -6,26 +6,76 @@
> >   */
> >  
> >  #include <linux/security.h>
> > +#include <linux/kthread.h>
> >  #include <linux/module.h>
> >  #include <linux/ctype.h>
> >  #include <linux/mutex.h>
> >  #include <linux/slab.h>
> >  #include <linux/rculist.h>
> > +#include <linux/llist.h>  
> 
> nit: Shouldn't we include this in "trace.h" too, because llist_node is used?

You mean to move it to trace.h instead of here?

> 
> >  
> >  #include "trace.h"
> >  
> >  static LIST_HEAD(trigger_commands);
> >  static DEFINE_MUTEX(trigger_cmd_mutex);
> >  
> > +static struct task_struct *trigger_kthread;
> > +static struct llist_head trigger_data_free_list;
> > +static DEFINE_MUTEX(trigger_data_kthread_mutex);
> > +
> > +/* Bulk garbage collection of event_trigger_data elements */
> > +static int trigger_kthread_fn(void *ignore)
> > +{
> > +	struct event_trigger_data *data, *tmp;
> > +	struct llist_node *llnodes;
> > +
> > +	/* Once this task starts, it lives forever */
> > +	for (;;) {
> > +		set_current_state(TASK_INTERRUPTIBLE);
> > +		if (llist_empty(&trigger_data_free_list))
> > +			schedule();
> > +
> > +		__set_current_state(TASK_RUNNING);
> > +
> > +		llnodes = llist_del_all(&trigger_data_free_list);
> > +
> > +		/* make sure current triggers exit before free */
> > +		tracepoint_synchronize_unregister();
> > +
> > +		llist_for_each_entry_safe(data, tmp, llnodes, llist)
> > +			kfree(data);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  void trigger_data_free(struct event_trigger_data *data)
> >  {
> >  	if (data->cmd_ops->set_filter)
> >  		data->cmd_ops->set_filter(NULL, data, NULL);
> >  
> > -	/* make sure current triggers exit before free */
> > -	tracepoint_synchronize_unregister();
> > +	if (unlikely(!trigger_kthread)) {
> > +		guard(mutex)(&trigger_data_kthread_mutex);
> > +		/* Check again after taking mutex */
> > +		if (!trigger_kthread) {
> > +			struct task_struct *kthread;
> > +
> > +			kthread = kthread_create(trigger_kthread_fn, NULL,
> > +						 "trigger_data_free");
> > +			if (!IS_ERR(kthread))
> > +				WRITE_ONCE(trigger_kthread, kthread);
> > +		}
> > +	}
> > +  
> 
> Hmm,
> 	/* This continues above error case, but we should do it without lock. */
> ?

Does this really need a comment? The lock is only needed to make sure we
only create one kthread. If that fails, then we do it the slow way. The
code below is unrelated to the lock. The lock is for kthread creation, not
the trigger_kthread variable itself.

-- Steve

> 
> > +	if (!trigger_kthread) {
> > +		/* Do it the slow way */
> > +		tracepoint_synchronize_unregister();
> > +		kfree(data);
> > +		return;
> > +	}
> >  
> > -	kfree(data);
> > +	llist_add(&data->llist, &trigger_data_free_list);
> > +	wake_up_process(trigger_kthread);
> >  }
> >  
> >  static inline void data_ops_trigger(struct event_trigger_data *data,
> > -- 
> > 2.51.0
> > 
> >   
> 
>