linux-kernel - Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command

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: <20170925121054.fqtqkzmwswgjyt75@tardis>
Date:   Mon, 25 Sep 2017 20:10:54 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Andrew Hunter <ahh@...gle.com>,
        maged michael <maged.michael@...il.com>,
        gromer <gromer@...gle.com>, Avi Kivity <avi@...lladb.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Dave Watson <davejwatson@...com>,
        Alan Stern <stern@...land.harvard.edu>,
        Will Deacon <will.deacon@....com>,
        Andy Lutomirski <luto@...nel.org>,
        linux-arch <linux-arch@...r.kernel.org>
Subject: Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited
 private command

On Sun, Sep 24, 2017 at 02:23:04PM +0000, Mathieu Desnoyers wrote:
[...]
> >> 
> >> copy_mm() is performed without holding current->sighand->siglock, so
> >> it appears to be racing with concurrent membarrier register cmd.
> > 
> > Speak of racing, I think we currently have a problem if we do a
> > register_private_expedited in one thread and do a
> > membarrer_private_expedited in another thread(sharing the same mm), as
> > follow:
> > 
> >	{t1,t2,t3 sharing the same ->mm}
> >	CPU 0				CPU 1				CPU2
> >	====================		===================		============
> >	{in thread t1}
> >	membarrier_register_private_expedited():
> >	  ...
> >	  WRITE_ONCE(->mm->membarrier_private_expedited, 1);
> >	  membarrier_arch_register_private_expedited():
> >	    ...
> >	    <haven't set the TIF for t3 yet>
> > 
> >	    				{in thread t2}
> >					membarrier_private_expedited():
> >					  READ_ONCE(->mm->membarrier_private_expedited); // == 1
> >					  ...
> >					  for_each_online_cpu()
> >					    ...
> >					    <p is cpu_rq(CPU2)->curr>
> >					    if (p && p->mm == current->mm) // false
> >					    <so no ipi sent to CPU2>
> >					    				
> >									{about to switch to t3}
> >									rq->curr = t3;
> >									....
> >									context_switch():
> >									  ...
> >									  finish_task_swtich():
> >									    membarrier_sched_in():
> >									    <TIF is not set>
> >									    // no smp_mb() here.
> > 
> > , and we will miss the smp_mb() on CPU2, right? And this could even
> > happen if t2 has a membarrier_register_private_expedited() preceding the
> > membarrier_private_expedited().
> >					
> > Am I missing something subtle here?
> 
> I think the problem sits in this function:
> 
> static void membarrier_register_private_expedited(void)
> {
>         struct task_struct *p = current;
> 
>         if (READ_ONCE(p->mm->membarrier_private_expedited))
>                 return;
>         WRITE_ONCE(p->mm->membarrier_private_expedited, 1);
>         membarrier_arch_register_private_expedited(p);
> }
> 
> I need to change the order between WRITE_ONCE() and invoking
> membarrier_arch_register_private_expedited. If I issue the
> WRITE_ONCE after the arch code (which sets the TIF flags),
> then concurrent membarrier priv exped commands will simply
> return an -EPERM error:
> 
> static void membarrier_register_private_expedited(void)
> {
>         struct task_struct *p = current;
> 
>         if (READ_ONCE(p->mm->membarrier_private_expedited))
>                 return;
>         membarrier_arch_register_private_expedited(p);
>         WRITE_ONCE(p->mm->membarrier_private_expedited, 1);
> }
> 
> Do you agree that this would fix the race you identified ?
> 

Yep, that will do the trick ;-)

Regards,
Boqun

> Thanks,
> 
> Mathieu
> 

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)