* [Question] smp_wmb() in prepare_uprobe() @ 2018-11-21 22:41 Andrea Parri 2018-11-22 12:36 ` Oleg Nesterov 0 siblings, 1 reply; 5+ messages in thread From: Andrea Parri @ 2018-11-21 22:41 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Oleg Nesterov Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Question] smp_wmb() in prepare_uprobe() 2018-11-21 22:41 [Question] smp_wmb() in prepare_uprobe() Andrea Parri @ 2018-11-22 12:36 ` Oleg Nesterov 2018-11-22 13:44 ` Andrea Parri 0 siblings, 1 reply; 5+ messages in thread From: Oleg Nesterov @ 2018-11-22 12:36 UTC (permalink / raw) To: Andrea Parri Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Question] smp_wmb() in prepare_uprobe() 2018-11-22 12:36 ` Oleg Nesterov @ 2018-11-22 13:44 ` Andrea Parri 2018-11-22 15:05 ` Oleg Nesterov 0 siblings, 1 reply; 5+ messages in thread From: Andrea Parri @ 2018-11-22 13:44 UTC (permalink / raw) To: Oleg Nesterov Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-kernel 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. > ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Question] smp_wmb() in prepare_uprobe() 2018-11-22 13:44 ` Andrea Parri @ 2018-11-22 15:05 ` Oleg Nesterov 2018-11-22 15:45 ` Andrea Parri 0 siblings, 1 reply; 5+ messages in thread From: Oleg Nesterov @ 2018-11-22 15:05 UTC (permalink / raw) To: Andrea Parri Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Question] smp_wmb() in prepare_uprobe() 2018-11-22 15:05 ` Oleg Nesterov @ 2018-11-22 15:45 ` Andrea Parri 0 siblings, 0 replies; 5+ messages in thread From: Andrea Parri @ 2018-11-22 15:45 UTC (permalink / raw) To: Oleg Nesterov Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-kernel 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. > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-22 15:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-11-21 22:41 [Question] smp_wmb() in prepare_uprobe() Andrea Parri 2018-11-22 12:36 ` Oleg Nesterov 2018-11-22 13:44 ` Andrea Parri 2018-11-22 15:05 ` Oleg Nesterov 2018-11-22 15:45 ` Andrea Parri
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox