Re: 5.8-rc*: kernel BUG at kernel/signal.c:1917

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Jul 20, 2020 at 05:35:15PM +0200, Oleg Nesterov wrote:
> On 07/20, Oleg Nesterov wrote:
> >
> > On 07/20, Peter Zijlstra wrote:
> > >
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -4193,9 +4193,6 @@ static void __sched notrace __schedule(bool preempt)
> > >  	local_irq_disable();
> > >  	rcu_note_context_switch(preempt);
> > >
> > > -	/* See deactivate_task() below. */
> > > -	prev_state = prev->state;
> > > -
> > >  	/*
> > >  	 * Make sure that signal_pending_state()->signal_pending() below
> > >  	 * can't be reordered with __set_current_state(TASK_INTERRUPTIBLE)
> > > @@ -4223,7 +4220,8 @@ static void __sched notrace __schedule(bool preempt)
> > >  	 * We must re-load prev->state in case ttwu_remote() changed it
> > >  	 * before we acquired rq->lock.
> > >  	 */
> > > -	if (!preempt && prev_state && prev_state == prev->state) {
> > > +	prev_state = prev->state;
> > > +	if (!preempt && prev_state) {
> >
> > Heh ;) Peter, you know what? I did the same change and tried to understand
> > why it is wrong and what have I missed.
> >
> > Thanks, now I can relax. But my head hurts too, I'll probably try to re-read
> > this code and other emails from you tomorrow.
> 
> Yes, I can no longer read this code today ;)
> 
> but now it seems to me that (in theory) we need READ_ONCE(prev->state) here
> and probably WRITE_ONCE(on_rq) in deactivate_task() to ensure ctrl-dep?
> 
> Probably not, I got lost.
> Probably not, I got lost.
> Probably not, I got lost.

So, task_struct::state is declared volatile (we should probably 'fix'
that some day), so that doesn't require READ_ONCE() -- in fact, that
caused a bunch of re-reads in the old code which made the loadavg race
more likely.

->on_rq is only ever written 0,1,2, there's no possibe store-tearing.
But possibly, yes, WRITE_ONCE() would be nicer.