Messages in this thread Patches in this message | | | Date | Thu, 22 Nov 2018 14:44:00 +0100 | From | Andrea Parri <> | Subject | Re: [Question] smp_wmb() in prepare_uprobe() |
| |
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. >
| |