Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 22 Nov 2018 14:44:00 +0100
From:   Andrea Parri <andrea.parri@amarulasolutions.com>
To:     Oleg Nesterov <oleg@redhat.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [Question] smp_wmb() in prepare_uprobe()
Message-ID: <20181122134400.GA10327@andrea>
References: <20181121224124.GB4016@andrea>
 <20181122123655.GD28270@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181122123655.GD28270@redhat.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 22, 2018 at 01:36:56PM +0100, Oleg Nesterov wrote:
> Hi,
> 
> On 11/21, Andrea Parri wrote:
> >
> > The comment for the smp_wmb() in prepare_uprobe() says:
> >
> >   "pairs with rmb() in find_active_uprobe()"
> 
> it seems that this comment was wrong from the very beginning,
> 
> 
> > but I see no (smp_)rmb() in find_active_uprobe(); I see the smp_rmb() in
> > handle_swbp(): is this the intended pairing barrier?
> 
> Yes, and the comment near this rmb() says "pairs with wmb() in install_breakpoint()",
> today this is not right too.

Thanks for the confirmation.  So, this is the easy part ;-), maybe
something like:

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 96d4bee83489b..2d29977522017 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -829,7 +829,7 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
 	BUG_ON((uprobe->offset & ~PAGE_MASK) +
 			UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
 
-	smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+	smp_wmb(); /* pairs with the smp_rmb() in handle_swbp() */
 	set_bit(UPROBE_COPY_INSN, &uprobe->flags);
 
  out:
@@ -2178,7 +2178,7 @@ 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 wmb() in install_breakpoint() */
+	smp_rmb(); /* pairs with the smp_wmb() in prepare_uprobe() */
 	if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
 		goto out;
 

> 
> > Which memory accesses do you want to "order" with this pairing?
> 
> 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();
+
 	/* Tracing handlers use ->utask to communicate with fetch methods */
 	if (!get_utask())
 		goto out;

Thoughts?

  Andrea


> 
> Oleg.
>