* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.