[PATCH v3 0/3] A few uprobe fixes

[PATCH v3 0/3] A few uprobe fixes

[Naveen N. Rao]

This is v3 of the series posted at:
http://lkml.kernel.org/r/cover.1505333828.git.naveen.n.rao@linux.vnet.ibm.com

This has been updated to address review comments from Oleg.
- Patch 1 is new and removes uprobe_warn() in favour of pr_warn().
- Patch 2 is updated to accommodate change in patch 1.
- Patch 3 has been updated to remove the check for UPROBE_COPY_INSN in 
  handle_swbp().

- Naveen

Naveen N. Rao (3):
  kernel/uprobes: Do away with uprobe_warn()
  kernel/uprobes: Warn if unable to install breakpoint
  kernel/uprobes: Fix check for active uprobe

 kernel/events/uprobes.c | 58 +++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

-- 
2.14.1

[PATCH v3 1/3] kernel/uprobes: Do away with uprobe_warn()

[Naveen N. Rao]

uprobe_warn() was added in commit 248d3a7b2f1000 ("uprobes: Change
uprobe_copy_process() to dup return_instances") to have a uniform format
for warnings thrown from the uprobes subsystem. Though it has served
its role, there are a couple of issues with it:
1. Though it was meant to accept a 'struct task_struct *' and use that,
the actual code just uses 'current', completely ignoring the task
structure being passed in.
2. It does not accept varargs currently.
3. The strings are split making it difficult to grep for failures.

To address these and simplify things, this patch moves to using pr_fmt()
to encode the uprobe string prefix for all printk's in this file. The
prefix string uses 'current->comm' and 'current->pid', so we update the
messages in uprobe_copy_process() to explicitly refer to the other task
structure.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 kernel/events/uprobes.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 267f6ef91d97..18bd6127d00e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -42,6 +42,11 @@
 
 #include <linux/uprobes.h>
 
+#ifdef pr_fmt
+#undef pr_fmt
+#endif
+#define pr_fmt(fmt) "uprobe: %s:%d " fmt, current->comm, current->pid
+
 #define UINSNS_PER_PAGE			(PAGE_SIZE/UPROBE_XOL_SLOT_BYTES)
 #define MAX_UPROBE_XOL_SLOTS		UINSNS_PER_PAGE
 
@@ -1468,12 +1473,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)
@@ -1481,7 +1480,7 @@ static void dup_xol_work(struct callback_head *work)
 
 	if (!__create_xol_area(current->utask->dup_xol_addr) &&
 			!fatal_signal_pending(current))
-		uprobe_warn(current, "dup xol area");
+		pr_warn("failed to dup xol area");
 }
 
 /*
@@ -1501,13 +1500,17 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
 	if (mm == t->mm && !(flags & CLONE_VFORK))
 		return;
 
-	if (dup_utask(t, utask))
-		return uprobe_warn(t, "dup ret instances");
+	if (dup_utask(t, utask)) {
+		pr_warn("failed to dup ret instances for %s:%d", t->comm, t->pid);
+		return;
+	}
 
 	/* The task can fork() after dup_xol_work() fails */
 	area = mm->uprobes_state.xol_area;
-	if (!area)
-		return uprobe_warn(t, "dup xol area");
+	if (!area) {
+		pr_warn("failed to dup xol area for %s:%d", t->comm, t->pid);
+		return;
+	}
 
 	if (mm == t->mm)
 		return;
@@ -1594,7 +1597,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
 			 * This situation is not possible. Likely we have an
 			 * attack from user-space.
 			 */
-			uprobe_warn(current, "handle tail call");
+			pr_warn("failed to handle tail call");
 			goto fail;
 		}
 		orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
@@ -1853,7 +1856,7 @@ static void handle_trampoline(struct pt_regs *regs)
 	return;
 
  sigill:
-	uprobe_warn(current, "handle uretprobe, sending SIGILL.");
+	pr_warn("failed to handle uretprobe, sending SIGILL.");
 	force_sig_info(SIGILL, SEND_SIG_FORCED, current);
 
 }
@@ -1961,7 +1964,7 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
 	spin_unlock_irq(&current->sighand->siglock);
 
 	if (unlikely(err)) {
-		uprobe_warn(current, "execute the probed insn, sending SIGILL.");
+		pr_warn("failed to execute the probed insn, sending SIGILL.");
 		force_sig_info(SIGILL, SEND_SIG_FORCED, current);
 	}
 }
-- 
2.14.1

[PATCH v3 2/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.

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 18bd6127d00e..8da6570b4467 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1092,7 +1092,10 @@ 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)
+				pr_warn_ratelimited("failed to setup probe at 0x%llx (%d)",
+							uprobe->offset, ret);
 		}
 		put_uprobe(uprobe);
 	}
-- 
2.14.1

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

[Naveen N. Rao]

If we try to install a uprobe on a breakpoint instruction when the
binary has not yet been mmap'ed, we register the probe but delay
installing the breakpoint for when the binary is actually mmap'ed. On
mmap, uprobe_mmap() calls prepare_uprobe() which then refuses to install
the breakpoint. In this case however, when the breakpoint hits, we
incorrectly assume that the probe hit and end up looping.

This happens because find_active_uprobe() does not check if we
successfully installed the breakpoint or not. Fix this by checking if
UPROBE_COPY_INSN is set in uprobe->flags in find_active_uprobe().
Since handle_swbp() calls find_active_uprobe(), we can remove the
redundant check there.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 8da6570b4467..4c5dc6fb2abf 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1751,6 +1751,19 @@ 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) {
+			/*
+			 * TODO: move copy_insn/etc into _register and remove
+			 * this hack.  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 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 {
@@ -1911,15 +1924,6 @@ static void handle_swbp(struct pt_regs *regs)
 	/* change it in advance for ->handler() and restart */
 	instruction_pointer_set(regs, bp_vaddr);
 
-	/*
-	 * TODO: move copy_insn/etc into _register and remove this hack.
-	 * 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;
-
 	/* Tracing handlers use ->utask to communicate with fetch methods */
 	if (!get_utask())
 		goto out;
-- 
2.14.1

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

[Oleg Nesterov]

On 09/22, Naveen N. Rao wrote:
>
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1751,6 +1751,19 @@ 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) {
> +			/*
> +			 * TODO: move copy_insn/etc into _register and remove
> +			 * this hack.  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 prepare_uprobe() */
> +			if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
> +				uprobe = NULL;
> +		}

ACK ...

but the comment is no longer valid, it only mentions the race  unregister +
register.

And note that "restart" is not true in that we are not going to simply restart,
we will check is_trap_at_addr() and then either send SIGTRAP or restart.

This is correct because we do this check under mmap_sem so we can't race with
install_breakpoint(), so is_trap_at_addr() == T can't be falsely true if
UPROBE_COPY_INSN is not set.

And btw, perhaps you should do this check right after find_uprobe() in the
if (valid_vma) block.

Oleg.

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

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 09/22, Naveen N. Rao wrote:
> >
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -1751,6 +1751,19 @@ 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) {
> > +			/*
> > +			 * TODO: move copy_insn/etc into _register and remove
> > +			 * this hack.  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 prepare_uprobe() */
> > +			if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
> > +				uprobe = NULL;
> > +		}
> 
> ACK ...
> 
> but the comment is no longer valid, it only mentions the race  unregister +
> register.
> 
> And note that "restart" is not true in that we are not going to simply restart,
> we will check is_trap_at_addr() and then either send SIGTRAP or restart.
> 
> This is correct because we do this check under mmap_sem so we can't race with
> install_breakpoint(), so is_trap_at_addr() == T can't be falsely true if
> UPROBE_COPY_INSN is not set.
> 
> And btw, perhaps you should do this check right after find_uprobe() in the
> if (valid_vma) block.

Ok guys - I'll wait for the next iteration that incorporates Oleg's suggestions.

Thanks,

	Ingo

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

[Srikar Dronamraju]

> >
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -1751,6 +1751,19 @@ 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) {
> > +			/*
> > +			 * TODO: move copy_insn/etc into _register and remove
> > +			 * this hack.  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 prepare_uprobe() */
> > +			if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
> > +				uprobe = NULL;
> > +		}
> 
> ACK ...
> 
> but the comment is no longer valid, it only mentions the race  unregister +
> register.
> 
> And note that "restart" is not true in that we are not going to simply restart,
> we will check is_trap_at_addr() and then either send SIGTRAP or restart.
> 
> This is correct because we do this check under mmap_sem so we can't race with
> install_breakpoint(), so is_trap_at_addr() == T can't be falsely true if
> UPROBE_COPY_INSN is not set.
> 

Right, Given that we are doing this in the mmap_sem, we should also be
removing the rmb/wmb pairs too.

Right Oleg?


> And btw, perhaps you should do this check right after find_uprobe() in the
> if (valid_vma) block.
> 
> Oleg.
> 

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

[Oleg Nesterov]

On 09/29, Srikar Dronamraju wrote:
>
> > This is correct because we do this check under mmap_sem so we can't race with
> > install_breakpoint(), so is_trap_at_addr() == T can't be falsely true if
> > UPROBE_COPY_INSN is not set.
> >
>
> Right, Given that we are doing this in the mmap_sem, we should also be
> removing the rmb/wmb pairs too.

Well, down_read(&mm->mmap_sem) can only guarantee that this mm can not be
modified by install_breakpoint().

But what if, say, another task with different ->mm does uprobe_mmap() and
calls prepare_uprobe() for the 1st time?

Or suppose we race with unregister+register...

OTOH, I agree that we can remove these barriers, but this needs a lengthy
comment while the current code looks "obviously correct" in that you do
not even need to think about potential races.

Oleg.