2015-07-07 01:23:48

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 00/11] uprobes: longjmp fixes

Sorry for delay,

Currently ret-probes can't work (the application will likely crash)
if the probed function does not return, and this is even documented
in handle_trampoline().

This series tries to make the first step to fix the problem, assuming
that the probed functions use the same stack.

TODO: sigaltstack() can obviously break this assumption.

NOTE: I don't think it is possible to make this logic 100% correct,
the user-space can do everything with its stack. For example, the
application can do longjmp-like tricks to implement the coroutines,
the kernel can do nothing in this case. The application (or debugger)
should cooperate somehow to let the kernel know whats going on.

v2, based on disccsussion with Srikar and Pratyush:

1-5: Unchanged, I preserved the acks from Srikar.

6-11: The only essential change is that we do not add the
(ugly) arch_uretprobe, we just export return_instance
to arch/.

This means that we do not need to touch the !x86 code,
and return_instance->stack can be initialized by the
generic code.

Srikar, I hope you can ack v2 too.

10/11: New. As Pratyush pointed out "bool on_call" is too
limited.

Plus v2 fixes the problem mentioned in "self nack" email, we must
not do cleanup_return_instances() after prepare_uretprobe() checks
chained && utask->return_instances != NULL.

Oleg.

arch/x86/kernel/uprobes.c | 9 ++
include/linux/uprobes.h | 17 ++++
kernel/events/uprobes.c | 184 +++++++++++++++++++++++++--------------------
3 files changed, 128 insertions(+), 82 deletions(-)


2015-07-07 01:24:17

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 01/11] uprobes: Introduce get_uprobe()

Cosmetic. Add the new trivial helper, get_uprobe(). It matches
put_uprobe() we already have and we can simplify a couple of its
users.

Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Srikar Dronamraju <[email protected]>
---
kernel/events/uprobes.c | 39 ++++++++++++++++++++-------------------
1 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index cb346f2..a9847b4 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -366,6 +366,18 @@ set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long v
return uprobe_write_opcode(mm, vaddr, *(uprobe_opcode_t *)&auprobe->insn);
}

+static struct uprobe *get_uprobe(struct uprobe *uprobe)
+{
+ atomic_inc(&uprobe->ref);
+ return uprobe;
+}
+
+static void put_uprobe(struct uprobe *uprobe)
+{
+ if (atomic_dec_and_test(&uprobe->ref))
+ kfree(uprobe);
+}
+
static int match_uprobe(struct uprobe *l, struct uprobe *r)
{
if (l->inode < r->inode)
@@ -393,10 +405,8 @@ static struct uprobe *__find_uprobe(struct inode *inode, loff_t offset)
while (n) {
uprobe = rb_entry(n, struct uprobe, rb_node);
match = match_uprobe(&u, uprobe);
- if (!match) {
- atomic_inc(&uprobe->ref);
- return uprobe;
- }
+ if (!match)
+ return get_uprobe(uprobe);

if (match < 0)
n = n->rb_left;
@@ -432,10 +442,8 @@ static struct uprobe *__insert_uprobe(struct uprobe *uprobe)
parent = *p;
u = rb_entry(parent, struct uprobe, rb_node);
match = match_uprobe(uprobe, u);
- if (!match) {
- atomic_inc(&u->ref);
- return u;
- }
+ if (!match)
+ return get_uprobe(u);

if (match < 0)
p = &parent->rb_left;
@@ -472,12 +480,6 @@ static struct uprobe *insert_uprobe(struct uprobe *uprobe)
return u;
}

-static void put_uprobe(struct uprobe *uprobe)
-{
- if (atomic_dec_and_test(&uprobe->ref))
- kfree(uprobe);
-}
-
static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
{
struct uprobe *uprobe, *cur_uprobe;
@@ -1039,14 +1041,14 @@ static void build_probe_list(struct inode *inode,
if (u->inode != inode || u->offset < min)
break;
list_add(&u->pending_list, head);
- atomic_inc(&u->ref);
+ get_uprobe(u);
}
for (t = n; (t = rb_next(t)); ) {
u = rb_entry(t, struct uprobe, rb_node);
if (u->inode != inode || u->offset > max)
break;
list_add(&u->pending_list, head);
- atomic_inc(&u->ref);
+ get_uprobe(u);
}
}
spin_unlock(&uprobes_treelock);
@@ -1437,7 +1439,7 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
return -ENOMEM;

*n = *o;
- atomic_inc(&n->uprobe->ref);
+ get_uprobe(n->uprobe);
n->next = NULL;

*p = n;
@@ -1565,8 +1567,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
}

- atomic_inc(&uprobe->ref);
- ri->uprobe = uprobe;
+ ri->uprobe = get_uprobe(uprobe);
ri->func = instruction_pointer(regs);
ri->orig_ret_vaddr = orig_ret_vaddr;
ri->chained = chained;
--
1.5.5.1

2015-07-07 01:24:27

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 02/11] uprobes: Introduce free_ret_instance()

We can simplify uprobe_free_utask() and handle_uretprobe_chain()
if we add a simple helper which does put_uprobe/kfree and returns
the ->next return_instance.

Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Srikar Dronamraju <[email protected]>
---
kernel/events/uprobes.c | 27 +++++++++++++--------------
1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a9847b4..d8c702f 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1378,6 +1378,14 @@ unsigned long uprobe_get_trap_addr(struct pt_regs *regs)
return instruction_pointer(regs);
}

+static struct return_instance *free_ret_instance(struct return_instance *ri)
+{
+ struct return_instance *next = ri->next;
+ put_uprobe(ri->uprobe);
+ kfree(ri);
+ return next;
+}
+
/*
* Called with no locks held.
* Called in context of a exiting or a exec-ing thread.
@@ -1385,7 +1393,7 @@ unsigned long uprobe_get_trap_addr(struct pt_regs *regs)
void uprobe_free_utask(struct task_struct *t)
{
struct uprobe_task *utask = t->utask;
- struct return_instance *ri, *tmp;
+ struct return_instance *ri;

if (!utask)
return;
@@ -1394,13 +1402,8 @@ void uprobe_free_utask(struct task_struct *t)
put_uprobe(utask->active_uprobe);

ri = utask->return_instances;
- while (ri) {
- tmp = ri;
- ri = ri->next;
-
- put_uprobe(tmp->uprobe);
- kfree(tmp);
- }
+ while (ri)
+ ri = free_ret_instance(ri);

xol_free_insn_slot(t);
kfree(utask);
@@ -1770,7 +1773,7 @@ handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
static bool handle_trampoline(struct pt_regs *regs)
{
struct uprobe_task *utask;
- struct return_instance *ri, *tmp;
+ struct return_instance *ri;
bool chained;

utask = current->utask;
@@ -1792,11 +1795,7 @@ static bool handle_trampoline(struct pt_regs *regs)
handle_uretprobe_chain(ri, regs);

chained = ri->chained;
- put_uprobe(ri->uprobe);
-
- tmp = ri;
- ri = ri->next;
- kfree(tmp);
+ ri = free_ret_instance(ri);
utask->depth--;

if (!chained)
--
1.5.5.1

2015-07-07 01:24:41

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 03/11] uprobes: Send SIGILL if handle_trampoline() fails

1. It doesn't make sense to continue if handle_trampoline() fails,
change handle_swbp() to always return after this call.

2. Turn pr_warn() into uprobe_warn(), and change handle_trampoline()
to send SIGILL on failure. It is pointless to return to user mode
with the corrupted instruction_pointer() which we can't restore.

Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Srikar Dronamraju <[email protected]>
---
kernel/events/uprobes.c | 21 ++++++++++-----------
1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index d8c702f..eabdc21 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1770,7 +1770,7 @@ handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
up_read(&uprobe->register_rwsem);
}

-static bool handle_trampoline(struct pt_regs *regs)
+static void handle_trampoline(struct pt_regs *regs)
{
struct uprobe_task *utask;
struct return_instance *ri;
@@ -1778,11 +1778,11 @@ static bool handle_trampoline(struct pt_regs *regs)

utask = current->utask;
if (!utask)
- return false;
+ goto sigill;

ri = utask->return_instances;
if (!ri)
- return false;
+ goto sigill;

/*
* TODO: we should throw out return_instance's invalidated by
@@ -1804,8 +1804,12 @@ static bool handle_trampoline(struct pt_regs *regs)
}

utask->return_instances = ri;
+ return;
+
+ sigill:
+ uprobe_warn(current, "handle uretprobe, sending SIGILL.");
+ force_sig_info(SIGILL, SEND_SIG_FORCED, current);

- return true;
}

bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs)
@@ -1824,13 +1828,8 @@ static void handle_swbp(struct pt_regs *regs)
int uninitialized_var(is_swbp);

bp_vaddr = uprobe_get_swbp_addr(regs);
- if (bp_vaddr == get_trampoline_vaddr()) {
- if (handle_trampoline(regs))
- return;
-
- pr_warn("uprobe: unable to handle uretprobe pid/tgid=%d/%d\n",
- current->pid, current->tgid);
- }
+ if (bp_vaddr == get_trampoline_vaddr())
+ return handle_trampoline(regs);

uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
if (!uprobe) {
--
1.5.5.1

2015-07-07 01:24:35

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 04/11] uprobes: Change prepare_uretprobe() to use uprobe_warn()

Turn the last pr_warn() in uprobes.c into uprobe_warn().

While at it:

- s/kzalloc/kmalloc, we initialize every member of ri

- remove the pointless comment above the obvious code

Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Srikar Dronamraju <[email protected]>
---
kernel/events/uprobes.c | 10 +++-------
1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index eabdc21..4c941fe 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1541,9 +1541,9 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
return;
}

- ri = kzalloc(sizeof(struct return_instance), GFP_KERNEL);
+ ri = kmalloc(sizeof(struct return_instance), GFP_KERNEL);
if (!ri)
- goto fail;
+ return;

trampoline_vaddr = get_trampoline_vaddr();
orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
@@ -1561,8 +1561,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.
*/
- pr_warn("uprobe: unable to set uretprobe pid/tgid=%d/%d\n",
- current->pid, current->tgid);
+ uprobe_warn(current, "handle tail call");
goto fail;
}

@@ -1576,13 +1575,10 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
ri->chained = chained;

utask->depth++;
-
- /* add instance to the stack */
ri->next = utask->return_instances;
utask->return_instances = ri;

return;
-
fail:
kfree(ri);
}
--
1.5.5.1

2015-07-07 01:26:39

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 05/11] uprobes: Change handle_trampoline() to find the next chain beforehand

No functional changes, preparation.

Add the new helper, find_next_ret_chain(), which finds the first !chained
entry and returns its ->next. Yes, it is suboptimal. We probably want to
turn ->chained into ->start_of_this_chain pointer and avoid another loop.
But this needs the boring changes in dup_utask(), so lets do this later.

Change the main loop in handle_trampoline() to unwind the stack until ri
is equal to the pointer returned by this new helper.

Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Srikar Dronamraju <[email protected]>
---
kernel/events/uprobes.c | 27 ++++++++++++++++-----------
1 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 4c941fe..98e4d97 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1766,11 +1766,22 @@ handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
up_read(&uprobe->register_rwsem);
}

+static struct return_instance *find_next_ret_chain(struct return_instance *ri)
+{
+ bool chained;
+
+ do {
+ chained = ri->chained;
+ ri = ri->next; /* can't be NULL if chained */
+ } while (chained);
+
+ return ri;
+}
+
static void handle_trampoline(struct pt_regs *regs)
{
struct uprobe_task *utask;
- struct return_instance *ri;
- bool chained;
+ struct return_instance *ri, *next;

utask = current->utask;
if (!utask)
@@ -1780,24 +1791,18 @@ static void handle_trampoline(struct pt_regs *regs)
if (!ri)
goto sigill;

+ next = find_next_ret_chain(ri);
/*
* TODO: we should throw out return_instance's invalidated by
* longjmp(), currently we assume that the probed function always
* returns.
*/
instruction_pointer_set(regs, ri->orig_ret_vaddr);
-
- for (;;) {
+ do {
handle_uretprobe_chain(ri, regs);
-
- chained = ri->chained;
ri = free_ret_instance(ri);
utask->depth--;
-
- if (!chained)
- break;
- BUG_ON(!ri);
- }
+ } while (ri != next);

utask->return_instances = ri;
return;
--
1.5.5.1

2015-07-07 01:26:08

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 06/11] uprobes: Export struct return_instance, introduce arch_uretprobe_is_alive()

Add the new "weak" helper, arch_uretprobe_is_alive(), used by the next
patches. It should return true if this return_instance is still valid.
The arch agnostic version just always returns true.

The patch exports "struct return_instance" for the architectures which
want to override this hook. We can also cleanup prepare_uretprobe() if
we pass the new return_instance to arch_uretprobe_hijack_return_addr().

Signed-off-by: Oleg Nesterov <[email protected]>
---
include/linux/uprobes.h | 10 ++++++++++
kernel/events/uprobes.c | 14 +++++---------
2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 60beb5d..50d2764 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -92,6 +92,15 @@ struct uprobe_task {
unsigned int depth;
};

+struct return_instance {
+ struct uprobe *uprobe;
+ unsigned long func;
+ unsigned long orig_ret_vaddr; /* original return address */
+ bool chained; /* true, if instance is nested */
+
+ struct return_instance *next; /* keep as stack */
+};
+
struct xol_area;

struct uprobes_state {
@@ -128,6 +137,7 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
+extern bool arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs);
extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
void *src, unsigned long len);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 98e4d97..1c71b62 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -86,15 +86,6 @@ struct uprobe {
struct arch_uprobe arch;
};

-struct return_instance {
- struct uprobe *uprobe;
- unsigned long func;
- unsigned long orig_ret_vaddr; /* original return address */
- bool chained; /* true, if instance is nested */
-
- struct return_instance *next; /* keep as stack */
-};
-
/*
* Execute out of line area: anonymous executable mapping installed
* by the probed task to execute the copy of the original instruction
@@ -1818,6 +1809,11 @@ bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs)
return false;
}

+bool __weak arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs)
+{
+ return true;
+}
+
/*
* Run handler and ask thread to singlestep.
* Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
--
1.5.5.1

2015-07-07 01:24:47

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 07/11] uprobes/x86: Reimplement arch_uretprobe_is_alive()

Add the x86-specific version of arch_uretprobe_is_alive() helper.
It returns true if the stack frame mangled by prepare_uretprobe()
is still on stack. So if it returns false, we know that the probed
function has already returned.

We add the new return_instance->stack member and change the generic
code to initialize it in prepare_uretprobe, but it should be equally
useful for other architectures.

TODO: this assumes that the probed application can't use multiple
stacks (say sigaltstack). We will try to improve this logic later.

Signed-off-by: Oleg Nesterov <[email protected]>
---
arch/x86/kernel/uprobes.c | 5 +++++
include/linux/uprobes.h | 1 +
kernel/events/uprobes.c | 1 +
3 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 0b81ad6..9d5f570 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -993,3 +993,8 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs

return -1;
}
+
+bool arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs)
+{
+ return regs->sp <= ret->stack;
+}
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 50d2764..7ab6d2c 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -95,6 +95,7 @@ struct uprobe_task {
struct return_instance {
struct uprobe *uprobe;
unsigned long func;
+ unsigned long stack; /* stack pointer */
unsigned long orig_ret_vaddr; /* original return address */
bool chained; /* true, if instance is nested */

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 1c71b62..c5f316e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1562,6 +1562,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)

ri->uprobe = get_uprobe(uprobe);
ri->func = instruction_pointer(regs);
+ ri->stack = user_stack_pointer(regs);
ri->orig_ret_vaddr = orig_ret_vaddr;
ri->chained = chained;

--
1.5.5.1

2015-07-07 01:25:58

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 08/11] uprobes: Change handle_trampoline() to flush the frames invalidated by longjmp()

Test-case:

#include <stdio.h>
#include <setjmp.h>

jmp_buf jmp;

void func_2(void)
{
longjmp(jmp, 1);
}

void func_1(void)
{
if (setjmp(jmp))
return;
func_2();
printf("ERR!! I am running on the caller's stack\n");
}

int main(void)
{
func_1();
return 0;
}

fails if you probe func_1() and func_2() because handle_trampoline()
assumes that the probed function should must return and hit the bp
installed be prepare_uretprobe(). But in this case func_2() does not
return, so when func_1() returns the kernel uses the no longer valid
return_instance of func_2().

Change handle_trampoline() to unwind ->return_instances until we know
that the next chain is alive or NULL, this ensures that the current
chain is the last we need to report and free.

Alternatively, every return_instance could use unique trampoline_vaddr,
in this case we could use it as a key. And this could solve the problem
with sigaltstack() automatically.

But this approach needs more changes, and it puts the "hard" limit on
MAX_URETPROBE_DEPTH. Plus it can not solve another problem partially
fixed by the next patch.

Note: this change has no effect on !x86, the arch-agnostic version of
arch_uretprobe_is_alive() just returns "true".

TODO: as documented by the previous change, arch_uretprobe_is_alive()
can be fooled by sigaltstack/etc.

Signed-off-by: Oleg Nesterov <[email protected]>
---
kernel/events/uprobes.c | 29 ++++++++++++++++++-----------
1 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index c5f316e..93d939c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1774,6 +1774,7 @@ static void handle_trampoline(struct pt_regs *regs)
{
struct uprobe_task *utask;
struct return_instance *ri, *next;
+ bool valid;

utask = current->utask;
if (!utask)
@@ -1783,18 +1784,24 @@ static void handle_trampoline(struct pt_regs *regs)
if (!ri)
goto sigill;

- next = find_next_ret_chain(ri);
- /*
- * TODO: we should throw out return_instance's invalidated by
- * longjmp(), currently we assume that the probed function always
- * returns.
- */
- instruction_pointer_set(regs, ri->orig_ret_vaddr);
do {
- handle_uretprobe_chain(ri, regs);
- ri = free_ret_instance(ri);
- utask->depth--;
- } while (ri != next);
+ /*
+ * We should throw out the frames invalidated by longjmp().
+ * If this chain is valid, then the next one should be alive
+ * or NULL; the latter case means that nobody but ri->func
+ * could hit this trampoline on return. TODO: sigaltstack().
+ */
+ next = find_next_ret_chain(ri);
+ valid = !next || arch_uretprobe_is_alive(next, regs);
+
+ instruction_pointer_set(regs, ri->orig_ret_vaddr);
+ do {
+ if (valid)
+ handle_uretprobe_chain(ri, regs);
+ ri = free_ret_instance(ri);
+ utask->depth--;
+ } while (ri != next);
+ } while (!valid);

utask->return_instances = ri;
return;
--
1.5.5.1

2015-07-07 01:24:57

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 09/11] uprobes: Change prepare_uretprobe() to (try to) flush the dead frames

Change prepare_uretprobe() to flush the !arch_uretprobe_is_alive()
return_instance's. This is not needed correctness-wise, but can help
to avoid the failure caused by MAX_URETPROBE_DEPTH.

Note: in this case arch_uretprobe_is_alive() can be false positive,
the stack can grow after longjmp(). Unfortunately, the kernel can't
100% solve this problem, but see the next patch.

Signed-off-by: Oleg Nesterov <[email protected]>
---
kernel/events/uprobes.c | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 93d939c..7e61c8c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1511,6 +1511,16 @@ static unsigned long get_trampoline_vaddr(void)
return trampoline_vaddr;
}

+static void cleanup_return_instances(struct uprobe_task *utask, struct pt_regs *regs)
+{
+ struct return_instance *ri = utask->return_instances;
+ while (ri && !arch_uretprobe_is_alive(ri, regs)) {
+ ri = free_ret_instance(ri);
+ utask->depth--;
+ }
+ utask->return_instances = ri;
+}
+
static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
{
struct return_instance *ri;
@@ -1541,6 +1551,9 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
if (orig_ret_vaddr == -1)
goto fail;

+ /* drop the entries invalidated by longjmp() */
+ cleanup_return_instances(utask, regs);
+
/*
* We don't want to keep trampoline address in stack, rather keep the
* original return address of first caller thru all the consequent
--
1.5.5.1

2015-07-07 01:25:09

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 10/11] uprobes: Add the "enum rp_check ctx" arg to arch_uretprobe_is_alive()

arch/x86 doesn't care (so far), but as Pratyush Anand pointed out
other architectures might want why arch_uretprobe_is_alive() was
called and use different checks depending on the context. Add the
new argument to distinguish 2 callers.

Signed-off-by: Oleg Nesterov <[email protected]>
---
arch/x86/kernel/uprobes.c | 3 ++-
include/linux/uprobes.h | 7 ++++++-
kernel/events/uprobes.c | 9 ++++++---
3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 9d5f570..67eb168 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -994,7 +994,8 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs
return -1;
}

-bool arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs)
+bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check ctx,
+ struct pt_regs *regs)
{
return regs->sp <= ret->stack;
}
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 7ab6d2c..c0a5402 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -102,6 +102,11 @@ struct return_instance {
struct return_instance *next; /* keep as stack */
};

+enum rp_check {
+ RP_CHECK_CALL,
+ RP_CHECK_RET,
+};
+
struct xol_area;

struct uprobes_state {
@@ -138,7 +143,7 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
-extern bool arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs);
+extern bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check ctx, struct pt_regs *regs);
extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
void *src, unsigned long len);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 7e61c8c..df5661a 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1514,7 +1514,9 @@ static unsigned long get_trampoline_vaddr(void)
static void cleanup_return_instances(struct uprobe_task *utask, struct pt_regs *regs)
{
struct return_instance *ri = utask->return_instances;
- while (ri && !arch_uretprobe_is_alive(ri, regs)) {
+ enum rp_check ctx = RP_CHECK_CALL;
+
+ while (ri && !arch_uretprobe_is_alive(ri, ctx, regs)) {
ri = free_ret_instance(ri);
utask->depth--;
}
@@ -1805,7 +1807,7 @@ static void handle_trampoline(struct pt_regs *regs)
* could hit this trampoline on return. TODO: sigaltstack().
*/
next = find_next_ret_chain(ri);
- valid = !next || arch_uretprobe_is_alive(next, regs);
+ valid = !next || arch_uretprobe_is_alive(next, RP_CHECK_RET, regs);

instruction_pointer_set(regs, ri->orig_ret_vaddr);
do {
@@ -1830,7 +1832,8 @@ bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs)
return false;
}

-bool __weak arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs)
+bool __weak arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check ctx,
+ struct pt_regs *regs)
{
return true;
}
--
1.5.5.1

2015-07-07 01:25:34

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH v2 11/11] uprobes/x86: Make arch_uretprobe_is_alive(RP_CHECK_CALL) more clever

The previous change documents that cleanup_return_instances() can't
always detect the dead frames, the stack can grow. But there is one
special case which imho worth fixing: arch_uretprobe_is_alive() can
return true when the stack didn't actually grow, but the next "call"
insn uses the already invalidated frame.

Test-case:

#include <stdio.h>
#include <setjmp.h>

jmp_buf jmp;
int nr = 1024;

void func_2(void)
{
if (--nr == 0)
return;
longjmp(jmp, 1);
}

void func_1(void)
{
setjmp(jmp);
func_2();
}

int main(void)
{
func_1();
return 0;
}

If you ret-probe func_1() and func_2() prepare_uretprobe() hits the
MAX_URETPROBE_DEPTH limit and "return" from func_2() is not reported.

When we know that the new call is not chained, we can do the more
strict check. In this case "sp" points to the new ret-addr, so every
frame which uses the same "sp" must be dead. The only complication is
that arch_uretprobe_is_alive() needs to know was it chained or not, so
we add the new RP_CHECK_CHAIN_CALL enum and change prepare_uretprobe()
to pass RP_CHECK_CALL only if !chained.

Note: arch_uretprobe_is_alive() could also re-read *sp and check if
this word is still trampoline_vaddr. This could obviously improve the
logic, but I would like to avoid another copy_from_user() especially
in the case when we can't avoid the false "alive == T" positives.

Signed-off-by: Oleg Nesterov <[email protected]>
---
arch/x86/kernel/uprobes.c | 5 ++++-
include/linux/uprobes.h | 1 +
kernel/events/uprobes.c | 14 +++++++-------
3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 67eb168..a5c59f2 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -997,5 +997,8 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs
bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check ctx,
struct pt_regs *regs)
{
- return regs->sp <= ret->stack;
+ if (ctx == RP_CHECK_CALL) /* sp was just decremented by "call" insn */
+ return regs->sp < ret->stack;
+ else
+ return regs->sp <= ret->stack;
}
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index c0a5402..0bdc72f 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -104,6 +104,7 @@ struct return_instance {

enum rp_check {
RP_CHECK_CALL,
+ RP_CHECK_CHAIN_CALL,
RP_CHECK_RET,
};

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index df5661a..0f370ef 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1511,10 +1511,11 @@ static unsigned long get_trampoline_vaddr(void)
return trampoline_vaddr;
}

-static void cleanup_return_instances(struct uprobe_task *utask, struct pt_regs *regs)
+static void cleanup_return_instances(struct uprobe_task *utask, bool chained,
+ struct pt_regs *regs)
{
struct return_instance *ri = utask->return_instances;
- enum rp_check ctx = RP_CHECK_CALL;
+ enum rp_check ctx = chained ? RP_CHECK_CHAIN_CALL : RP_CHECK_CALL;

while (ri && !arch_uretprobe_is_alive(ri, ctx, regs)) {
ri = free_ret_instance(ri);
@@ -1528,7 +1529,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
struct return_instance *ri;
struct uprobe_task *utask;
unsigned long orig_ret_vaddr, trampoline_vaddr;
- bool chained = false;
+ bool chained;

if (!get_xol_area())
return;
@@ -1554,14 +1555,15 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
goto fail;

/* drop the entries invalidated by longjmp() */
- cleanup_return_instances(utask, regs);
+ chained = (orig_ret_vaddr == trampoline_vaddr);
+ cleanup_return_instances(utask, chained, regs);

/*
* We don't want to keep trampoline address in stack, rather keep the
* original return address of first caller thru all the consequent
* instances. This also makes breakpoint unwrapping easier.
*/
- if (orig_ret_vaddr == trampoline_vaddr) {
+ if (chained) {
if (!utask->return_instances) {
/*
* This situation is not possible. Likely we have an
@@ -1570,8 +1572,6 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
uprobe_warn(current, "handle tail call");
goto fail;
}
-
- chained = true;
orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
}

--
1.5.5.1

2015-07-07 12:44:56

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 01/11] uprobes: Introduce get_uprobe()

On Tue, Jul 07, 2015 at 03:22:35AM +0200, Oleg Nesterov wrote:
> Cosmetic. Add the new trivial helper, get_uprobe(). It matches
> put_uprobe() we already have and we can simplify a couple of its
> users.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
> Acked-by: Srikar Dronamraju <[email protected]>
Acked-by: Anton Arapov <[email protected]>

> ---
> kernel/events/uprobes.c | 39 ++++++++++++++++++++-------------------
> 1 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index cb346f2..a9847b4 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -366,6 +366,18 @@ set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long v
> return uprobe_write_opcode(mm, vaddr, *(uprobe_opcode_t *)&auprobe->insn);
> }
>
> +static struct uprobe *get_uprobe(struct uprobe *uprobe)
> +{
> + atomic_inc(&uprobe->ref);
> + return uprobe;
> +}
> +
> +static void put_uprobe(struct uprobe *uprobe)
> +{
> + if (atomic_dec_and_test(&uprobe->ref))
> + kfree(uprobe);
> +}
> +
> static int match_uprobe(struct uprobe *l, struct uprobe *r)
> {
> if (l->inode < r->inode)
> @@ -393,10 +405,8 @@ static struct uprobe *__find_uprobe(struct inode *inode, loff_t offset)
> while (n) {
> uprobe = rb_entry(n, struct uprobe, rb_node);
> match = match_uprobe(&u, uprobe);
> - if (!match) {
> - atomic_inc(&uprobe->ref);
> - return uprobe;
> - }
> + if (!match)
> + return get_uprobe(uprobe);
>
> if (match < 0)
> n = n->rb_left;
> @@ -432,10 +442,8 @@ static struct uprobe *__insert_uprobe(struct uprobe *uprobe)
> parent = *p;
> u = rb_entry(parent, struct uprobe, rb_node);
> match = match_uprobe(uprobe, u);
> - if (!match) {
> - atomic_inc(&u->ref);
> - return u;
> - }
> + if (!match)
> + return get_uprobe(u);
>
> if (match < 0)
> p = &parent->rb_left;
> @@ -472,12 +480,6 @@ static struct uprobe *insert_uprobe(struct uprobe *uprobe)
> return u;
> }
>
> -static void put_uprobe(struct uprobe *uprobe)
> -{
> - if (atomic_dec_and_test(&uprobe->ref))
> - kfree(uprobe);
> -}
> -
> static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
> {
> struct uprobe *uprobe, *cur_uprobe;
> @@ -1039,14 +1041,14 @@ static void build_probe_list(struct inode *inode,
> if (u->inode != inode || u->offset < min)
> break;
> list_add(&u->pending_list, head);
> - atomic_inc(&u->ref);
> + get_uprobe(u);
> }
> for (t = n; (t = rb_next(t)); ) {
> u = rb_entry(t, struct uprobe, rb_node);
> if (u->inode != inode || u->offset > max)
> break;
> list_add(&u->pending_list, head);
> - atomic_inc(&u->ref);
> + get_uprobe(u);
> }
> }
> spin_unlock(&uprobes_treelock);
> @@ -1437,7 +1439,7 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
> return -ENOMEM;
>
> *n = *o;
> - atomic_inc(&n->uprobe->ref);
> + get_uprobe(n->uprobe);
> n->next = NULL;
>
> *p = n;
> @@ -1565,8 +1567,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
> }
>
> - atomic_inc(&uprobe->ref);
> - ri->uprobe = uprobe;
> + ri->uprobe = get_uprobe(uprobe);
> ri->func = instruction_pointer(regs);
> ri->orig_ret_vaddr = orig_ret_vaddr;
> ri->chained = chained;
> --
> 1.5.5.1
>

2015-07-07 12:46:34

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 02/11] uprobes: Introduce free_ret_instance()

On Tue, Jul 07, 2015 at 03:22:39AM +0200, Oleg Nesterov wrote:
> We can simplify uprobe_free_utask() and handle_uretprobe_chain()
> if we add a simple helper which does put_uprobe/kfree and returns
> the ->next return_instance.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
> Acked-by: Srikar Dronamraju <[email protected]>
Acked-by: Anton Arapov <[email protected]>

> ---
> kernel/events/uprobes.c | 27 +++++++++++++--------------
> 1 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index a9847b4..d8c702f 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1378,6 +1378,14 @@ unsigned long uprobe_get_trap_addr(struct pt_regs *regs)
> return instruction_pointer(regs);
> }
>
> +static struct return_instance *free_ret_instance(struct return_instance *ri)
> +{
> + struct return_instance *next = ri->next;
> + put_uprobe(ri->uprobe);
> + kfree(ri);
> + return next;
> +}
> +
> /*
> * Called with no locks held.
> * Called in context of a exiting or a exec-ing thread.
> @@ -1385,7 +1393,7 @@ unsigned long uprobe_get_trap_addr(struct pt_regs *regs)
> void uprobe_free_utask(struct task_struct *t)
> {
> struct uprobe_task *utask = t->utask;
> - struct return_instance *ri, *tmp;
> + struct return_instance *ri;
>
> if (!utask)
> return;
> @@ -1394,13 +1402,8 @@ void uprobe_free_utask(struct task_struct *t)
> put_uprobe(utask->active_uprobe);
>
> ri = utask->return_instances;
> - while (ri) {
> - tmp = ri;
> - ri = ri->next;
> -
> - put_uprobe(tmp->uprobe);
> - kfree(tmp);
> - }
> + while (ri)
> + ri = free_ret_instance(ri);
>
> xol_free_insn_slot(t);
> kfree(utask);
> @@ -1770,7 +1773,7 @@ handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> static bool handle_trampoline(struct pt_regs *regs)
> {
> struct uprobe_task *utask;
> - struct return_instance *ri, *tmp;
> + struct return_instance *ri;
> bool chained;
>
> utask = current->utask;
> @@ -1792,11 +1795,7 @@ static bool handle_trampoline(struct pt_regs *regs)
> handle_uretprobe_chain(ri, regs);
>
> chained = ri->chained;
> - put_uprobe(ri->uprobe);
> -
> - tmp = ri;
> - ri = ri->next;
> - kfree(tmp);
> + ri = free_ret_instance(ri);
> utask->depth--;
>
> if (!chained)
> --
> 1.5.5.1
>

2015-07-07 12:51:36

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 03/11] uprobes: Send SIGILL if handle_trampoline() fails

On Tue, Jul 07, 2015 at 03:22:43AM +0200, Oleg Nesterov wrote:
> 1. It doesn't make sense to continue if handle_trampoline() fails,
> change handle_swbp() to always return after this call.
>
> 2. Turn pr_warn() into uprobe_warn(), and change handle_trampoline()
> to send SIGILL on failure. It is pointless to return to user mode
> with the corrupted instruction_pointer() which we can't restore.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
> Acked-by: Srikar Dronamraju <[email protected]>
Acked-by: Anton Arapov <[email protected]>


> ---
> kernel/events/uprobes.c | 21 ++++++++++-----------
> 1 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index d8c702f..eabdc21 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1770,7 +1770,7 @@ handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> up_read(&uprobe->register_rwsem);
> }
>
> -static bool handle_trampoline(struct pt_regs *regs)
> +static void handle_trampoline(struct pt_regs *regs)
> {
> struct uprobe_task *utask;
> struct return_instance *ri;
> @@ -1778,11 +1778,11 @@ static bool handle_trampoline(struct pt_regs *regs)
>
> utask = current->utask;
> if (!utask)
> - return false;
> + goto sigill;
>
> ri = utask->return_instances;
> if (!ri)
> - return false;
> + goto sigill;
>
> /*
> * TODO: we should throw out return_instance's invalidated by
> @@ -1804,8 +1804,12 @@ static bool handle_trampoline(struct pt_regs *regs)
> }
>
> utask->return_instances = ri;
> + return;
> +
> + sigill:
> + uprobe_warn(current, "handle uretprobe, sending SIGILL.");
> + force_sig_info(SIGILL, SEND_SIG_FORCED, current);
>
> - return true;
> }
>
> bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs)
> @@ -1824,13 +1828,8 @@ static void handle_swbp(struct pt_regs *regs)
> int uninitialized_var(is_swbp);
>
> bp_vaddr = uprobe_get_swbp_addr(regs);
> - if (bp_vaddr == get_trampoline_vaddr()) {
> - if (handle_trampoline(regs))
> - return;
> -
> - pr_warn("uprobe: unable to handle uretprobe pid/tgid=%d/%d\n",
> - current->pid, current->tgid);
> - }
> + if (bp_vaddr == get_trampoline_vaddr())
> + return handle_trampoline(regs);
>
> uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> if (!uprobe) {
> --
> 1.5.5.1
>

2015-07-07 12:52:40

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 04/11] uprobes: Change prepare_uretprobe() to use uprobe_warn()

On Tue, Jul 07, 2015 at 03:22:47AM +0200, Oleg Nesterov wrote:
> Turn the last pr_warn() in uprobes.c into uprobe_warn().
>
> While at it:
>
> - s/kzalloc/kmalloc, we initialize every member of ri
>
> - remove the pointless comment above the obvious code
>
> Signed-off-by: Oleg Nesterov <[email protected]>
> Acked-by: Srikar Dronamraju <[email protected]>
Acked-by: Anton Arapov <[email protected]>

> ---
> kernel/events/uprobes.c | 10 +++-------
> 1 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index eabdc21..4c941fe 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1541,9 +1541,9 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> return;
> }
>
> - ri = kzalloc(sizeof(struct return_instance), GFP_KERNEL);
> + ri = kmalloc(sizeof(struct return_instance), GFP_KERNEL);
> if (!ri)
> - goto fail;
> + return;
>
> trampoline_vaddr = get_trampoline_vaddr();
> orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
> @@ -1561,8 +1561,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.
> */
> - pr_warn("uprobe: unable to set uretprobe pid/tgid=%d/%d\n",
> - current->pid, current->tgid);
> + uprobe_warn(current, "handle tail call");
> goto fail;
> }
>
> @@ -1576,13 +1575,10 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> ri->chained = chained;
>
> utask->depth++;
> -
> - /* add instance to the stack */
> ri->next = utask->return_instances;
> utask->return_instances = ri;
>
> return;
> -
> fail:
> kfree(ri);
> }
> --
> 1.5.5.1
>

2015-07-07 12:54:47

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 05/11] uprobes: Change handle_trampoline() to find the next chain beforehand

On Tue, Jul 07, 2015 at 03:22:50AM +0200, Oleg Nesterov wrote:
> No functional changes, preparation.
>
> Add the new helper, find_next_ret_chain(), which finds the first !chained
> entry and returns its ->next. Yes, it is suboptimal. We probably want to
> turn ->chained into ->start_of_this_chain pointer and avoid another loop.
> But this needs the boring changes in dup_utask(), so lets do this later.
>
> Change the main loop in handle_trampoline() to unwind the stack until ri
> is equal to the pointer returned by this new helper.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
> Acked-by: Srikar Dronamraju <[email protected]>
Acked-by: Anton Arapov <[email protected]>


> ---
> kernel/events/uprobes.c | 27 ++++++++++++++++-----------
> 1 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4c941fe..98e4d97 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1766,11 +1766,22 @@ handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> up_read(&uprobe->register_rwsem);
> }
>
> +static struct return_instance *find_next_ret_chain(struct return_instance *ri)
> +{
> + bool chained;
> +
> + do {
> + chained = ri->chained;
> + ri = ri->next; /* can't be NULL if chained */
> + } while (chained);
> +
> + return ri;
> +}
> +
> static void handle_trampoline(struct pt_regs *regs)
> {
> struct uprobe_task *utask;
> - struct return_instance *ri;
> - bool chained;
> + struct return_instance *ri, *next;
>
> utask = current->utask;
> if (!utask)
> @@ -1780,24 +1791,18 @@ static void handle_trampoline(struct pt_regs *regs)
> if (!ri)
> goto sigill;
>
> + next = find_next_ret_chain(ri);
> /*
> * TODO: we should throw out return_instance's invalidated by
> * longjmp(), currently we assume that the probed function always
> * returns.
> */
> instruction_pointer_set(regs, ri->orig_ret_vaddr);
> -
> - for (;;) {
> + do {
> handle_uretprobe_chain(ri, regs);
> -
> - chained = ri->chained;
> ri = free_ret_instance(ri);
> utask->depth--;
> -
> - if (!chained)
> - break;
> - BUG_ON(!ri);
> - }
> + } while (ri != next);
>
> utask->return_instances = ri;
> return;
> --
> 1.5.5.1
>

2015-07-07 12:58:51

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 06/11] uprobes: Export struct return_instance, introduce arch_uretprobe_is_alive()

On Tue, Jul 07, 2015 at 03:22:54AM +0200, Oleg Nesterov wrote:
> Add the new "weak" helper, arch_uretprobe_is_alive(), used by the next
> patches. It should return true if this return_instance is still valid.
> The arch agnostic version just always returns true.
>
> The patch exports "struct return_instance" for the architectures which
> want to override this hook. We can also cleanup prepare_uretprobe() if
> we pass the new return_instance to arch_uretprobe_hijack_return_addr().
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Anton Arapov <[email protected]>

> ---
> include/linux/uprobes.h | 10 ++++++++++
> kernel/events/uprobes.c | 14 +++++---------
> 2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 60beb5d..50d2764 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -92,6 +92,15 @@ struct uprobe_task {
> unsigned int depth;
> };
>
> +struct return_instance {
> + struct uprobe *uprobe;
> + unsigned long func;
> + unsigned long orig_ret_vaddr; /* original return address */
> + bool chained; /* true, if instance is nested */
> +
> + struct return_instance *next; /* keep as stack */
> +};
> +
> struct xol_area;
>
> struct uprobes_state {
> @@ -128,6 +137,7 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
> extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
> extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
> extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
> +extern bool arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs);
> extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
> extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
> void *src, unsigned long len);
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 98e4d97..1c71b62 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -86,15 +86,6 @@ struct uprobe {
> struct arch_uprobe arch;
> };
>
> -struct return_instance {
> - struct uprobe *uprobe;
> - unsigned long func;
> - unsigned long orig_ret_vaddr; /* original return address */
> - bool chained; /* true, if instance is nested */
> -
> - struct return_instance *next; /* keep as stack */
> -};
> -
> /*
> * Execute out of line area: anonymous executable mapping installed
> * by the probed task to execute the copy of the original instruction
> @@ -1818,6 +1809,11 @@ bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs)
> return false;
> }
>
> +bool __weak arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs)
> +{
> + return true;
> +}
> +
> /*
> * Run handler and ask thread to singlestep.
> * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
> --
> 1.5.5.1
>

2015-07-07 13:02:59

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 07/11] uprobes/x86: Reimplement arch_uretprobe_is_alive()

On Tue, Jul 07, 2015 at 03:22:58AM +0200, Oleg Nesterov wrote:
> Add the x86-specific version of arch_uretprobe_is_alive() helper.
> It returns true if the stack frame mangled by prepare_uretprobe()
> is still on stack. So if it returns false, we know that the probed
> function has already returned.
>
> We add the new return_instance->stack member and change the generic
> code to initialize it in prepare_uretprobe, but it should be equally
> useful for other architectures.
>
> TODO: this assumes that the probed application can't use multiple
> stacks (say sigaltstack). We will try to improve this logic later.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Anton Arapov <[email protected]>

> ---
> arch/x86/kernel/uprobes.c | 5 +++++
> include/linux/uprobes.h | 1 +
> kernel/events/uprobes.c | 1 +
> 3 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 0b81ad6..9d5f570 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -993,3 +993,8 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs
>
> return -1;
> }
> +
> +bool arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs)
> +{
> + return regs->sp <= ret->stack;
> +}
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 50d2764..7ab6d2c 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -95,6 +95,7 @@ struct uprobe_task {
> struct return_instance {
> struct uprobe *uprobe;
> unsigned long func;
> + unsigned long stack; /* stack pointer */
> unsigned long orig_ret_vaddr; /* original return address */
> bool chained; /* true, if instance is nested */
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 1c71b62..c5f316e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1562,6 +1562,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
>
> ri->uprobe = get_uprobe(uprobe);
> ri->func = instruction_pointer(regs);
> + ri->stack = user_stack_pointer(regs);
> ri->orig_ret_vaddr = orig_ret_vaddr;
> ri->chained = chained;
>
> --
> 1.5.5.1
>

2015-07-07 13:05:10

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 08/11] uprobes: Change handle_trampoline() to flush the frames invalidated by longjmp()

On Tue, Jul 07, 2015 at 03:23:02AM +0200, Oleg Nesterov wrote:
> Test-case:
>
> #include <stdio.h>
> #include <setjmp.h>
>
> jmp_buf jmp;
>
> void func_2(void)
> {
> longjmp(jmp, 1);
> }
>
> void func_1(void)
> {
> if (setjmp(jmp))
> return;
> func_2();
> printf("ERR!! I am running on the caller's stack\n");
> }
>
> int main(void)
> {
> func_1();
> return 0;
> }
>
> fails if you probe func_1() and func_2() because handle_trampoline()
> assumes that the probed function should must return and hit the bp
> installed be prepare_uretprobe(). But in this case func_2() does not
> return, so when func_1() returns the kernel uses the no longer valid
> return_instance of func_2().
>
> Change handle_trampoline() to unwind ->return_instances until we know
> that the next chain is alive or NULL, this ensures that the current
> chain is the last we need to report and free.
>
> Alternatively, every return_instance could use unique trampoline_vaddr,
> in this case we could use it as a key. And this could solve the problem
> with sigaltstack() automatically.
>
> But this approach needs more changes, and it puts the "hard" limit on
> MAX_URETPROBE_DEPTH. Plus it can not solve another problem partially
> fixed by the next patch.
>
> Note: this change has no effect on !x86, the arch-agnostic version of
> arch_uretprobe_is_alive() just returns "true".
>
> TODO: as documented by the previous change, arch_uretprobe_is_alive()
> can be fooled by sigaltstack/etc.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Anton Arapov <[email protected]>

> ---
> kernel/events/uprobes.c | 29 ++++++++++++++++++-----------
> 1 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index c5f316e..93d939c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1774,6 +1774,7 @@ static void handle_trampoline(struct pt_regs *regs)
> {
> struct uprobe_task *utask;
> struct return_instance *ri, *next;
> + bool valid;
>
> utask = current->utask;
> if (!utask)
> @@ -1783,18 +1784,24 @@ static void handle_trampoline(struct pt_regs *regs)
> if (!ri)
> goto sigill;
>
> - next = find_next_ret_chain(ri);
> - /*
> - * TODO: we should throw out return_instance's invalidated by
> - * longjmp(), currently we assume that the probed function always
> - * returns.
> - */
> - instruction_pointer_set(regs, ri->orig_ret_vaddr);
> do {
> - handle_uretprobe_chain(ri, regs);
> - ri = free_ret_instance(ri);
> - utask->depth--;
> - } while (ri != next);
> + /*
> + * We should throw out the frames invalidated by longjmp().
> + * If this chain is valid, then the next one should be alive
> + * or NULL; the latter case means that nobody but ri->func
> + * could hit this trampoline on return. TODO: sigaltstack().
> + */
> + next = find_next_ret_chain(ri);
> + valid = !next || arch_uretprobe_is_alive(next, regs);
> +
> + instruction_pointer_set(regs, ri->orig_ret_vaddr);
> + do {
> + if (valid)
> + handle_uretprobe_chain(ri, regs);
> + ri = free_ret_instance(ri);
> + utask->depth--;
> + } while (ri != next);
> + } while (!valid);
>
> utask->return_instances = ri;
> return;
> --
> 1.5.5.1
>

2015-07-07 13:08:06

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 09/11] uprobes: Change prepare_uretprobe() to (try to) flush the dead frames

On Tue, Jul 07, 2015 at 03:23:06AM +0200, Oleg Nesterov wrote:
> Change prepare_uretprobe() to flush the !arch_uretprobe_is_alive()
> return_instance's. This is not needed correctness-wise, but can help
> to avoid the failure caused by MAX_URETPROBE_DEPTH.
>
> Note: in this case arch_uretprobe_is_alive() can be false positive,
> the stack can grow after longjmp(). Unfortunately, the kernel can't
> 100% solve this problem, but see the next patch.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Anton Arapov <[email protected]>

> ---
> kernel/events/uprobes.c | 13 +++++++++++++
> 1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 93d939c..7e61c8c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1511,6 +1511,16 @@ static unsigned long get_trampoline_vaddr(void)
> return trampoline_vaddr;
> }
>
> +static void cleanup_return_instances(struct uprobe_task *utask, struct pt_regs *regs)
> +{
> + struct return_instance *ri = utask->return_instances;
> + while (ri && !arch_uretprobe_is_alive(ri, regs)) {
> + ri = free_ret_instance(ri);
> + utask->depth--;
> + }
> + utask->return_instances = ri;
> +}
> +
> static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> {
> struct return_instance *ri;
> @@ -1541,6 +1551,9 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> if (orig_ret_vaddr == -1)
> goto fail;
>
> + /* drop the entries invalidated by longjmp() */
> + cleanup_return_instances(utask, regs);
> +
> /*
> * We don't want to keep trampoline address in stack, rather keep the
> * original return address of first caller thru all the consequent
> --
> 1.5.5.1
>

2015-07-07 13:09:08

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 10/11] uprobes: Add the "enum rp_check ctx" arg to arch_uretprobe_is_alive()

On Tue, Jul 07, 2015 at 03:23:10AM +0200, Oleg Nesterov wrote:
> arch/x86 doesn't care (so far), but as Pratyush Anand pointed out
> other architectures might want why arch_uretprobe_is_alive() was
> called and use different checks depending on the context. Add the
> new argument to distinguish 2 callers.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Anton Arapov <[email protected]>

> ---
> arch/x86/kernel/uprobes.c | 3 ++-
> include/linux/uprobes.h | 7 ++++++-
> kernel/events/uprobes.c | 9 ++++++---
> 3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 9d5f570..67eb168 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -994,7 +994,8 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs
> return -1;
> }
>
> -bool arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs)
> +bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check ctx,
> + struct pt_regs *regs)
> {
> return regs->sp <= ret->stack;
> }
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 7ab6d2c..c0a5402 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -102,6 +102,11 @@ struct return_instance {
> struct return_instance *next; /* keep as stack */
> };
>
> +enum rp_check {
> + RP_CHECK_CALL,
> + RP_CHECK_RET,
> +};
> +
> struct xol_area;
>
> struct uprobes_state {
> @@ -138,7 +143,7 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
> extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
> extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
> extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
> -extern bool arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs);
> +extern bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check ctx, struct pt_regs *regs);
> extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
> extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
> void *src, unsigned long len);
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 7e61c8c..df5661a 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1514,7 +1514,9 @@ static unsigned long get_trampoline_vaddr(void)
> static void cleanup_return_instances(struct uprobe_task *utask, struct pt_regs *regs)
> {
> struct return_instance *ri = utask->return_instances;
> - while (ri && !arch_uretprobe_is_alive(ri, regs)) {
> + enum rp_check ctx = RP_CHECK_CALL;
> +
> + while (ri && !arch_uretprobe_is_alive(ri, ctx, regs)) {
> ri = free_ret_instance(ri);
> utask->depth--;
> }
> @@ -1805,7 +1807,7 @@ static void handle_trampoline(struct pt_regs *regs)
> * could hit this trampoline on return. TODO: sigaltstack().
> */
> next = find_next_ret_chain(ri);
> - valid = !next || arch_uretprobe_is_alive(next, regs);
> + valid = !next || arch_uretprobe_is_alive(next, RP_CHECK_RET, regs);
>
> instruction_pointer_set(regs, ri->orig_ret_vaddr);
> do {
> @@ -1830,7 +1832,8 @@ bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs)
> return false;
> }
>
> -bool __weak arch_uretprobe_is_alive(struct return_instance *ret, struct pt_regs *regs)
> +bool __weak arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check ctx,
> + struct pt_regs *regs)
> {
> return true;
> }
> --
> 1.5.5.1
>

2015-07-07 13:12:14

by Anton Arapov

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] uprobes/x86: Make arch_uretprobe_is_alive(RP_CHECK_CALL) more clever

On Tue, Jul 07, 2015 at 03:23:13AM +0200, Oleg Nesterov wrote:
> The previous change documents that cleanup_return_instances() can't
> always detect the dead frames, the stack can grow. But there is one
> special case which imho worth fixing: arch_uretprobe_is_alive() can
> return true when the stack didn't actually grow, but the next "call"
> insn uses the already invalidated frame.
>
> Test-case:
>
> #include <stdio.h>
> #include <setjmp.h>
>
> jmp_buf jmp;
> int nr = 1024;
>
> void func_2(void)
> {
> if (--nr == 0)
> return;
> longjmp(jmp, 1);
> }
>
> void func_1(void)
> {
> setjmp(jmp);
> func_2();
> }
>
> int main(void)
> {
> func_1();
> return 0;
> }
>
> If you ret-probe func_1() and func_2() prepare_uretprobe() hits the
> MAX_URETPROBE_DEPTH limit and "return" from func_2() is not reported.
>
> When we know that the new call is not chained, we can do the more
> strict check. In this case "sp" points to the new ret-addr, so every
> frame which uses the same "sp" must be dead. The only complication is
> that arch_uretprobe_is_alive() needs to know was it chained or not, so
> we add the new RP_CHECK_CHAIN_CALL enum and change prepare_uretprobe()
> to pass RP_CHECK_CALL only if !chained.
>
> Note: arch_uretprobe_is_alive() could also re-read *sp and check if
> this word is still trampoline_vaddr. This could obviously improve the
> logic, but I would like to avoid another copy_from_user() especially
> in the case when we can't avoid the false "alive == T" positives.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Anton Arapov <[email protected]>


> ---
> arch/x86/kernel/uprobes.c | 5 ++++-
> include/linux/uprobes.h | 1 +
> kernel/events/uprobes.c | 14 +++++++-------
> 3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 67eb168..a5c59f2 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -997,5 +997,8 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs
> bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check ctx,
> struct pt_regs *regs)
> {
> - return regs->sp <= ret->stack;
> + if (ctx == RP_CHECK_CALL) /* sp was just decremented by "call" insn */
> + return regs->sp < ret->stack;
> + else
> + return regs->sp <= ret->stack;
> }
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index c0a5402..0bdc72f 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -104,6 +104,7 @@ struct return_instance {
>
> enum rp_check {
> RP_CHECK_CALL,
> + RP_CHECK_CHAIN_CALL,
> RP_CHECK_RET,
> };
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index df5661a..0f370ef 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1511,10 +1511,11 @@ static unsigned long get_trampoline_vaddr(void)
> return trampoline_vaddr;
> }
>
> -static void cleanup_return_instances(struct uprobe_task *utask, struct pt_regs *regs)
> +static void cleanup_return_instances(struct uprobe_task *utask, bool chained,
> + struct pt_regs *regs)
> {
> struct return_instance *ri = utask->return_instances;
> - enum rp_check ctx = RP_CHECK_CALL;
> + enum rp_check ctx = chained ? RP_CHECK_CHAIN_CALL : RP_CHECK_CALL;
>
> while (ri && !arch_uretprobe_is_alive(ri, ctx, regs)) {
> ri = free_ret_instance(ri);
> @@ -1528,7 +1529,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> struct return_instance *ri;
> struct uprobe_task *utask;
> unsigned long orig_ret_vaddr, trampoline_vaddr;
> - bool chained = false;
> + bool chained;
>
> if (!get_xol_area())
> return;
> @@ -1554,14 +1555,15 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> goto fail;
>
> /* drop the entries invalidated by longjmp() */
> - cleanup_return_instances(utask, regs);
> + chained = (orig_ret_vaddr == trampoline_vaddr);
> + cleanup_return_instances(utask, chained, regs);
>
> /*
> * We don't want to keep trampoline address in stack, rather keep the
> * original return address of first caller thru all the consequent
> * instances. This also makes breakpoint unwrapping easier.
> */
> - if (orig_ret_vaddr == trampoline_vaddr) {
> + if (chained) {
> if (!utask->return_instances) {
> /*
> * This situation is not possible. Likely we have an
> @@ -1570,8 +1572,6 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> uprobe_warn(current, "handle tail call");
> goto fail;
> }
> -
> - chained = true;
> orig_ret_vaddr = utask->return_instances->orig_ret_vaddr;
> }
>
> --
> 1.5.5.1
>

2015-07-10 11:53:25

by Srikar Dronamraju

[permalink] [raw]
Subject: Re: [PATCH v2 06/11] uprobes: Export struct return_instance, introduce arch_uretprobe_is_alive()

* Oleg Nesterov <[email protected]> [2015-07-07 03:22:54]:

> Add the new "weak" helper, arch_uretprobe_is_alive(), used by the next
> patches. It should return true if this return_instance is still valid.
> The arch agnostic version just always returns true.
>
> The patch exports "struct return_instance" for the architectures which
> want to override this hook. We can also cleanup prepare_uretprobe() if
> we pass the new return_instance to arch_uretprobe_hijack_return_addr().
>
> Signed-off-by: Oleg Nesterov <[email protected]>

Looks good to me.

Acked-by: Srikar Dronamraju <[email protected]>

--
Thanks and Regards
Srikar Dronamraju

2015-07-10 11:53:50

by Srikar Dronamraju

[permalink] [raw]
Subject: Re: [PATCH v2 07/11] uprobes/x86: Reimplement arch_uretprobe_is_alive()

* Oleg Nesterov <[email protected]> [2015-07-07 03:22:58]:

> Add the x86-specific version of arch_uretprobe_is_alive() helper.
> It returns true if the stack frame mangled by prepare_uretprobe()
> is still on stack. So if it returns false, we know that the probed
> function has already returned.
>
> We add the new return_instance->stack member and change the generic
> code to initialize it in prepare_uretprobe, but it should be equally
> useful for other architectures.
>
> TODO: this assumes that the probed application can't use multiple
> stacks (say sigaltstack). We will try to improve this logic later.
>
> Signed-off-by: Oleg Nesterov <[email protected]>


Looks good to me.

Acked-by: Srikar Dronamraju <[email protected]>
--
Thanks and Regards
Srikar Dronamraju

2015-07-10 11:56:57

by Srikar Dronamraju

[permalink] [raw]
Subject: Re: [PATCH v2 08/11] uprobes: Change handle_trampoline() to flush the frames invalidated by longjmp()

> Change handle_trampoline() to unwind ->return_instances until we know
> that the next chain is alive or NULL, this ensures that the current
> chain is the last we need to report and free.
>
> Alternatively, every return_instance could use unique trampoline_vaddr,
> in this case we could use it as a key. And this could solve the problem
> with sigaltstack() automatically.
>
> But this approach needs more changes, and it puts the "hard" limit on
> MAX_URETPROBE_DEPTH. Plus it can not solve another problem partially
> fixed by the next patch.
>
> Note: this change has no effect on !x86, the arch-agnostic version of
> arch_uretprobe_is_alive() just returns "true".
>
> TODO: as documented by the previous change, arch_uretprobe_is_alive()
> can be fooled by sigaltstack/etc.
>
> Signed-off-by: Oleg Nesterov <[email protected]>

Looks good to me.

Acked-by: Srikar Dronamraju <[email protected]>

--
Thanks and Regards
Srikar Dronamraju

2015-07-10 11:57:29

by Srikar Dronamraju

[permalink] [raw]
Subject: Re: [PATCH v2 09/11] uprobes: Change prepare_uretprobe() to (try to) flush the dead frames

* Oleg Nesterov <[email protected]> [2015-07-07 03:23:06]:

> Change prepare_uretprobe() to flush the !arch_uretprobe_is_alive()
> return_instance's. This is not needed correctness-wise, but can help
> to avoid the failure caused by MAX_URETPROBE_DEPTH.
>
> Note: in this case arch_uretprobe_is_alive() can be false positive,
> the stack can grow after longjmp(). Unfortunately, the kernel can't
> 100% solve this problem, but see the next patch.
>
> Signed-off-by: Oleg Nesterov <[email protected]>

Looks good to me.

Acked-by: Srikar Dronamraju <[email protected]>

--
Thanks and Regards
Srikar Dronamraju

2015-07-10 12:01:15

by Pratyush Anand

[permalink] [raw]
Subject: Re: [PATCH v2 00/11] uprobes: longjmp fixes

Hi Oleg,

Thanks a lot for the patches.
I did corresponding changes [1] for arch_uretprobe_is_alive on top of
my ARM64 RFC V2 [2] + these patches and found that longjump tests are working.

So for the series's common code

Tested-by: Pratyush Anand <[email protected]>

~Pratyush

[1] https://github.com/pratyushanand/linux/commit/880df93e2dac48f620069fffb16d551e96681774
[2] http://thread.gmane.org/gmane.linux.kernel/1979016

On 07/07/2015:03:22:10 AM, Oleg Nesterov wrote:
> Sorry for delay,
>
> Currently ret-probes can't work (the application will likely crash)
> if the probed function does not return, and this is even documented
> in handle_trampoline().
>
> This series tries to make the first step to fix the problem, assuming
> that the probed functions use the same stack.
>
> TODO: sigaltstack() can obviously break this assumption.
>
> NOTE: I don't think it is possible to make this logic 100% correct,
> the user-space can do everything with its stack. For example, the
> application can do longjmp-like tricks to implement the coroutines,
> the kernel can do nothing in this case. The application (or debugger)
> should cooperate somehow to let the kernel know whats going on.
>
> v2, based on disccsussion with Srikar and Pratyush:
>
> 1-5: Unchanged, I preserved the acks from Srikar.
>
> 6-11: The only essential change is that we do not add the
> (ugly) arch_uretprobe, we just export return_instance
> to arch/.
>
> This means that we do not need to touch the !x86 code,
> and return_instance->stack can be initialized by the
> generic code.
>
> Srikar, I hope you can ack v2 too.
>
> 10/11: New. As Pratyush pointed out "bool on_call" is too
> limited.
>
> Plus v2 fixes the problem mentioned in "self nack" email, we must
> not do cleanup_return_instances() after prepare_uretprobe() checks
> chained && utask->return_instances != NULL.
>
> Oleg.
>
> arch/x86/kernel/uprobes.c | 9 ++
> include/linux/uprobes.h | 17 ++++
> kernel/events/uprobes.c | 184 +++++++++++++++++++++++++--------------------
> 3 files changed, 128 insertions(+), 82 deletions(-)

2015-07-10 12:08:49

by Srikar Dronamraju

[permalink] [raw]
Subject: Re: [PATCH v2 10/11] uprobes: Add the "enum rp_check ctx" arg to arch_uretprobe_is_alive()

* Oleg Nesterov <[email protected]> [2015-07-07 03:23:10]:

> arch/x86 doesn't care (so far), but as Pratyush Anand pointed out
> other architectures might want why arch_uretprobe_is_alive() was
> called and use different checks depending on the context. Add the
> new argument to distinguish 2 callers.
>
> Signed-off-by: Oleg Nesterov <[email protected]>

Looks good to me.

Acked-by: Srikar Dronamraju <[email protected]>
--
Thanks and Regards
Srikar Dronamraju

2015-07-10 12:08:26

by Srikar Dronamraju

[permalink] [raw]
Subject: Re: [PATCH v2 11/11] uprobes/x86: Make arch_uretprobe_is_alive(RP_CHECK_CALL) more clever

> If you ret-probe func_1() and func_2() prepare_uretprobe() hits the
> MAX_URETPROBE_DEPTH limit and "return" from func_2() is not reported.
>
> When we know that the new call is not chained, we can do the more
> strict check. In this case "sp" points to the new ret-addr, so every
> frame which uses the same "sp" must be dead. The only complication is
> that arch_uretprobe_is_alive() needs to know was it chained or not, so
> we add the new RP_CHECK_CHAIN_CALL enum and change prepare_uretprobe()
> to pass RP_CHECK_CALL only if !chained.
>
> Note: arch_uretprobe_is_alive() could also re-read *sp and check if
> this word is still trampoline_vaddr. This could obviously improve the
> logic, but I would like to avoid another copy_from_user() especially
> in the case when we can't avoid the false "alive == T" positives.
>
> Signed-off-by: Oleg Nesterov <[email protected]>

Looks good to me.

Acked-by: Srikar Dronamraju <[email protected]>

--
Thanks and Regards
Srikar Dronamraju