linux-kernel - Re: [Question] smp_wmb() in prepare

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] [day] [month] [year] [list]

Date:   Thu, 22 Nov 2018 16:45:27 +0100
From:   Andrea Parri <andrea.parri@...rulasolutions.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [Question] smp_wmb() in prepare_uprobe()

On Thu, Nov 22, 2018 at 04:05:24PM +0100, Oleg Nesterov wrote:
> On 11/22, Andrea Parri wrote:
> >
> > > See 142b18ddc81439acda4bc4231b291e99fe67d507 ("uprobes: Fix handle_swbp()
> > > vs unregister() + register() race") and the comment above this rmb().
> >
> > Mmh..., at first glance, this suggests me that the above set_bit() and
> > test_bit() to/from uprobe->flags are among these memory accesses.  But
> > this doesn't make sense to me: these accesses do not "alternate" (i.e.,
> > they both appear after the corresponding barrier..); instead I'd expect
> > something like (on top of the above diff):
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 2d29977522017..a75b9a08dee54 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2178,10 +2178,18 @@ static void handle_swbp(struct pt_regs *regs)
> >  	 * After we hit the bp, _unregister + _register can install the
> >  	 * new and not-yet-analyzed uprobe at the same address, restart.
> >  	 */
> > -	smp_rmb(); /* pairs with the smp_wmb() in prepare_uprobe() */
> >  	if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
> >  		goto out;
> >
> > +	/*
> > +	 * Pairs with the smp_wmb() in prepare_uprobe().
> > +	 *
> > +	 * Guarantees that if we see the UPROBE_COPY_INSN bit set, then
> > +	 * we must (can) also see the stores to &uprobe->arch performed
> > +	 * by prepare_uprobe() (say).
> > +	 */
> > +	smp_rmb();
> 
> OOPS, you are right! Thanks.

Thank you for the clarification; I'll send a patch with the fix shortly.

  Andrea


> 
> Oleg.
>