Hello,
RFC v4 uretprobes implementation. I'd be grateful for review.
/* Oleg, this one is more quirky than previous, don't beat me. */
These patches extending uprobes by enabling tools, such as perf(trace_event),
set a breakpoint on probed function's return address.
v4:
- get rid of area->rp_trampoline_vaddr as it always the same as ->vaddr
- preallocate slot, as the first one in xol_add_vma()
- cleanup ->return_uprobes list in uprobe_free_utask(), because the
task can exit from inside the ret-probe'd function(s).
- in find_active_uprobe(): Once we inserted "int3" we must ensure that
handle_swbp() will be called even if this uprobe goes away. We have
the reference but it only protects uprobe itself, it can't protect
agains delete_uprobe().
IOW, we must ensure that uprobe_pre_sstep_notifier() can't return 0.
- check, whether utask is not NULL in handle_uretprobe()
? do we want a printk() for this case?
- minor handle_uretprobe() fixups
v3 changes:
- removed uretprobe_bypass logic, it will be better to send it as
independent patch
- unified xol_get_trampoline_slot() and xol_get_insn_slot()
- protected uprobe with refcounter in prepare_uretprobe()
- uprobe_register() routine fails now, if neither consumer is set
- enclosed implementation into 1/6, 6/6 patches -ENOSYS bits
v2 changes:
- introduced rp_handler(), get rid of return_consumers
- get rid of uretprobe_[un]register()
- introduced arch_uretprobe_get_sp()
- removed uprobe_task->doomed, kill task immediately
- fix arch_uretprobe_hijack_return_addr()'s returns
- address the v1 minor issues
integrated patchset:
http://github.com/arapov/linux-aa/commits/uretprobes_v3
previous implementations:
RFCv3: https://lkml.org/lkml/2013/2/28/148
RFCv2: https://lkml.org/lkml/2013/1/9/157
RFCv1: https://lkml.org/lkml/2012/12/21/133
thanks,
Anton
Anton Arapov (6):
uretprobes: preparation patch
uretprobes/x86: hijack return address
uretprobes: generalize xol_get_insn_slot()
uretprobes: return probe entry, prepare uretprobe
uretprobes: invoke return probe handlers
uretprobes: implemented, thus remove -ENOSYS
arch/x86/include/asm/uprobes.h | 6 ++
arch/x86/kernel/uprobes.c | 29 +++++++++
include/linux/uprobes.h | 5 ++
kernel/events/uprobes.c | 134 ++++++++++++++++++++++++++++++++++++++---
4 files changed, 166 insertions(+), 8 deletions(-)
--
1.8.1.2
hijack the return address and replace it with a "trampoline"
v2:
- remove ->doomed flag, kill task immediately
Signed-off-by: Anton Arapov <[email protected]>
---
arch/x86/include/asm/uprobes.h | 1 +
arch/x86/kernel/uprobes.c | 29 +++++++++++++++++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 8ff8be7..c353555 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -55,4 +55,5 @@ extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
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 rp_trampoline_vaddr, struct pt_regs *regs);
#endif /* _ASM_UPROBES_H */
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 0ba4cfb..85e2153 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -697,3 +697,32 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
send_sig(SIGTRAP, current, 0);
return ret;
}
+
+extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
+ rp_trampoline_vaddr, struct pt_regs *regs)
+{
+ int rasize, ncopied;
+ unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
+
+ rasize = is_ia32_task() ? 4 : 8;
+ ncopied = copy_from_user(&orig_ret_vaddr, (void __user *)regs->sp, rasize);
+ if (unlikely(ncopied))
+ return 0;
+
+ /* check whether address has been already hijacked */
+ if (orig_ret_vaddr == rp_trampoline_vaddr)
+ return orig_ret_vaddr;
+
+ ncopied = copy_to_user((void __user *)regs->sp, &rp_trampoline_vaddr, rasize);
+ if (unlikely(ncopied)) {
+ if (ncopied != rasize) {
+ printk(KERN_ERR "uretprobe: return address clobbered: "
+ "pid=%d, %%sp=%#lx, %%ip=%#lx\n",
+ current->pid, regs->sp, regs->ip);
+ /* kill task immediately */
+ send_sig(SIGSEGV, current, 0);
+ }
+ }
+
+ return orig_ret_vaddr;
+}
--
1.8.1.2
Generalize xol_take_insn_slot() to enable more consumers of the
function, e.g. trampoline implementation for return probes.
The first time a uprobe with return consumer is hit for a process, a
trampoline slot is obtained in the xol_area and initialized with a
breakpoint instruction. This slot is subsequently used by all
uretprobes.
v3:
- unified xol_get_insn_slot(), thus get rid of xol_get_trampoline_slot()
Signed-off-by: Anton Arapov <[email protected]>
---
kernel/events/uprobes.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index d904164..69bf060 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1221,7 +1221,7 @@ static unsigned long xol_take_insn_slot(struct xol_area *area)
* xol_get_insn_slot - allocate a slot for xol.
* Returns the allocated slot address or 0.
*/
-static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
+static unsigned long xol_get_insn_slot(unsigned char *insn)
{
struct xol_area *area;
unsigned long offset;
@@ -1239,7 +1239,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
/* Initialize the slot */
offset = xol_vaddr & ~PAGE_MASK;
vaddr = kmap_atomic(area->page);
- memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
+ memcpy(vaddr + offset, insn, MAX_UINSN_BYTES);
kunmap_atomic(vaddr);
/*
* We probably need flush_icache_user_range() but it needs vma.
@@ -1353,7 +1353,7 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
if (!utask)
return -ENOMEM;
- xol_vaddr = xol_get_insn_slot(uprobe);
+ xol_vaddr = xol_get_insn_slot(uprobe->arch.insn);
if (!xol_vaddr)
return -ENOMEM;
--
1.8.1.2
Uretprobe handlers are invoked when the trampoline is hit, on completion the
trampoline is replaced with the saved return address and the uretprobe instance
deleted.
v4:
- check, whether utask is not NULL in handle_uretprobe()
? do we want a printk() for this case?
- get rid of area->rp_trampoline_vaddr
- minor handle_uretprobe() fixups
v3:
- protected uprobe with refcounter. See put_uprobe() in handle_uretprobe()
that reflects increment in prepare_uretprobe()
v2:
- get rid of ->return_consumers member from struct uprobe, introduce
rp_handler() in consumer instead
Signed-off-by: Anton Arapov <[email protected]>
---
arch/x86/include/asm/uprobes.h | 5 ++++
kernel/events/uprobes.c | 57 ++++++++++++++++++++++++++++++++++++++++--
2 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index c353555..fa9d9de 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -56,4 +56,9 @@ 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 rp_trampoline_vaddr, struct pt_regs *regs);
+
+static inline unsigned long arch_uretprobe_get_sp(struct pt_regs *regs)
+{
+ return (unsigned long)regs->sp;
+}
#endif /* _ASM_UPROBES_H */
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 12acc10..e86b6ea 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1574,6 +1574,55 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
up_read(&uprobe->register_rwsem);
}
+static void handler_uretprobe_chain(struct uprobe *uprobe, struct pt_regs *regs)
+{
+ struct uprobe_consumer *uc;
+
+ down_read(&uprobe->register_rwsem);
+ for (uc = uprobe->consumers; uc; uc = uc->next) {
+ if (uc->rp_handler)
+ uc->rp_handler(uc, regs);
+ }
+ up_read(&uprobe->register_rwsem);
+}
+
+static void handle_uretprobe(struct xol_area *area, struct pt_regs *regs)
+{
+ struct hlist_head *head;
+ struct hlist_node *tmp;
+ struct return_uprobe_i *ri;
+ struct uprobe_task *utask;
+ unsigned long orig_ret_vaddr;
+
+ /* TODO: uretprobe bypass logic */
+
+ utask = get_utask();
+ if (!utask) {
+ /* TODO:RFC task is not probed, do we want printk here? */
+ return;
+ }
+ head = &utask->return_uprobes;
+ hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+ if (ri->uprobe->consumers) {
+ instruction_pointer_set(regs, ri->orig_ret_vaddr);
+ handler_uretprobe_chain(ri->uprobe, regs);
+ }
+
+ orig_ret_vaddr = ri->orig_ret_vaddr;
+ put_uprobe(ri->uprobe);
+ hlist_del(&ri->hlist);
+ kfree(ri);
+
+ if (orig_ret_vaddr != area->vaddr)
+ return;
+ }
+
+ /* TODO: change the message */
+ printk(KERN_ERR "uprobe: no instance found! pid/tgid=%d/%d",
+ current->pid, current->tgid);
+ send_sig(SIGSEGV, current, 0);
+}
+
/*
* Run handler and ask thread to singlestep.
* Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -1581,6 +1630,7 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
static void handle_swbp(struct pt_regs *regs)
{
struct uprobe *uprobe;
+ struct xol_area *area;
unsigned long bp_vaddr;
int uninitialized_var(is_swbp);
@@ -1589,8 +1639,11 @@ static void handle_swbp(struct pt_regs *regs)
if (!uprobe) {
if (is_swbp > 0) {
- /* No matching uprobe; signal SIGTRAP. */
- send_sig(SIGTRAP, current, 0);
+ area = get_xol_area();
+ if (area && bp_vaddr == area->vaddr)
+ handle_uretprobe(area, regs);
+ else
+ send_sig(SIGTRAP, current, 0);
} else {
/*
* Either we raced with uprobe_unregister() or we can't
--
1.8.1.2
1/6 and 6/6 patches are here to enclose return probes implementation
as well as prohibit uprobe_register() routine with no consumer set.
v3 changes: (the patch is introduced in v3)
- remove 'TODO' as return probes implemented now
Signed-off-by: Anton Arapov <[email protected]>
---
kernel/events/uprobes.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e86b6ea..b9c51dc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -840,10 +840,6 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
if (!uc->handler && !uc->rp_handler)
return -EINVAL;
- /* TODO: Implement return probes */
- if (uc->rp_handler)
- return -ENOSYS;
-
/* Racy, just to catch the obvious mistakes */
if (offset > i_size_read(inode))
return -EINVAL;
--
1.8.1.2
1/6 and 6/6 patches are here to enclose return probes implementation as well
as prohibit uprobe_register() routine with no consumer set.
v3 changes: (the patch is introduced in v3)
- check whether at least one of the consumer's handlers were set
- a 'TODO' cap that will be removed once return probes be implemented
Signed-off-by: Anton Arapov <[email protected]>
---
include/linux/uprobes.h | 1 +
kernel/events/uprobes.c | 8 ++++++++
2 files changed, 9 insertions(+)
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 02b83db..a28bdee 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -46,6 +46,7 @@ enum uprobe_filter_ctx {
struct uprobe_consumer {
int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
+ int (*rp_handler)(struct uprobe_consumer *self, struct pt_regs *regs);
bool (*filter)(struct uprobe_consumer *self,
enum uprobe_filter_ctx ctx,
struct mm_struct *mm);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a567c8c..d904164 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -828,6 +828,14 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
struct uprobe *uprobe;
int ret;
+ /* Uprobe must have at least one set consumer */
+ if (!uc->handler && !uc->rp_handler)
+ return -EINVAL;
+
+ /* TODO: Implement return probes */
+ if (uc->rp_handler)
+ return -ENOSYS;
+
/* Racy, just to catch the obvious mistakes */
if (offset > i_size_read(inode))
return -EINVAL;
--
1.8.1.2
When a uprobe with return consumer is hit, prepare_uretprobe function is
invoked. It creates return_instance, hijacks return address and replaces
it with the trampoline.
N.B. it might be a good idea to introduce get_uprobe() to reflect
put_uprobe() later, but it is not a subject of this patchset.
v4:
- get rid of area->rp_trampoline_vaddr as it always the same as ->vaddr
- preallocate slot, as the first one in xol_add_vma()
- cleanup ->return_uprobes list in uprobe_free_utask(), because the
task can exit from inside the ret-probe'd function(s).
- in find_active_uprobe(): Once we inserted "int3" we must ensure that
handle_swbp() will be called even if this uprobe goes away. We have
the reference but it only protects uprobe itself, it can't protect
agains delete_uprobe().
IOW, we must ensure that uprobe_pre_sstep_notifier() can't return 0.
v3:
- protected uprobe with refcounter. See atomic_inc in prepare_uretprobe()
and put_uprobe() in a following patch in handle_uretprobe()
v2:
- get rid of ->return_consumers member from struct uprobe, introduce
rp_handler() in consumer
Signed-off-by: Anton Arapov <[email protected]>
---
include/linux/uprobes.h | 4 +++
kernel/events/uprobes.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index a28bdee..6b0a309 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -69,6 +69,10 @@ struct uprobe_task {
enum uprobe_task_state state;
struct arch_uprobe_task autask;
+ /*
+ * list for tracking uprobes with return consumers
+ */
+ struct hlist_head return_uprobes;
struct uprobe *active_uprobe;
unsigned long xol_vaddr;
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 69bf060..12acc10 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -57,6 +57,8 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
static struct percpu_rw_semaphore dup_mmap_sem;
+static unsigned long xol_get_insn_slot(unsigned char *insn);
+
/* Have a copy of original instruction */
#define UPROBE_COPY_INSN 0
/* Can skip singlestep */
@@ -75,6 +77,12 @@ struct uprobe {
struct arch_uprobe arch;
};
+struct return_uprobe_i {
+ struct uprobe *uprobe;
+ struct hlist_node hlist; /* node in list */
+ unsigned long orig_ret_vaddr; /* original return address */
+};
+
/*
* valid_vma: Verify if the specified vma is an executable vma
* Relax restrictions while unregistering: vm_flags might have
@@ -1085,6 +1093,7 @@ static int xol_add_vma(struct xol_area *area)
{
struct mm_struct *mm = current->mm;
int ret = -EALREADY;
+ uprobe_opcode_t insn = UPROBE_SWBP_INSN;
down_write(&mm->mmap_sem);
if (mm->uprobes_state.xol_area)
@@ -1106,6 +1115,13 @@ static int xol_add_vma(struct xol_area *area)
smp_wmb(); /* pairs with get_xol_area() */
mm->uprobes_state.xol_area = area;
ret = 0;
+
+ /*
+ * If we reached this place, we did allocate a new area. We want
+ * pre-alloc a slot for the return probes here.
+ */
+ xol_get_insn_slot(&insn);
+
fail:
up_write(&mm->mmap_sem);
@@ -1306,6 +1322,9 @@ unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs)
void uprobe_free_utask(struct task_struct *t)
{
struct uprobe_task *utask = t->utask;
+ struct hlist_head *head;
+ struct hlist_node *tmp;
+ struct return_uprobe_i *ri;
if (!utask)
return;
@@ -1313,6 +1332,13 @@ void uprobe_free_utask(struct task_struct *t)
if (utask->active_uprobe)
put_uprobe(utask->active_uprobe);
+ head = &utask->return_uprobes;
+ hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+ put_uprobe(ri->uprobe);
+ hlist_del(&ri->hlist);
+ kfree(ri);
+ }
+
xol_free_insn_slot(t);
kfree(utask);
t->utask = NULL;
@@ -1336,11 +1362,37 @@ void uprobe_copy_process(struct task_struct *t)
*/
static struct uprobe_task *get_utask(void)
{
- if (!current->utask)
+ if (!current->utask) {
current->utask = kzalloc(sizeof(struct uprobe_task), GFP_KERNEL);
+ if (current->utask)
+ INIT_HLIST_HEAD(¤t->utask->return_uprobes);
+ }
return current->utask;
}
+static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
+{
+ struct return_uprobe_i *ri;
+ struct uprobe_task *utask;
+ struct xol_area *area;
+
+ ri = kzalloc(sizeof(struct return_uprobe_i), GFP_KERNEL);
+ if (!ri)
+ return;
+
+ area = get_xol_area();
+ utask = get_utask();
+ ri->orig_ret_vaddr = arch_uretprobe_hijack_return_addr(area->vaddr, regs);
+ if (likely(ri->orig_ret_vaddr)) {
+ /* TODO: uretprobe bypass logic */
+ atomic_inc(&uprobe->ref);
+ ri->uprobe = uprobe;
+ INIT_HLIST_NODE(&ri->hlist);
+ hlist_add_head(&ri->hlist, &utask->return_uprobes);
+ } else
+ kfree(ri);
+}
+
/* Prepare to single-step probed instruction out of line. */
static int
pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
@@ -1468,6 +1520,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
struct mm_struct *mm = current->mm;
struct uprobe *uprobe = NULL;
struct vm_area_struct *vma;
+ struct uprobe_task *utask;
down_read(&mm->mmap_sem);
vma = find_vma(mm, bp_vaddr);
@@ -1485,8 +1538,11 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
*is_swbp = -EFAULT;
}
- if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
+ utask = get_utask();
+ if (!uprobe && hlist_empty(&utask->return_uprobes) &&
+ test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags)) {
mmf_recalc_uprobes(mm);
+ }
up_read(&mm->mmap_sem);
return uprobe;
@@ -1494,12 +1550,17 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
{
+ int rc = 0;
struct uprobe_consumer *uc;
int remove = UPROBE_HANDLER_REMOVE;
down_read(&uprobe->register_rwsem);
for (uc = uprobe->consumers; uc; uc = uc->next) {
- int rc = uc->handler(uc, regs);
+ if (uc->handler)
+ rc = uc->handler(uc, regs);
+
+ if (uc->rp_handler)
+ prepare_uretprobe(uprobe, regs); /* put bp at return */
WARN(rc & ~UPROBE_HANDLER_MASK,
"bad rc=0x%x from %pf()\n", rc, uc->handler);
--
1.8.1.2
On 03/04, Anton Arapov wrote:
>
> @@ -1085,6 +1093,7 @@ static int xol_add_vma(struct xol_area *area)
> {
> struct mm_struct *mm = current->mm;
> int ret = -EALREADY;
> + uprobe_opcode_t insn = UPROBE_SWBP_INSN;
>
> down_write(&mm->mmap_sem);
> if (mm->uprobes_state.xol_area)
> @@ -1106,6 +1115,13 @@ static int xol_add_vma(struct xol_area *area)
> smp_wmb(); /* pairs with get_xol_area() */
> mm->uprobes_state.xol_area = area;
> ret = 0;
> +
> + /*
> + * If we reached this place, we did allocate a new area. We want
> + * pre-alloc a slot for the return probes here.
> + */
> + xol_get_insn_slot(&insn);
Just change get_xol_area() to do set_bit(0, bitmap) and copy_to_page(page, int3)
(extacted from xol_get_insn_slot().
> @@ -1485,8 +1538,11 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> *is_swbp = -EFAULT;
> }
>
> - if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
> + utask = get_utask();
> + if (!uprobe && hlist_empty(&utask->return_uprobes) &&
> + test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags)) {
> mmf_recalc_uprobes(mm);
Wait, I was wrong. We should not clear MMF_* if another thread has
->return_uprobes. Perhaps we should change uprobe_pre_sstep_notifier()
instead...
> down_read(&uprobe->register_rwsem);
> for (uc = uprobe->consumers; uc; uc = uc->next) {
> - int rc = uc->handler(uc, regs);
> + if (uc->handler)
> + rc = uc->handler(uc, regs);
> +
> + if (uc->rp_handler)
> + prepare_uretprobe(uprobe, regs); /* put bp at return */
Again, this is not right. We should do this only once after the main
loop.
Oleg.
On 03/04, Anton Arapov wrote:
>
> +static void handle_uretprobe(struct xol_area *area, struct pt_regs *regs)
> +{
> + struct hlist_head *head;
> + struct hlist_node *tmp;
> + struct return_uprobe_i *ri;
> + struct uprobe_task *utask;
> + unsigned long orig_ret_vaddr;
> +
> + /* TODO: uretprobe bypass logic */
> +
> + utask = get_utask();
> + if (!utask) {
> + /* TODO:RFC task is not probed, do we want printk here? */
> + return;
> + }
> + head = &utask->return_uprobes;
> + hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> + if (ri->uprobe->consumers) {
> + instruction_pointer_set(regs, ri->orig_ret_vaddr);
This doesn't look right if ri->orig_ret_vaddr == area->vaddr. We should
splice the list and find orig_ret_vaddr in advance.
> @@ -1589,8 +1639,11 @@ static void handle_swbp(struct pt_regs *regs)
>
> if (!uprobe) {
> if (is_swbp > 0) {
> - /* No matching uprobe; signal SIGTRAP. */
> - send_sig(SIGTRAP, current, 0);
> + area = get_xol_area();
> + if (area && bp_vaddr == area->vaddr)
> + handle_uretprobe(area, regs);
> + else
> + send_sig(SIGTRAP, current, 0);
Why? We can check bp_vaddr at the start, before find_active_uprobe().
And I'd suggest to not use area->vaddr directly, imho a trivial helper
makes sense.
Oleg.
On Mon, Mar 04, 2013 at 03:38:12PM +0100, Anton Arapov wrote:
>
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index c353555..fa9d9de 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -56,4 +56,9 @@ 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 rp_trampoline_vaddr, struct pt_regs *regs);
> +
> +static inline unsigned long arch_uretprobe_get_sp(struct pt_regs *regs)
> +{
> + return (unsigned long)regs->sp;
> +}
You could use GET_USP() here.
* Anton Arapov <[email protected]> wrote:
> Hello,
>
> RFC v4 uretprobes implementation. I'd be grateful for review.
> /* Oleg, this one is more quirky than previous, don't beat me. */
Please include a good high level description of the new feature in one of the
patches - preferably also stick it somewhere into Documentation/.
Thanks,
Ingo
On Tue, Mar 05, 2013 at 01:04:17PM +0100, Ingo Molnar wrote:
>
> * Anton Arapov <[email protected]> wrote:
>
> > Hello,
> >
> > RFC v4 uretprobes implementation. I'd be grateful for review.
> > /* Oleg, this one is more quirky than previous, don't beat me. */
>
> Please include a good high level description of the new feature in one of the
> patches - preferably also stick it somewhere into Documentation/.
Yes, I apologize, I did wait for someone's complaint to push myself to
write meaningful description.
thanks,
Anton
> Thanks,
>
> Ingo
On Tue, Mar 05, 2013 at 12:33:26PM +0530, Ananth N Mavinakayanahalli wrote:
> On Mon, Mar 04, 2013 at 03:38:12PM +0100, Anton Arapov wrote:
> >
> > diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> > index c353555..fa9d9de 100644
> > --- a/arch/x86/include/asm/uprobes.h
> > +++ b/arch/x86/include/asm/uprobes.h
> > @@ -56,4 +56,9 @@ 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 rp_trampoline_vaddr, struct pt_regs *regs);
> > +
> > +static inline unsigned long arch_uretprobe_get_sp(struct pt_regs *regs)
> > +{
> > + return (unsigned long)regs->sp;
> > +}
>
> You could use GET_USP() here.
perhaps, which in turn is a helper for user_stack_pointer() :) comment
is valid though.
thanks,
Anton.
On Mon, Mar 04, 2013 at 05:47:53PM +0100, Oleg Nesterov wrote:
> On 03/04, Anton Arapov wrote:
> >
> > @@ -1085,6 +1093,7 @@ static int xol_add_vma(struct xol_area *area)
> > {
> > struct mm_struct *mm = current->mm;
> > int ret = -EALREADY;
> > + uprobe_opcode_t insn = UPROBE_SWBP_INSN;
> >
> > down_write(&mm->mmap_sem);
> > if (mm->uprobes_state.xol_area)
> > @@ -1106,6 +1115,13 @@ static int xol_add_vma(struct xol_area *area)
> > smp_wmb(); /* pairs with get_xol_area() */
> > mm->uprobes_state.xol_area = area;
> > ret = 0;
> > +
> > + /*
> > + * If we reached this place, we did allocate a new area. We want
> > + * pre-alloc a slot for the return probes here.
> > + */
> > + xol_get_insn_slot(&insn);
> Just change get_xol_area() to do set_bit(0, bitmap) and copy_to_page(page, int3)
> (extacted from xol_get_insn_slot().
ah yes, right. Thanks for this hint.
>
> > @@ -1485,8 +1538,11 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> > *is_swbp = -EFAULT;
> > }
> >
> > - if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
> > + utask = get_utask();
> > + if (!uprobe && hlist_empty(&utask->return_uprobes) &&
> > + test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags)) {
> > mmf_recalc_uprobes(mm);
> Wait, I was wrong. We should not clear MMF_* if another thread has
> ->return_uprobes. Perhaps we should change uprobe_pre_sstep_notifier()
> instead...
Will look into it.
> > down_read(&uprobe->register_rwsem);
> > for (uc = uprobe->consumers; uc; uc = uc->next) {
> > - int rc = uc->handler(uc, regs);
> > + if (uc->handler)
> > + rc = uc->handler(uc, regs);
> > +
> > + if (uc->rp_handler)
> > + prepare_uretprobe(uprobe, regs); /* put bp at return */
> Again, this is not right. We should do this only once after the main
> loop.
fixed for v5.
Thanks!
Anton.
On Mon, Mar 04, 2013 at 05:51:20PM +0100, Oleg Nesterov wrote:
> On 03/04, Anton Arapov wrote:
> >
> > +static void handle_uretprobe(struct xol_area *area, struct pt_regs *regs)
> > +{
> > + struct hlist_head *head;
> > + struct hlist_node *tmp;
> > + struct return_uprobe_i *ri;
> > + struct uprobe_task *utask;
> > + unsigned long orig_ret_vaddr;
> > +
> > + /* TODO: uretprobe bypass logic */
> > +
> > + utask = get_utask();
> > + if (!utask) {
> > + /* TODO:RFC task is not probed, do we want printk here? */
> > + return;
> > + }
> > + head = &utask->return_uprobes;
> > + hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> > + if (ri->uprobe->consumers) {
> > + instruction_pointer_set(regs, ri->orig_ret_vaddr);
> This doesn't look right if ri->orig_ret_vaddr == area->vaddr. We should
> splice the list and find orig_ret_vaddr in advance.
True, this cycle is buggy. I will rework handle_uretprobe().
> > @@ -1589,8 +1639,11 @@ static void handle_swbp(struct pt_regs *regs)
> >
> > if (!uprobe) {
> > if (is_swbp > 0) {
> > - /* No matching uprobe; signal SIGTRAP. */
> > - send_sig(SIGTRAP, current, 0);
> > + area = get_xol_area();
> > + if (area && bp_vaddr == area->vaddr)
> > + handle_uretprobe(area, regs);
> > + else
> > + send_sig(SIGTRAP, current, 0);
>
> Why? We can check bp_vaddr at the start, before find_active_uprobe().
For some reason, I was thinking it is better to hide this logic under
if (!uprobe). Will correct this chunk.
> And I'd suggest to not use area->vaddr directly, imho a trivial helper
> makes sense.
I see the idea behind, with this change it will be more clear and
fragile in case someone change the underneath logic. Will do this.
thank you very much!
Anton.