All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: rostedt <rostedt@goodmis.org>
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
	"Joel Fernandes, Google" <joel@joelfernandes.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	paulmck <paulmck@linux.ibm.com>,
	Boqun Feng <boqun.feng@gmail.com>,
	Will Deacon <will.deacon@arm.com>,
	David Howells <dhowells@redhat.com>,
	Alan Stern <stern@rowland.harvard.edu>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH 1/1] Fix: trace sched switch start/stop racy updates
Date: Fri, 16 Aug 2019 13:19:20 -0400 (EDT)	[thread overview]
Message-ID: <623129606.21592.1565975960497.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <20190816122539.34fada7b@oasis.local.home>

----- On Aug 16, 2019, at 12:25 PM, rostedt rostedt@goodmis.org wrote:

> On Fri, 16 Aug 2019 10:26:43 -0400 Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
[...]
>> 
>> Also, write and read to/from those variables should be done with
>> WRITE_ONCE() and READ_ONCE(), given that those are read within tracing
>> probes without holding the sched_register_mutex.
>> 
> 
> I understand the READ_ONCE() but is the WRITE_ONCE() truly necessary?
> It's done while holding the mutex. It's not that critical of a path,
> and makes the code look ugly.

The update is done while holding the mutex, but the read-side does not
hold that mutex, so it can observe the intermediate state caused by
store-tearing or invented stores which can be generated by the compiler
on the update-side.

Please refer to the following LWN article:

https://lwn.net/Articles/793253/

Sections:
- "Store tearing"
- "Invented stores"

Arguably, based on that article, store tearing is only observed in the
wild for constants (which is not the case here), and invented stores
seem to require specific code patterns. But I wonder why we would ever want to
pair a fragile non-volatile store with a READ_ONCE() ? Considering the pain
associated to reproduce and hunt down this kind of issue in the wild, I would
be tempted to enforce that any READ_ONCE() operating on a variable would either
need to be paired with WRITE_ONCE() or with atomic operations, so those can
eventually be validated by static code checkers and code sanitizers.

If coding style is your only concern here, we may want to consider
introducing new macros in compiler.h:

WRITE_ONCE_INC(v) /* v++ */
WRITE_ONCE_DEC(v) /* v-- */
WRITE_ONCE_ADD(v, count) /* v += count */
WRITE_ONCE_SUB(v, count) /* v -= count */

Thanks,

Mathieu

> 
> -- Steve
> 
> 
> 
>> [ Compile-tested only. I suspect it might fix the following syzbot
>>   report:
>> 
>>   syzbot+774fddf07b7ab29a1e55@syzkaller.appspotmail.com ]
>> 
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> CC: Joel Fernandes (Google) <joel@joelfernandes.org>
>> CC: Peter Zijlstra <peterz@infradead.org>
>> CC: Steven Rostedt (VMware) <rostedt@goodmis.org>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: Paul E. McKenney <paulmck@linux.ibm.com>
>> ---
>>  kernel/trace/trace_sched_switch.c | 32 ++++++++++++++++++++++----------
>>  1 file changed, 22 insertions(+), 10 deletions(-)
>> 
>> diff --git a/kernel/trace/trace_sched_switch.c
>> b/kernel/trace/trace_sched_switch.c
>> index e288168661e1..902e8bf59aeb 100644
>> --- a/kernel/trace/trace_sched_switch.c
>> +++ b/kernel/trace/trace_sched_switch.c
>> @@ -26,8 +26,8 @@ probe_sched_switch(void *ignore, bool preempt,
>>  {
>>  	int flags;
>>  
>> -	flags = (RECORD_TGID * !!sched_tgid_ref) +
>> -		(RECORD_CMDLINE * !!sched_cmdline_ref);
>> +	flags = (RECORD_TGID * !!READ_ONCE(sched_tgid_ref)) +
>> +		(RECORD_CMDLINE * !!READ_ONCE(sched_cmdline_ref));
>>  
>>  	if (!flags)
>>  		return;
>> @@ -39,8 +39,8 @@ probe_sched_wakeup(void *ignore, struct task_struct *wakee)
>>  {
>>  	int flags;
>>  
>> -	flags = (RECORD_TGID * !!sched_tgid_ref) +
>> -		(RECORD_CMDLINE * !!sched_cmdline_ref);
>> +	flags = (RECORD_TGID * !!READ_ONCE(sched_tgid_ref)) +
>> +		(RECORD_CMDLINE * !!READ_ONCE(sched_cmdline_ref));
>>  
>>  	if (!flags)
>>  		return;
>> @@ -89,21 +89,28 @@ static void tracing_sched_unregister(void)
>>  
>>  static void tracing_start_sched_switch(int ops)
>>  {
>> -	bool sched_register = (!sched_cmdline_ref && !sched_tgid_ref);
>> +	bool sched_register;
>> +
>>  	mutex_lock(&sched_register_mutex);
>> +	sched_register = (!sched_cmdline_ref && !sched_tgid_ref);
>>  
>>  	switch (ops) {
>>  	case RECORD_CMDLINE:
>> -		sched_cmdline_ref++;
>> +		WRITE_ONCE(sched_cmdline_ref, sched_cmdline_ref + 1);
>>  		break;
>>  
>>  	case RECORD_TGID:
>> -		sched_tgid_ref++;
>> +		WRITE_ONCE(sched_tgid_ref, sched_tgid_ref + 1);
>>  		break;
>> +
>> +	default:
>> +		WARN_ONCE(1, "Unsupported tracing op: %d", ops);
>> +		goto end;
>>  	}
>>  
>> -	if (sched_register && (sched_cmdline_ref || sched_tgid_ref))
>> +	if (sched_register)
>>  		tracing_sched_register();
>> +end:
>>  	mutex_unlock(&sched_register_mutex);
>>  }
>>  
>> @@ -113,16 +120,21 @@ static void tracing_stop_sched_switch(int ops)
>>  
>>  	switch (ops) {
>>  	case RECORD_CMDLINE:
>> -		sched_cmdline_ref--;
>> +		WRITE_ONCE(sched_cmdline_ref, sched_cmdline_ref - 1);
>>  		break;
>>  
>>  	case RECORD_TGID:
>> -		sched_tgid_ref--;
>> +		WRITE_ONCE(sched_tgid_ref, sched_tgid_ref - 1);
>>  		break;
>> +
>> +	default:
>> +		WARN_ONCE(1, "Unsupported tracing op: %d", ops);
>> +		goto end;
>>  	}
>>  
>>  	if (!sched_cmdline_ref && !sched_tgid_ref)
>>  		tracing_sched_unregister();
>> +end:
>>  	mutex_unlock(&sched_register_mutex);
>>  }

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

  parent reply	other threads:[~2019-08-16 17:19 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-18 10:29 WARNING in tracepoint_probe_register_prio (3) syzbot
2019-08-16  0:11 ` syzbot
2019-08-16 14:26   ` [PATCH 1/1] Fix: trace sched switch start/stop racy updates Mathieu Desnoyers
2019-08-16 16:25     ` Steven Rostedt
2019-08-16 16:48       ` Valentin Schneider
2019-08-16 17:04         ` Steven Rostedt
2019-08-16 17:41           ` Mathieu Desnoyers
2019-08-16 19:18             ` Steven Rostedt
2019-08-16 19:19             ` Alan Stern
2019-08-16 20:44               ` Joel Fernandes
2019-08-16 20:49                 ` Thomas Gleixner
2019-08-16 20:57                   ` Joel Fernandes
2019-08-16 22:27                     ` Valentin Schneider
2019-08-16 22:57                       ` Linus Torvalds
2019-08-17  1:41                         ` Mathieu Desnoyers
2019-08-17  4:52                         ` Paul E. McKenney
2019-08-17  8:28                           ` Linus Torvalds
2019-08-17  8:44                             ` Linus Torvalds
2019-08-17 15:02                               ` Mathieu Desnoyers
2019-08-17 20:03                                 ` Valentin Schneider
2019-08-17 23:00                                   ` Paul E. McKenney
2019-08-19 10:34                                     ` Valentin Schneider
2019-08-17 22:28                             ` Paul E. McKenney
2019-08-20 14:01                           ` Peter Zijlstra
2019-08-20 20:31                             ` Paul E. McKenney
2019-08-20 20:39                               ` Peter Zijlstra
2019-08-20 20:52                                 ` Paul E. McKenney
2019-08-16 21:04                   ` Linus Torvalds
2019-08-17  1:36                     ` Mathieu Desnoyers
2019-08-17  2:13                       ` Steven Rostedt
2019-08-17 14:40                         ` Mathieu Desnoyers
2019-08-17 15:26                           ` Steven Rostedt
2019-08-17 15:55                             ` Mathieu Desnoyers
2019-08-17 16:40                               ` Steven Rostedt
2019-08-17 22:06                                 ` Paul E. McKenney
2019-08-17  8:08                       ` Linus Torvalds
2019-08-20 13:56                         ` Peter Zijlstra
2019-08-20 20:29                           ` Paul E. McKenney
2019-08-21 10:32                             ` Will Deacon
2019-08-21 13:23                               ` Paul E. McKenney
2019-08-21 13:32                                 ` Will Deacon
2019-08-21 13:56                                   ` Paul E. McKenney
2019-08-21 16:22                                     ` Will Deacon
2019-08-21 15:33                                 ` Peter Zijlstra
2019-08-21 15:48                                   ` Mathieu Desnoyers
2019-08-21 16:14                                     ` Paul E. McKenney
2019-08-21 19:03                                     ` Joel Fernandes
2019-09-09  6:21                           ` Herbert Xu
2019-08-16 20:49                 ` Steven Rostedt
2019-08-16 20:59                   ` Joel Fernandes
2019-08-17  1:25                   ` Mathieu Desnoyers
2019-08-18  9:15                   ` stable markup was " Pavel Machek
2019-08-16 17:19       ` Mathieu Desnoyers [this message]
2019-08-16 19:15         ` Steven Rostedt
2019-08-17 14:27           ` Mathieu Desnoyers
2019-08-17 15:42             ` Steven Rostedt
2019-08-17 15:53               ` Mathieu Desnoyers
2019-08-17 16:43                 ` Steven Rostedt
2019-08-16 12:32 ` WARNING in tracepoint_probe_register_prio (3) syzbot
2019-08-16 12:41   ` Sebastian Andrzej Siewior

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=623129606.21592.1565975960497.JavaMail.zimbra@efficios.com \
    --to=mathieu.desnoyers@efficios.com \
    --cc=boqun.feng@gmail.com \
    --cc=dhowells@redhat.com \
    --cc=joel@joelfernandes.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paulmck@linux.ibm.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=stern@rowland.harvard.edu \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=will.deacon@arm.com \
    --subject='Re: [PATCH 1/1] Fix: trace sched switch start/stop racy updates' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.