linux-kernel - Re: [PATCH v2 6/6] tracing/dynevent: Adopt guard() and scoped

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <20250105175736.bc0f4568f5e64ea5608ab50d@kernel.org>
Date: Sun, 5 Jan 2025 17:57:36 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Naveen N Rao <naveen@...nel.org>, Anil S Keshavamurthy
 <anil.s.keshavamurthy@...el.com>, "David S . Miller" <davem@...emloft.net>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Oleg Nesterov
 <oleg@...hat.com>, Tzvetomir Stoyanov <tz.stoyanov@...il.com>,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/6] tracing/dynevent: Adopt guard() and
 scoped_guard()

On Fri, 27 Dec 2024 10:12:18 -0500
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Sat, 30 Nov 2024 01:48:40 +0900
> "Masami Hiramatsu (Google)" <mhiramat@...nel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> > 
> > Use guard() or scoped_guard() in dynamic events for critical sections
> > rather than discrete lock/unlock pairs.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> > ---
> >  Changes in v2:
> >   - Use scoped_guard() instead of guard() to avoid goto warnings.
> 
> I forgot you touched this file, and added a free guard to it which
> conflicts. That said, I have some issues with this patch.
> 
> > ---
> >  kernel/trace/trace_dynevent.c |   35 ++++++++++++++++-------------------
> >  1 file changed, 16 insertions(+), 19 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
> > index 4376887e0d8a..eb8f669c15e1 100644
> > --- a/kernel/trace/trace_dynevent.c
> > +++ b/kernel/trace/trace_dynevent.c
> > @@ -63,9 +63,8 @@ int dyn_event_register(struct dyn_event_operations *ops)
> >  		return -EINVAL;
> >  
> >  	INIT_LIST_HEAD(&ops->list);
> > -	mutex_lock(&dyn_event_ops_mutex);
> > +	guard(mutex)(&dyn_event_ops_mutex);
> >  	list_add_tail(&ops->list, &dyn_event_ops_list);
> > -	mutex_unlock(&dyn_event_ops_mutex);
> 
> I don't care for a scoped guards around simple paths. The great thing about
> guard()s is that they help prevent bugs when you have complex code between
> the lock and unlock that may need to exit.
> 
> But replacing:
> 
> 
> 	mutex_lock(&dyn_event_ops_mutex);
>  	list_add_tail(&ops->list, &dyn_event_ops_list);
> 	mutex_unlock(&dyn_event_ops_mutex);
>  }
> 
> 
> With:
> 
> 	guard(mutex)(&dyn_event_ops_mutex);
>  	list_add_tail(&ops->list, &dyn_event_ops_list);
>  }
> 
> is overkill to me. The first one is much easier to read. The second one
> begs the question, "why did they use a guard here?"

OK. fair enough. I think I was getting a little too excited. :(

> 
> >  	return 0;
> >  }
> >  
> > @@ -106,20 +105,20 @@ int dyn_event_release(const char *raw_command,
> > struct dyn_event_operations *type goto out;
> >  	}
> >  
> > -	mutex_lock(&event_mutex);
> > -	for_each_dyn_event_safe(pos, n) {
> > -		if (type && type != pos->ops)
> > -			continue;
> > -		if (!pos->ops->match(system, event,
> > -				argc - 1, (const char **)argv + 1, pos))
> > -			continue;
> > -
> > -		ret = pos->ops->free(pos);
> > -		if (ret)
> > -			break;
> > +	scoped_guard(mutex, &event_mutex) {
> > +		for_each_dyn_event_safe(pos, n) {
> > +			if (type && type != pos->ops)
> > +				continue;
> > +			if (!pos->ops->match(system, event,
> > +					argc - 1, (const char **)argv +
> > 1, pos))
> > +				continue;
> > +
> > +			ret = pos->ops->free(pos);
> > +			if (ret)
> > +				break;
> > +		}
> > +		tracing_reset_all_online_cpus();
> >  	}
> 
> This scoped_guard() doesn't give us anything. We still have the out label
> below (which my patch removes).

OK.

> > -	tracing_reset_all_online_cpus();
> > -	mutex_unlock(&event_mutex);
> >  out:
> >  	argv_free(argv);
> >  	return ret;
> > @@ -133,13 +132,12 @@ static int create_dyn_event(const char *raw_command)
> >  	if (raw_command[0] == '-' || raw_command[0] == '!')
> >  		return dyn_event_release(raw_command, NULL);
> >  
> > -	mutex_lock(&dyn_event_ops_mutex);
> > +	guard(mutex)(&dyn_event_ops_mutex);
> >  	list_for_each_entry(ops, &dyn_event_ops_list, list) {
> >  		ret = ops->create(raw_command);
> >  		if (!ret || ret != -ECANCELED)
> >  			break;
> >  	}
> 
> I also don't think this helps much here.

OK.

> > -	mutex_unlock(&dyn_event_ops_mutex);
> >  	if (ret == -ECANCELED)
> >  		ret = -EINVAL;
> >  
> > @@ -198,7 +196,7 @@ int dyn_events_release_all(struct dyn_event_operations *type)
> >  	struct dyn_event *ev, *tmp;
> >  	int ret = 0;
> >  
> > -	mutex_lock(&event_mutex);
> > +	guard(mutex)(&event_mutex);
> >  	for_each_dyn_event(ev) {
> >  		if (type && ev->ops != type)
> >  			continue;
> > @@ -216,7 +214,6 @@ int dyn_events_release_all(struct dyn_event_operations *type)
> >  	}
> >  out:
> 
> And the same issue here too. Why the guard when you still need to do the
> goto?

Yeah, we still need to call tracing_reset_all_online_cpus() here.

> 
> 
> >  	tracing_reset_all_online_cpus();
> > -	mutex_unlock(&event_mutex);
> >  
> >  	return ret;
> >  }
> 
> 
> There's a reason I looked at this file an didn't add any guards to it (when
> I forgot that you touched it). They are all special cases, and I rather
> avoid adding special case guards to handle it. I don't believe it makes it
> any less error prone.
> 
> Are you OK with dropping this patch?

Yeah, let's drop it.

Thanks for your review!


> 
> -- Steve


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