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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20241227101218.5948129c@gandalf.local.home>
Date: Fri, 27 Dec 2024 10:12:18 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.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 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?"

>  	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).

> -	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.
> -	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?

>  	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?

-- Steve