Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1731840 > unrolled thread

[PATCH v2 0/3] A few uprobe fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] A few uprobe fixes "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-09-13 22:30 +0200
    [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-09-13 22:40 +0200
      Re: [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe Oleg Nesterov <oleg@redhat.com> - 2017-09-15 17:40 +0200
        Re: [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-09-16 13:40 +0200

#1731840 — [PATCH v2 0/3] A few uprobe fixes

The first patch adds a warning if we are unable to install a uprobe 
breakpoint and is unchanged from the previous posting at:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg123290.html

The second patch just rate limits the uprobe warnings and is new in this 
series.

The third patch fixes how we check for an active uprobe -- we need to 
consider if we were actually successful in installing the breakpoint.  
This patch is new in this series as well.


- Naveen

Naveen N. Rao (3):
  kernel/uprobes: Warn if unable to install breakpoint
  kernel/uprobes: Ratelimit messages
  kernel/uprobes: Fix check for active uprobe

 kernel/events/uprobes.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

-- 
2.14.1

[toc] | [next] | [standalone]

#1731842 — [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe

If we try to install a uprobe on a breakpoint instruction, we register the
probe, but refuse to install it. In this case, when the breakpoint hits, we
incorrectly assume that the probe hit and end up looping.

Fix this by checking that the trap was actually installed in
find_active_uprobe().

Reported-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 kernel/events/uprobes.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e14eb0a6e4f3..599078e6a092 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1752,6 +1752,13 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 			uprobe = find_uprobe(inode, offset);
 		}
 
+		/* Ensure that the breakpoint was actually installed */
+		if (uprobe) {
+			smp_rmb(); /* pairs with wmb() in prepare_uprobe() */
+			if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
+				uprobe = NULL;
+		}
+
 		if (!uprobe)
 			*is_swbp = is_trap_at_addr(mm, bp_vaddr);
 	} else {
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1732930 — Re: [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe

Hi Naveen,

I forgot almost everything about this code, but at first glance this patch
needs more comments and the changelog should be updated, at least. And in
any case this needs more changes iirc.

On 09/14, Naveen N. Rao wrote:
>
> If we try to install a uprobe on a breakpoint instruction, we register the
> probe, but refuse to install it. In this case, when the breakpoint hits, we
> incorrectly assume that the probe hit and end up looping.

This looks confusing to me...

If we try to install a uprobe on a breakpoint instruction, uprobe_register()
should fail and remove uprobe. The task which hits this uprobe will loop
until this uprobe goes away, this is fine.

But there is another case and probably this is what you mean. uprobe_register()
can do nothing except insert_uprobe() if nobody mmaps this binary, and after that
uprobe_mmap()->prepare_uprobe() can fail if the original isns is int3. In this
case this !UPROBE_COPY_INSN uprobe won't go away, and the task can loop until
it is killed or uprobe_unregister().

Right?


Now. The real fix should kill UPROBE_COPY_INSN altogether and simply move
prepare_uprobe() from install_breakpoint() into __uprobe_register(), before
it does register_for_each_vma().

The only problem is that read_mapping_page() needs "struct file *" and nobody
confirmed that read_mapping_page(data => NULL) is fine on all filesystems.
I think it should be fine though, and perhaps we should finally do this change.


until then we can probably make the things a bit better, but

> Fix this by checking that the trap was actually installed in
> find_active_uprobe().
> 
> Reported-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  kernel/events/uprobes.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index e14eb0a6e4f3..599078e6a092 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1752,6 +1752,13 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  			uprobe = find_uprobe(inode, offset);
>  		}
>  
> +		/* Ensure that the breakpoint was actually installed */
> +		if (uprobe) {
> +			smp_rmb(); /* pairs with wmb() in prepare_uprobe() */
> +			if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
> +				uprobe = NULL;
> +		}

I need to recall how this code works... but if we add this change, shouldn't
we remove another similar UPROBE_COPY_INSN check in handle_swbp() and add more
comments?

Oleg.

[toc] | [prev] | [next] | [standalone]

#1733259 — Re: [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe

On 2017/09/15 05:38PM, Oleg Nesterov wrote:
> Hi Naveen,

Hi Oleg,

> 
> I forgot almost everything about this code, but at first glance this patch
> needs more comments and the changelog should be updated, at least. And in
> any case this needs more changes iirc.

Sure, thanks for the review!

> 
> On 09/14, Naveen N. Rao wrote:
> >
> > If we try to install a uprobe on a breakpoint instruction, we register the
> > probe, but refuse to install it. In this case, when the breakpoint hits, we
> > incorrectly assume that the probe hit and end up looping.
> 
> This looks confusing to me...
> 
> If we try to install a uprobe on a breakpoint instruction, uprobe_register()
> should fail and remove uprobe. The task which hits this uprobe will loop
> until this uprobe goes away, this is fine.
> 
> But there is another case and probably this is what you mean. uprobe_register()
> can do nothing except insert_uprobe() if nobody mmaps this binary, and after that
> uprobe_mmap()->prepare_uprobe() can fail if the original isns is int3. In this
> case this !UPROBE_COPY_INSN uprobe won't go away, and the task can loop until
> it is killed or uprobe_unregister().
> 
> Right?

You're right -- I should have elaborated.

> 
> 
> Now. The real fix should kill UPROBE_COPY_INSN altogether and simply move
> prepare_uprobe() from install_breakpoint() into __uprobe_register(), before
> it does register_for_each_vma().
> 
> The only problem is that read_mapping_page() needs "struct file *" and nobody
> confirmed that read_mapping_page(data => NULL) is fine on all filesystems.
> I think it should be fine though, and perhaps we should finally do this change.

Sure. I will take a stab at this after these fixes.

> 
> 
> until then we can probably make the things a bit better, but
> 
> > Fix this by checking that the trap was actually installed in
> > find_active_uprobe().
> > 
> > Reported-by: Anton Blanchard <anton@samba.org>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  kernel/events/uprobes.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index e14eb0a6e4f3..599078e6a092 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -1752,6 +1752,13 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> >  			uprobe = find_uprobe(inode, offset);
> >  		}
> >  
> > +		/* Ensure that the breakpoint was actually installed */
> > +		if (uprobe) {
> > +			smp_rmb(); /* pairs with wmb() in prepare_uprobe() */
> > +			if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
> > +				uprobe = NULL;
> > +		}
> 
> I need to recall how this code works... but if we add this change, shouldn't
> we remove another similar UPROBE_COPY_INSN check in handle_swbp() and add more
> comments?

Yes, you're right. The check in handle_swbp() won't be needed anymore. I 
will make that change and re-spin.

Thanks,
Naveen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web