Hi,
The comment for the smp_wmb() in prepare_uprobe() says:
"pairs with rmb() in find_active_uprobe()"
but I see no (smp_)rmb() in find_active_uprobe(); I see the smp_rmb() in
handle_swbp(): is this the intended pairing barrier?
Which memory accesses do you want to "order" with this pairing?
Thanks,
Andrea
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.
> 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().
Oleg.
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.
>
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.
Oleg.
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.
>