linux-kernel - Re: wake_up_process implied memory barrier clarification

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: <20150828145121.GG5301@dhcp22.suse.cz>
Date:	Fri, 28 Aug 2015 16:51:21 +0200
From:	Michal Hocko <mhocko@...nel.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	David Howells <dhowells@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: wake_up_process implied memory barrier clarification

On Thu 27-08-15 20:26:54, Oleg Nesterov wrote:
> On 08/27, Michal Hocko wrote:
> >
> > --- a/Documentation/memory-barriers.txt
> > +++ b/Documentation/memory-barriers.txt
> > @@ -2031,6 +2031,9 @@ something up.  The barrier occurs before the task state is cleared, and so sits
> >  	    <general barrier>		  STORE current->state
> >  	LOAD event_indicated
> >
> > +Please note that wake_up_process is an exception here because it implies
> > +the write memory barrier unconditionally.
> > +
> 
> I simply can't understand (can't even parse) this part of memory-barriers.txt.

Do you mean the added text or the example above it?

> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1967,8 +1967,7 @@ static void try_to_wake_up_local(struct task_struct *p)
> >   *
> >   * Return: 1 if the process was woken up, 0 if it was already running.
> >   *
> > - * It may be assumed that this function implies a write memory barrier before
> > - * changing the task state if and only if any tasks are woken up.
> > + * It may be assumed that this function implies a write memory barrier.
> >   */
> 
> I won't argue, technically this is correct of course. And I agree that
> the old comment is misleading.

Well the reason I've noticed is the following race in the scsi code
    CPU0                                        CPU1
scsi_error_handler                      scsi_host_dev_release
                                          kthread_stop()
  while (!kthread_should_stop()) {
                                            set_bit(KTHREAD_SHOULD_STOP)
                                            wake_up_process()
                                            wait_for_completion()

    set_current_state(TASK_INTERRUPTIBLE)
    schedule()
    [...]
  }

I have read the comment for wake_up_process and was wondering that
moving set_current_state before kthread_should_stop wouldn't be enough
because the the task at CPU0 might be TASK_RUNNIG and so wake_up_process
wouldn't wake up it and the missing write barrier could lead to a missed
KTHREAD_SHOULD_STOP. A look into ttwu made my worry void.

> But the new comment looks as if it is fine to avoid wmb() if you do
> wake_up_process(). Say,
> 
> 	void w(void)
> 	{
> 		A = 1;
> 		wake_up_process(something_unrelated);
> 		// we know that it implies wmb().
> 		B = 1;
> 	}
> 
> 	void r(void)
> 	{
> 		int a, b;
> 
> 		b = B;
> 		rmb();
> 		a = A;
> 
> 		BUG_ON(b && !a);
> 	}
> 
> Perhaps this part of the comment should be simply removed, the unconditional
> wmb() in ttwu() is just implementation detail. And note that initially the
> documented behaviour of smp_mb__before_spinlock() was only the STORE - LOAD
> serialization. Then people noticed that it actually does wmb() and started
> to rely on this fact.
> 
> To me, this comment should just explain that this function implies a barrier
> but only in a sense that you do not need another one after CONDITION = T and
> before wake_up_process().

I have no objection against more precise wording here but what we have is just
misleading.
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/