[PATCH v2 0/3] A few uprobe fixes

[PATCH v2 0/3] A few uprobe fixes

[Naveen N. Rao]

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

[PATCH v2 1/3] kernel/uprobes: Warn if unable to install breakpoint

[Naveen N. Rao]

When we try to install a uprobe breakpoint in uprobe_mmap(), we ignore
all errors encountered in the process per this comment at the top of
the function:
    /*
     * Called from mmap_region/vma_adjust with mm->mmap_sem acquired.
     *
     * Currently we ignore all errors and always return 0, the callers
     * can't handle the failure anyway.
     */

However, this is very confusing for users since no probe hits are
recorded nor is an error logged in dmesg.

Fix this by logging an error in dmesg so that users can discover that
there was an issue with the uprobe. To facilitate use of uprobe_warn(),
we move that function to the top of the file.

With this patch, we see a message similar to this in dmesg:
    [  201.449213] uprobe: uprobe_t:9740 failed to setup probe at 0x95c (-524)

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 267f6ef91d97..4af1acff9cc3 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -112,6 +112,12 @@ struct xol_area {
 	unsigned long 			vaddr;		/* Page(s) of instruction slots */
 };
 
+static void uprobe_warn(struct task_struct *t, const char *msg)
+{
+	pr_warn("uprobe: %s:%d failed to %s\n",
+			current->comm, current->pid, msg);
+}
+
 /*
  * valid_vma: Verify if the specified vma is an executable vma
  * Relax restrictions while unregistering: vm_flags might have
@@ -1087,7 +1093,14 @@ int uprobe_mmap(struct vm_area_struct *vma)
 		if (!fatal_signal_pending(current) &&
 		    filter_chain(uprobe, UPROBE_FILTER_MMAP, vma->vm_mm)) {
 			unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
-			install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
+			int ret = install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
+			if (ret) {
+				char msg[64];
+				snprintf(msg, sizeof(msg),
+						"setup probe at 0x%llx (%d)",
+						uprobe->offset, ret);
+				uprobe_warn(current, (const char *)msg);
+			}
 		}
 		put_uprobe(uprobe);
 	}
@@ -1468,12 +1481,6 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
 	return 0;
 }
 
-static void uprobe_warn(struct task_struct *t, const char *msg)
-{
-	pr_warn("uprobe: %s:%d failed to %s\n",
-			current->comm, current->pid, msg);
-}
-
 static void dup_xol_work(struct callback_head *work)
 {
 	if (current->flags & PF_EXITING)
-- 
2.14.1

Re: [PATCH v2 1/3] kernel/uprobes: Warn if unable to install breakpoint

[Oleg Nesterov]

On 09/14, Naveen N. Rao wrote:
>
> +static void uprobe_warn(struct task_struct *t, const char *msg)
> +{
> +	pr_warn("uprobe: %s:%d failed to %s\n",
> +			current->comm, current->pid, msg);
> +}
> +
>  /*
>   * valid_vma: Verify if the specified vma is an executable vma
>   * Relax restrictions while unregistering: vm_flags might have
> @@ -1087,7 +1093,14 @@ int uprobe_mmap(struct vm_area_struct *vma)
>  		if (!fatal_signal_pending(current) &&
>  		    filter_chain(uprobe, UPROBE_FILTER_MMAP, vma->vm_mm)) {
>  			unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
> -			install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
> +			int ret = install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
> +			if (ret) {
> +				char msg[64];
> +				snprintf(msg, sizeof(msg),
> +						"setup probe at 0x%llx (%d)",
> +						uprobe->offset, ret);
> +				uprobe_warn(current, (const char *)msg);

Agreed, but... this is cosmetic, but I don't really like this snprintf().

I won't insist too much, but wouldn't it better to turn uprobe_warn() into
uprobe_warn(struct task_struct *t, char *fmt, ...) ?

Oleg.

Re: [PATCH v2 1/3] kernel/uprobes: Warn if unable to install breakpoint

[Naveen N. Rao]

On 2017/09/15 05:53PM, Oleg Nesterov wrote:
> On 09/14, Naveen N. Rao wrote:
> >
> > +static void uprobe_warn(struct task_struct *t, const char *msg)
> > +{
> > +	pr_warn("uprobe: %s:%d failed to %s\n",
> > +			current->comm, current->pid, msg);
> > +}
> > +
> >  /*
> >   * valid_vma: Verify if the specified vma is an executable vma
> >   * Relax restrictions while unregistering: vm_flags might have
> > @@ -1087,7 +1093,14 @@ int uprobe_mmap(struct vm_area_struct *vma)
> >  		if (!fatal_signal_pending(current) &&
> >  		    filter_chain(uprobe, UPROBE_FILTER_MMAP, vma->vm_mm)) {
> >  			unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
> > -			install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
> > +			int ret = install_breakpoint(uprobe, vma->vm_mm, vma, vaddr);
> > +			if (ret) {
> > +				char msg[64];
> > +				snprintf(msg, sizeof(msg),
> > +						"setup probe at 0x%llx (%d)",
> > +						uprobe->offset, ret);
> > +				uprobe_warn(current, (const char *)msg);
> 
> Agreed, but... this is cosmetic, but I don't really like this snprintf().
> 
> I won't insist too much, but wouldn't it better to turn uprobe_warn() into
> uprobe_warn(struct task_struct *t, char *fmt, ...) ?

Good point. In fact, I think we can just use pr_fmt().

Thanks,
Naveen

[PATCH v2 2/3] kernel/uprobes: Ratelimit messages

[Naveen N. Rao]

Ratelimit warnings from uprobes so as not to flood the kernel log.

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 kernel/events/uprobes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 4af1acff9cc3..e14eb0a6e4f3 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -114,8 +114,8 @@ struct xol_area {
 
 static void uprobe_warn(struct task_struct *t, const char *msg)
 {
-	pr_warn("uprobe: %s:%d failed to %s\n",
-			current->comm, current->pid, msg);
+	pr_warn_ratelimited("uprobe: %s:%d failed to %s\n",
+				current->comm, current->pid, msg);
 }
 
 /*
-- 
2.14.1

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

[Naveen N. Rao]

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

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

[Oleg Nesterov]

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.

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

[Naveen N. Rao]

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