uprobes: Teach handler_chain() to filter out the probed task
Subsystems (2)
Oleg Nesterov
maintainercover uprobes: Teach handler_chain() to filter out the probed task
Hello.
On top of "uprobes: pre-filtering".
Srikar, If you still disagree and prefer to use ->filter() in handler_chain() please argue.
Note that unapply_uprobe() added by this patch can be generalized to implement uprobe_apply(consumer, mm, is_register).
Oleg.
Oleg Nesterov
maintainerPATCH 1/1 uprobes: Teach handler_chain() to filter out the probed task
Currrently the are 2 problems with pre-filtering:
1. It is not possible to add/remove a task (mm) after uprobe_register()
2. A forked child inherits all breakpoints and uprobe_consumer can not control this.
This patch does the first step to improve the filtering. handler_chain() removes the breakpoints installed by this uprobe from current->mm if all handlers return UPROBE_HANDLER_REMOVE.
Note that handler_chain() relies on ->register_rwsem to avoid the race with uprobe_register/unregister which can add/del a consumer, or even remove and then insert the new uprobe at the same address.
Perhaps we will add uprobe_apply_mm(uprobe, mm, is_register) and teach copy_mm() to do filter(UPROBE_FILTER_FORK), but I think this change makes sense anyway.
Note: instead of checking the retcode from uc->handler, we could add uc->filter(UPROBE_FILTER_BPHIT). But I think this is not optimal to call 2 hooks in a row. This buys nothing, and if handler/filter do something nontrivial they will probably do the same work twice.
--- include/linux/uprobes.h | 3 ++ kernel/events/uprobes.c | 58 ++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 51 insertions(+), 10 deletions(-)
include/linux/uprobes.h +3 -0
@@ -35,6 +35,9 @@ struct inode;# include <asm/uprobes.h> #endif +#define UPROBE_HANDLER_REMOVE 1 +#define UPROBE_HANDLER_MASK 1 +enum uprobe_filter_ctx {UPROBE_FILTER_REGISTER ,UPROBE_FILTER_UNREGISTER ,
kernel/events/uprobes.c +48 -10
76 lines not on this page · raw message
Signature · 1 line
1.5.5.1
Quoted 11 lines · Oleg Nesterov
Currrently the are 2 problems with pre-filtering:
1. It is not possible to add/remove a task (mm) after
uprobe_register()2. A forked child inherits all breakpoints and
uprobe_consumercan not control this.This patch does the first step to improve the filtering.
handler_chain()removes the breakpoints installed by this uprobe fromcurrent->mmif all handlers returnUPROBE_HANDLER_REMOVE.
I am thinking of tid based filter, If let say a tracer is just interested in a particular thread of a process, should such a hanlder always return 0.
In general, does this mean if the handler is not interested for this particular task, but not sure if other tasks in the same process could be interested, then such a handler should always return 0?
If yes, should we document it (either in handler_chain() or near uprobe_consumer definition)
Quoted 4 lines
Note that
handler_chain()relies on ->register_rwsem to avoid the race with uprobe_register/unregister which can add/del a consumer, or even remove and then insert the new uprobe at the same address.
Quoted 8 lines
Perhaps we will add
uprobe_apply_mm(uprobe, mm, is_register)and teachcopy_mm()to dofilter(UPROBE_FILTER_FORK), but I think this change makes sense anyway.Note: instead of checking the retcode from
uc->handler, we could adduc->filter(UPROBE_FILTER_BPHIT). But I think this is not optimal to call 2 hooks in a row. This buys nothing, and if handler/filter do something nontrivial they will probably do the same work twice.
I was for having the filter called explicitly. But I am okay with it being called internally by the handler. My only small concern was
- Given that we have an explicit filter, handlers (or folks writing handlers can misunderstand and miss filtering assuming that handlers would be called after filtering.
VG
Quoted 80 lines
Signed-off-by Oleg Nesterov <[email protected]>--- include/linux/uprobes.h | 3 ++ kernel/events/uprobes.c | 58 ++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 51 insertions(+), 10 deletions(-)include/linux/uprobes.h +3
-0@@ -35,6 +35,9 @@ struct inode;# include <asm/uprobes.h> #endif +#define UPROBE_HANDLER_REMOVE 1 +#define UPROBE_HANDLER_MASK 1 +enum uprobe_filter_ctx {UPROBE_FILTER_REGISTER ,UPROBE_FILTER_UNREGISTER ,kernel/events/uprobes.c +28
-10@@ -440,16 +440,6 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)return uprobe; } -static void handler_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) - uc->handler (uc, regs); -up_read (&uprobe->register_rwsem); -} -static void consumer_add (struct uprobe *uprobe,struct uprobe_consumer *uc) {down_write (&uprobe->consumer_rwsem);@@ -882,6 +872,33 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumeput_uprobe (uprobe); } +static int unapply_uprobe (struct uprobe *uprobe,struct mm_struct *mm) +{ +struct vm_area_struct *vma; +int err =0 ; + +down_read (&mm->mmap_sem); +for (vma = mm->mmap; vma; vma = vma->vm_next) { +unsigned long vaddr; +loff_t offset; + +if (!valid_vma (vma, false) || + vma->vm_file->f_mapping->host != uprobe->inode) +continue ; + + offset = (loff_t )vma->vm_pgoff <<PAGE_SHIFT ; +if (uprobe->offset < offset || + uprobe->offset >= offset + vma->vm_end - vma->vm_start) +continue ; + + vaddr =offset_to_vaddr (vma, uprobe->offset); + err |=remove_breakpoint (uprobe, mm, vaddr); + } +up_read (&mm->mmap_sem); + +return err; +} +static struct rb_node *find_node_in_range (struct inode *inode,loff_t min,loff_t max) {@@ -1435,6 +1452,27 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)return uprobe; } +static void handler_chain (struct uprobe *uprobe,struct pt_regs *regs)
kernel/events/uprobes.c static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) · line 1465
Quoted 10 lines
kernel/events/uprobes.c +10
-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); + +WARN (rc & ~UPROBE_HANDLER_MASK , +"bad rc=0x%x from %pf()\n" , rc, uc->handler);
Is this warning required?
kernel/events/uprobes.c static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) · line 1475
Quoted 10 lines
kernel/events/uprobes.c +10
-0+ remove &= rc; + } + +if (remove && uprobe->consumers) { +WARN_ON (!uprobe_is_active (uprobe)); +unapply_uprobe (uprobe, current->mm); + } +up_read (&uprobe->register_rwsem); +} +
The rest looks good.
Signature · 2 lines
Thanks and Regards Srikar
Oleg Nesterov
maintainerQuoted 11 lines · Srikar Dronamraju
Quoted 4 lines · Oleg Nesterov
This patch does the first step to improve the filtering.
handler_chain()removes the breakpoints installed by this uprobe fromcurrent->mmif all handlers returnUPROBE_HANDLER_REMOVE.I am thinking of tid based filter, If let say a tracer is just interested in a particular thread of a process, should such a hanlder always return 0.
In this case ->handler() should return
current->mm == PROBED_THEAD->mm ? 0 : UPROBE_HANDLER_REMOVE
Quoted 3 lines
In general, does this mean if the handler is not interested for this particular task, but not sure if other tasks in the same process could be interested, then such a handler should always return 0?
Probably yes. Obviously it should return UPROBE_HANDLER_REMOVE only if it knows for sure that current can't share ->mm with the "interesting" task.
Because, whatever we do, remove_breakpoint() affects ->mm, not task_struct. Our goal is eliminate do_int3(), not to skip uc->handler() call.
Quoted 2 lines
If yes, should we document it (either in
handler_chain()or nearuprobe_consumerdefinition)
Oh yes, I do agree. We need to add some documentation. I'll try to do this in a separate patch (although I would be happy to see the patch from someone else ;).
Quoted 7 lines
Quoted 4 lines
Note: instead of checking the retcode from
uc->handler, we could adduc->filter(UPROBE_FILTER_BPHIT). But I think this is not optimal to call 2 hooks in a row. This buys nothing, and if handler/filter do something nontrivial they will probably do the same work twice.I was for having the filter called explicitly. But I am okay with it being called internally by the handler.
OK, thanks,
Quoted 5 lines
My only small concern was
- Given that we have an explicit filter, handlers (or folks writing handlers can misunderstand and miss filtering assuming that handlers would be called after filtering.
Do you mean that they can assume that uc->filter(mm) should be called at least once before uc->handler() with the same current->mm ?
They shouldn't in any case. To remind, we can optimize filter_chain() for example and avoid the potentially costly uc->filter() call. Say, we can detect/remember the fact that at least one consumre has ->filter == NULL.
OTOH, UPROBE_HANDLER_REMOVE is not really pre-filtering (although I think it helps to make the things better). It is more like uprobe_unapply_mm() which (perhaps) we need as well. But doing uprobe_unapply_mm() from uc->handler is a) deadlockable and b) not optimal because it has to consult other consumers.
Anyway I agree, the folks writing handlers should understand what do they do ;) and this needs some documentation.
Quoted 5 lines
Quoted 5 lines
kernel/events/uprobes.c +5
-0+static void handler_chain (struct uprobe *uprobe,struct pt_regs *regs) +{ +struct uprobe_consumer *uc; +int remove =UPROBE_HANDLER_REMOVE ; +
Quoted 8 lines
kernel/events/uprobes.c static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) · line 1465
Quoted 6 lines
kernel/events/uprobes.c +6
-0+down_read (&uprobe->register_rwsem); +for (uc = uprobe->consumers; uc; uc = uc->next) { +int rc = uc->handler (uc, regs); + +WARN (rc & ~UPROBE_HANDLER_MASK , +"bad rc=0x%x from %pf()\n" , rc, uc->handler);Is this warning required?
Of course this is not strictly needed, just to catch the simple mistakes. I can remove it.
Oleg.
Quoted 17 lines · Oleg Nesterov
Quoted 11 lines · Srikar Dronamraju
Quoted 4 lines · Oleg Nesterov
This patch does the first step to improve the filtering.
handler_chain()removes the breakpoints installed by this uprobe fromcurrent->mmif all handlers returnUPROBE_HANDLER_REMOVE.I am thinking of tid based filter, If let say a tracer is just interested in a particular thread of a process, should such a hanlder always return 0.
In this case ->handler() should return
current->mm == PROBED_THEAD->mm ? 0 : UPROBE_HANDLER_REMOVE
Quoted 8 lines
Quoted 3 lines
In general, does this mean if the handler is not interested for this particular task, but not sure if other tasks in the same process could be interested, then such a handler should always return 0?
Probably yes. Obviously it should return
UPROBE_HANDLER_REMOVEonly if it knows for sure that current can't share ->mm with the "interesting" task.
okay, then looks good.
Quoted 7 lines
Because, whatever we do,
remove_breakpoint()affects ->mm, nottask_struct. Our goal is eliminatedo_int3(), not to skipuc->handler()call.Quoted 2 lines
If yes, should we document it (either in
handler_chain()or nearuprobe_consumerdefinition)Oh yes, I do agree. We need to add some documentation.
Okay
Quoted 14 lines
I'll try to do
this in a separate patch (although I would be happy to see the patch from someone else ;).Quoted 7 lines
Quoted 4 lines
Note: instead of checking the retcode from
uc->handler, we could adduc->filter(UPROBE_FILTER_BPHIT). But I think this is not optimal to call 2 hooks in a row. This buys nothing, and if handler/filter do something nontrivial they will probably do the same work twice.I was for having the filter called explicitly. But I am okay with it being called internally by the handler.
OK, thanks,
Quoted 9 lines
Quoted 5 lines
My only small concern was
- Given that we have an explicit filter, handlers (or folks writing handlers can misunderstand and miss filtering assuming that handlers would be called after filtering.
Do you mean that they can assume that
uc->filter(mm)should be called at least once beforeuc->handler()with the samecurrent->mm?
yes, thats what I think they can assume.
Quoted 5 lines
They shouldn't in any case. To remind, we can optimize
filter_chain()for example and avoid the potentially costlyuc->filter()call. Say, we can detect/remember the fact that at least one consumre has ->filter == NULL.
Quoted 8 lines
OTOH,
UPROBE_HANDLER_REMOVEis not really pre-filtering (although I think it helps to make the things better). It is more likeuprobe_unapply_mm()which (perhaps) we need as well. But doinguprobe_unapply_mm()fromuc->handleris a) deadlockable and b) not optimal because it has to consult other consumers.Anyway I agree, the folks writing handlers should understand what do they do ;) and this needs some documentation.
If we document explicitly that filter wont be called, then this should be okay.
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
kernel/events/uprobes.c +11
-0+static void handler_chain (struct uprobe *uprobe,struct pt_regs *regs) +{ +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); + +WARN (rc & ~UPROBE_HANDLER_MASK , +"bad rc=0x%x from %pf()\n" , rc, uc->handler);
Oleg Nesterov
maintainerQuoted 14 lines · Srikar Dronamraju
Quoted 11 lines · Oleg Nesterov
They shouldn't in any case. To remind, we can optimize
filter_chain()for example and avoid the potentially costlyuc->filter()call. Say, we can detect/remember the fact that at least one consumre has ->filter == NULL.OTOH,
UPROBE_HANDLER_REMOVEis not really pre-filtering (although I think it helps to make the things better). It is more likeuprobe_unapply_mm()which (perhaps) we need as well. But doinguprobe_unapply_mm()fromuc->handleris a) deadlockable and b) not optimal because it has to consult other consumers.
Quoted 5 lines
Quoted 2 lines
Anyway I agree, the folks writing handlers should understand what do they do ;) and this needs some documentation.
If we document explicitly that filter wont be called, then this should be okay.
OK.
Can I take this as your ACK?
Oleg.
Quoted 19 lines · Oleg Nesterov
Currrently the are 2 problems with pre-filtering:
1. It is not possible to add/remove a task (mm) after
uprobe_register()2. A forked child inherits all breakpoints and
uprobe_consumercan not control this.This patch does the first step to improve the filtering.
handler_chain()removes the breakpoints installed by this uprobe fromcurrent->mmif all handlers returnUPROBE_HANDLER_REMOVE.Note that
handler_chain()relies on ->register_rwsem to avoid the race with uprobe_register/unregister which can add/del a consumer, or even remove and then insert the new uprobe at the same address.Perhaps we will add
uprobe_apply_mm(uprobe, mm, is_register)and teachcopy_mm()to dofilter(UPROBE_FILTER_FORK), but I think this change makes sense anyway.
Quoted 6 lines
Note: instead of checking the retcode from
uc->handler, we could adduc->filter(UPROBE_FILTER_BPHIT). But I think this is not optimal to call 2 hooks in a row. This buys nothing, and if handler/filter do something nontrivial they will probably do the same work twice.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 106 lines
--- include/linux/uprobes.h | 3 ++ kernel/events/uprobes.c | 58 ++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 51 insertions(+), 10 deletions(-)include/linux/uprobes.h +3
-0@@ -35,6 +35,9 @@ struct inode;# include <asm/uprobes.h> #endif +#define UPROBE_HANDLER_REMOVE 1 +#define UPROBE_HANDLER_MASK 1 +enum uprobe_filter_ctx {UPROBE_FILTER_REGISTER ,UPROBE_FILTER_UNREGISTER ,kernel/events/uprobes.c +48
-10@@ -440,16 +440,6 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)return uprobe; } -static void handler_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) - uc->handler (uc, regs); -up_read (&uprobe->register_rwsem); -} -static void consumer_add (struct uprobe *uprobe,struct uprobe_consumer *uc) {down_write (&uprobe->consumer_rwsem);@@ -882,6 +872,33 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumeput_uprobe (uprobe); } +static int unapply_uprobe (struct uprobe *uprobe,struct mm_struct *mm) +{ +struct vm_area_struct *vma; +int err =0 ; + +down_read (&mm->mmap_sem); +for (vma = mm->mmap; vma; vma = vma->vm_next) { +unsigned long vaddr; +loff_t offset; + +if (!valid_vma (vma, false) || + vma->vm_file->f_mapping->host != uprobe->inode) +continue ; + + offset = (loff_t )vma->vm_pgoff <<PAGE_SHIFT ; +if (uprobe->offset < offset || + uprobe->offset >= offset + vma->vm_end - vma->vm_start) +continue ; + + vaddr =offset_to_vaddr (vma, uprobe->offset); + err |=remove_breakpoint (uprobe, mm, vaddr); + } +up_read (&mm->mmap_sem); + +return err; +} +static struct rb_node *find_node_in_range (struct inode *inode,loff_t min,loff_t max) {@@ -1435,6 +1452,27 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)return uprobe; } +static void handler_chain (struct uprobe *uprobe,struct pt_regs *regs) +{ +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); + +WARN (rc & ~UPROBE_HANDLER_MASK , +"bad rc=0x%x from %pf()\n" , rc, uc->handler); + remove &= rc; + } + +if (remove && uprobe->consumers) { +WARN_ON (!uprobe_is_active (uprobe)); +unapply_uprobe (uprobe, current->mm); + } +up_read (&uprobe->register_rwsem); +} +/* * Run handler and ask thread to singlestep. * Ensure all non-fatal signals cannot interrupt thread while it singlesteps. Signature · 1 line
1.5.5.1