* [PATCH] uprobes: Fix handle_swbp() vs. unregister() + register() race once more
@ 2018-11-22 16:10 Andrea Parri
2018-11-22 16:19 ` Oleg Nesterov
2018-11-23 7:33 ` [tip:perf/urgent] " tip-bot for Andrea Parri
0 siblings, 2 replies; 6+ messages in thread
From: Andrea Parri @ 2018-11-22 16:10 UTC (permalink / raw)
To: linux-kernel
Cc: Andrea Parri, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Oleg Nesterov, stable
Commit 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() +
register() race") added the UPROBE_COPY_INSN flag, and corresponding
smp_wmb() and smp_rmb() memory barriers, to ensure that handle_swbp()
uses fully-initialized uprobes only.
However, the smp_rmb() is mis-placed: this barrier should be placed
after handle_swbp() has tested for the flag, thus guaranteeing that
(program-order) subsequent loads from the uprobe can see the initial
stores performed by prepare_uprobe().
Move the smp_rmb() accordingly. Also amend the comments associated
to the two memory barriers to indicate their actual locations.
Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: stable@kernel.org
Fixes: 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() + register() race")
---
kernel/events/uprobes.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 96d4bee83489b..322e97bbb4370 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,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 wmb() in install_breakpoint() */
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 also see the stores to &uprobe->arch performed by the
+ * prepare_uprobe() call.
+ */
+ smp_rmb();
+
/* Tracing handlers use ->utask to communicate with fetch methods */
if (!get_utask())
goto out;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] uprobes: Fix handle_swbp() vs. unregister() + register() race once more
2018-11-22 16:10 [PATCH] uprobes: Fix handle_swbp() vs. unregister() + register() race once more Andrea Parri
@ 2018-11-22 16:19 ` Oleg Nesterov
2018-11-22 16:27 ` Oleg Nesterov
2018-11-23 7:33 ` [tip:perf/urgent] " tip-bot for Andrea Parri
1 sibling, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2018-11-22 16:19 UTC (permalink / raw)
To: Andrea Parri
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, stable
On 11/22, Andrea Parri wrote:
>
> Commit 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() +
> register() race") added the UPROBE_COPY_INSN flag, and corresponding
> smp_wmb() and smp_rmb() memory barriers, to ensure that handle_swbp()
> uses fully-initialized uprobes only.
>
> However, the smp_rmb() is mis-placed: this barrier should be placed
> after handle_swbp() has tested for the flag, thus guaranteeing that
> (program-order) subsequent loads from the uprobe can see the initial
> stores performed by prepare_uprobe().
>
> Move the smp_rmb() accordingly. Also amend the comments associated
> to the two memory barriers to indicate their actual locations.
>
> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: stable@kernel.org
> Fixes: 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() + register() race")
Thanks,
Acked-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/events/uprobes.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 96d4bee83489b..322e97bbb4370 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,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 wmb() in install_breakpoint() */
> 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 also see the stores to &uprobe->arch performed by the
> + * prepare_uprobe() call.
> + */
> + smp_rmb();
> +
> /* Tracing handlers use ->utask to communicate with fetch methods */
> if (!get_utask())
> goto out;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] uprobes: Fix handle_swbp() vs. unregister() + register() race once more
2018-11-22 16:19 ` Oleg Nesterov
@ 2018-11-22 16:27 ` Oleg Nesterov
2018-11-22 19:38 ` Andrea Parri
2018-11-23 7:34 ` Ingo Molnar
0 siblings, 2 replies; 6+ messages in thread
From: Oleg Nesterov @ 2018-11-22 16:27 UTC (permalink / raw)
To: Andrea Parri
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, stable
On 11/22, Oleg Nesterov wrote:
> On 11/22, Andrea Parri wrote:
> >
> > Commit 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() +
> > register() race") added the UPROBE_COPY_INSN flag, and corresponding
> > smp_wmb() and smp_rmb() memory barriers, to ensure that handle_swbp()
> > uses fully-initialized uprobes only.
> >
> > However, the smp_rmb() is mis-placed: this barrier should be placed
> > after handle_swbp() has tested for the flag, thus guaranteeing that
> > (program-order) subsequent loads from the uprobe can see the initial
> > stores performed by prepare_uprobe().
> >
> > Move the smp_rmb() accordingly. Also amend the comments associated
> > to the two memory barriers to indicate their actual locations.
> >
> > Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: stable@kernel.org
> > Fixes: 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() + register() race")
>
> Thanks,
>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
Yes, but I am not sure this is the -stable material...
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] uprobes: Fix handle_swbp() vs. unregister() + register() race once more
2018-11-22 16:27 ` Oleg Nesterov
@ 2018-11-22 19:38 ` Andrea Parri
2018-11-23 7:34 ` Ingo Molnar
1 sibling, 0 replies; 6+ messages in thread
From: Andrea Parri @ 2018-11-22 19:38 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, stable
On Thu, Nov 22, 2018 at 05:27:09PM +0100, Oleg Nesterov wrote:
> On 11/22, Oleg Nesterov wrote:
> > On 11/22, Andrea Parri wrote:
> > >
> > > Commit 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() +
> > > register() race") added the UPROBE_COPY_INSN flag, and corresponding
> > > smp_wmb() and smp_rmb() memory barriers, to ensure that handle_swbp()
> > > uses fully-initialized uprobes only.
> > >
> > > However, the smp_rmb() is mis-placed: this barrier should be placed
> > > after handle_swbp() has tested for the flag, thus guaranteeing that
> > > (program-order) subsequent loads from the uprobe can see the initial
> > > stores performed by prepare_uprobe().
> > >
> > > Move the smp_rmb() accordingly. Also amend the comments associated
> > > to the two memory barriers to indicate their actual locations.
> > >
> > > Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Ingo Molnar <mingo@redhat.com>
> > > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > > Cc: Jiri Olsa <jolsa@redhat.com>
> > > Cc: Namhyung Kim <namhyung@kernel.org>
> > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > Cc: stable@kernel.org
> > > Fixes: 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() + register() race")
> >
> > Thanks,
> >
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
Thanks, Oleg.
>
> Yes, but I am not sure this is the -stable material...
I'm inclined to leave it to you/the maintainers in question to decide
about this, ;-) but please let me know if a resend or another action
is needed from me.
Andrea
>
> Oleg.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:perf/urgent] uprobes: Fix handle_swbp() vs. unregister() + register() race once more
2018-11-22 16:10 [PATCH] uprobes: Fix handle_swbp() vs. unregister() + register() race once more Andrea Parri
2018-11-22 16:19 ` Oleg Nesterov
@ 2018-11-23 7:33 ` tip-bot for Andrea Parri
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Andrea Parri @ 2018-11-23 7:33 UTC (permalink / raw)
To: linux-tip-commits
Cc: eranian, tglx, mingo, akpm, hpa, torvalds, namhyung, jolsa,
andrea.parri, paulmck, acme, oleg, vincent.weaver,
alexander.shishkin, peterz, linux-kernel
Commit-ID: 09d3f015d1e1b4fee7e9bbdcf54201d239393391
Gitweb: https://git.kernel.org/tip/09d3f015d1e1b4fee7e9bbdcf54201d239393391
Author: Andrea Parri <andrea.parri@amarulasolutions.com>
AuthorDate: Thu, 22 Nov 2018 17:10:31 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 23 Nov 2018 08:31:19 +0100
uprobes: Fix handle_swbp() vs. unregister() + register() race once more
Commit:
142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() + register() race")
added the UPROBE_COPY_INSN flag, and corresponding smp_wmb() and smp_rmb()
memory barriers, to ensure that handle_swbp() uses fully-initialized
uprobes only.
However, the smp_rmb() is mis-placed: this barrier should be placed
after handle_swbp() has tested for the flag, thus guaranteeing that
(program-order) subsequent loads from the uprobe can see the initial
stores performed by prepare_uprobe().
Move the smp_rmb() accordingly. Also amend the comments associated
to the two memory barriers to indicate their actual locations.
Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: stable@kernel.org
Fixes: 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() + register() race")
Link: http://lkml.kernel.org/r/20181122161031.15179-1-andrea.parri@amarulasolutions.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/uprobes.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 96d4bee83489..322e97bbb437 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,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 wmb() in install_breakpoint() */
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 also see the stores to &uprobe->arch performed by the
+ * prepare_uprobe() call.
+ */
+ smp_rmb();
+
/* Tracing handlers use ->utask to communicate with fetch methods */
if (!get_utask())
goto out;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] uprobes: Fix handle_swbp() vs. unregister() + register() race once more
2018-11-22 16:27 ` Oleg Nesterov
2018-11-22 19:38 ` Andrea Parri
@ 2018-11-23 7:34 ` Ingo Molnar
1 sibling, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2018-11-23 7:34 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrea Parri, linux-kernel, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, stable
* Oleg Nesterov <oleg@redhat.com> wrote:
> On 11/22, Oleg Nesterov wrote:
> > On 11/22, Andrea Parri wrote:
> > >
> > > Commit 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() +
> > > register() race") added the UPROBE_COPY_INSN flag, and corresponding
> > > smp_wmb() and smp_rmb() memory barriers, to ensure that handle_swbp()
> > > uses fully-initialized uprobes only.
> > >
> > > However, the smp_rmb() is mis-placed: this barrier should be placed
> > > after handle_swbp() has tested for the flag, thus guaranteeing that
> > > (program-order) subsequent loads from the uprobe can see the initial
> > > stores performed by prepare_uprobe().
> > >
> > > Move the smp_rmb() accordingly. Also amend the comments associated
> > > to the two memory barriers to indicate their actual locations.
> > >
> > > Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Ingo Molnar <mingo@redhat.com>
> > > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > > Cc: Jiri Olsa <jolsa@redhat.com>
> > > Cc: Namhyung Kim <namhyung@kernel.org>
> > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > Cc: stable@kernel.org
> > > Fixes: 142b18ddc8143 ("uprobes: Fix handle_swbp() vs unregister() + register() race")
> >
> > Thanks,
> >
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
>
> Yes, but I am not sure this is the -stable material...
So I left the Cc: stable tag intact, because this is a really low-risk
fix (it just moves barriers around), and clearly fixes a bug that people
might or might not have observed.
Even if they observed it the race is probably very hard to reproduce and
almost impossible to report - so we are better off propagating this fix
to -stable, as there's no realistic actionable way for users to actually
complain about the bug if it affects them.
That's the general backporting policy for race fixes, unless they are
really, really intrusive - which this one isn't really.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-23 7:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-22 16:10 [PATCH] uprobes: Fix handle_swbp() vs. unregister() + register() race once more Andrea Parri
2018-11-22 16:19 ` Oleg Nesterov
2018-11-22 16:27 ` Oleg Nesterov
2018-11-22 19:38 ` Andrea Parri
2018-11-23 7:34 ` Ingo Molnar
2018-11-23 7:33 ` [tip:perf/urgent] " tip-bot for Andrea Parri
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).