linux-kernel - Re: [PATCH 1/2] tracing: Simplify and fix "buffered event" synchronization

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: <20231129095826.1aec6381@gandalf.local.home>
Date:   Wed, 29 Nov 2023 09:58:26 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Petr Pavlu <petr.pavlu@...e.com>
Cc:     mhiramat@...nel.org, mathieu.desnoyers@...icios.com,
        zhengyejian1@...wei.com, linux-trace-kernel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] tracing: Simplify and fix "buffered event"
 synchronization

On Wed, 29 Nov 2023 10:22:23 +0100
Petr Pavlu <petr.pavlu@...e.com> wrote:

> Yes, I believe this should address this potential race condition.
> 
> An alternative would be instead to update
> trace_event_buffer_lock_reserve() as follows:
> 
> 	if (this_cpu_inc_return(trace_buffered_event_cnt) == 1) {
> 		smp_rmb();

This is the problem I have with your approach. That smp_rmb() is in the
highly critical path. On some architectures, this has a significant impact
on the overhead of this code. This path is called during code execution and
increases the overhead of the tracing infrastructure.

If I'm given two solutions where one adds a smp_rmb() to the critical path
and the other just slows down the non-critical path more, I'll take the
slow down of non-critical path every time.

> 		if ((entry = __this_cpu_read(trace_buffered_event))) {
> 			[...]
> 
> That saves the synchronize_rcu() call but additionally modifies
> trace_buffered_event_cnt even if trace_buffered_event is currently NULL.
> 
> Another alternative is the approach taken by my patch which avoids more
> RCU work and unnecessary memory modifications.
> 
> I'd be interested if you could have a look again at what I'm proposing
> in my patch. I think it simplifies the code while addressing these
> problems as well. However, if you have reservations about that approach
> then it is ok, I can fix the found problems individually as discussed.

Fix this without adding any memory barriers to the critical path, then I'll
take another look.

FYI, this code was designed in the first place to avoid adding memory
barriers in the critical path.

Thanks!

-- Steve