uprobes: register/unregister preparations for filtering
Subsystems (3)
Oleg Nesterov
maintainercover uprobes: register/unregister preparations for filtering
Hello.
Srikar, it is not clear if you was convinced or not during the last discussion. We will discuss this again probably. So far I am sending the initial changes which (I think) make sense in any case. Please review.
The next step is locking. Alas, we can not use consumer_rwsem to protect the list in filter_chain(). Whatever we do, at least uprobe_mmap() needs to do this under mm->mmap_sem. And we do want to allow uc->handler() to play with current->mm.
Then we will actually immplement filter_chain(), this should be trivial unless you insist we need for_each_mm_user() (cough, I hope you do not ;).
Then we will try to solve the problem with fork.
Oleg.
Oleg Nesterov
maintainerPATCH 1/7 uprobes: Move __set_bit(UPROBE_SKIP_SSTEP) into alloc_uprobe()
Cosmetic. __set_bit(UPROBE_SKIP_SSTEP) is the part of initialization, it is not clear why it is set in insert_uprobe().
--- kernel/events/uprobes.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
kernel/events/uprobes.c +2 -3
@@ -430,9 +430,6 @@ static struct uprobe *insert_uprobe(struct uprobe *uprobe) u =__insert_uprobe (uprobe);spin_unlock (&uprobes_treelock); -/* For now assume that the instruction need not be single-stepped */ -__set_bit (UPROBE_SKIP_SSTEP , &uprobe->flags); -return u; }
@@ -454,6 +451,8 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset) uprobe->offset = offset;init_rwsem (&uprobe->consumer_rwsem);mutex_init (&uprobe->copy_mutex); +/* For now assume that the instruction need not be single-stepped */ +__set_bit (UPROBE_SKIP_SSTEP , &uprobe->flags);/* add to uprobes_tree, sorted on inode:offset */ cur_uprobe =insert_uprobe (uprobe);
Signature · 1 line
1.5.5.1
Re: [PATCH 1/7] uprobes: Move __set_bit(UPROBE_SKIP_SSTEP) into alloc_uprobe()
Quoted 6 lines
Cosmetic.
__set_bit(UPROBE_SKIP_SSTEP)is the part of initialization, it is not clear why it is set ininsert_uprobe().Signed-off-by Oleg Nesterov <[email protected]>
Quoted 30 lines
--- kernel/events/uprobes.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)kernel/events/uprobes.c +2
-3@@ -430,9 +430,6 @@ static struct uprobe *insert_uprobe(struct uprobe *uprobe) u =__insert_uprobe (uprobe);spin_unlock (&uprobes_treelock); -/* For now assume that the instruction need not be single-stepped */ -__set_bit (UPROBE_SKIP_SSTEP , &uprobe->flags); -return u; }@@ -454,6 +451,8 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset) uprobe->offset = offset;init_rwsem (&uprobe->consumer_rwsem);mutex_init (&uprobe->copy_mutex); +/* For now assume that the instruction need not be single-stepped */ +__set_bit (UPROBE_SKIP_SSTEP , &uprobe->flags);/* add to uprobes_tree, sorted on inode:offset */ cur_uprobe =insert_uprobe (uprobe);Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 2/7 uprobes: Kill the "uprobe != NULL" check in uprobe_unregister()
In reply to Oleg NesterovTrivial. uprobe can't be NULL after mutex_unlock(), it was already used.
--- kernel/events/uprobes.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
kernel/events/uprobes.c +1 -2
@@ -900,8 +900,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume }mutex_unlock (uprobes_hash (inode)); -if (uprobe) -put_uprobe (uprobe); +put_uprobe (uprobe); } static struct rb_node *
Signature · 1 line
1.5.5.1
Re: [PATCH 2/7] uprobes: Kill the "uprobe != NULL" check in uprobe_unregister()
Quoted 3 lines · Oleg Nesterov
Trivial. uprobe can't be NULL after
mutex_unlock(), it was already used.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 21 lines
--- kernel/events/uprobes.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)kernel/events/uprobes.c +1
-2@@ -900,8 +900,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume }mutex_unlock (uprobes_hash (inode)); -if (uprobe) -put_uprobe (uprobe); +put_uprobe (uprobe); } static struct rb_node *Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 3/7 uprobes: Kill the pointless inode/uc checks in register/unregister
In reply to Oleg Nesterovregister/unregister verifies that inode/uc != NULL. For what? This really looks like "hide the potential problem", the caller should pass the valid data.
register() also checks uc->next == NULL, probably to prevent the double-register but the caller can do other stupid/wrong things. If we do this check, then we should document that uc->next should be cleared before register() and add BUG_ON().
Also add the small comment about the i_size_read() check.
--- kernel/events/uprobes.c | 7 +------ 1 files changed, 1 insertions(+), 6 deletions(-)
kernel/events/uprobes.c +1 -6
@@ -844,9 +844,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *struct uprobe *uprobe;int ret; -if (!inode || !uc || uc->next) -return -EINVAL ; - +/* Racy, just to catch the obvious mistakes */ if (offset >i_size_read (inode))return -EINVAL ;
@@ -883,9 +881,6 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume {struct uprobe *uprobe; -if (!inode || !uc) -return ; - uprobe =find_uprobe (inode, offset);if (!uprobe)return ;
Signature · 1 line
1.5.5.1
Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister
Quoted 4 lines · Oleg Nesterov
register/unregister verifies that inode/uc != NULL. For what? This really looks like "hide the potential problem", the caller should pass the valid data.
Agree that users should pass valid data.
I do understand that we expect the users to be knowledge-able. Also users are routed thro in-kernel api that does this check.
However from an api perspective, if a user passes invalid data, do we want the system to crash.
Esp if kernel can identify that users has indeed passed wrong info. I do agree that users can still pass invalid data that kernel maynot be able to identify in most cases.
Quoted 2 lines
register()also checksuc->next== NULL, probably to prevent the double-register but the caller can do other stupid/wrong things.
Users can surely do more stupid things. But this is again something that kernel can identify. By allowing a double-register of a consumer, thats already registered, we might end up allowing circular loop of consumers.
Quoted 38 lines
If we do this check, then we should document that
uc->nextshould be cleared beforeregister()and addBUG_ON().Also add the small comment about the
i_size_read()check.Signed-off-by Oleg Nesterov <[email protected]>--- kernel/events/uprobes.c | 7 +------ 1 files changed, 1 insertions(+), 6 deletions(-)kernel/events/uprobes.c +1
-6@@ -844,9 +844,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *struct uprobe *uprobe;int ret; -if (!inode || !uc || uc->next) -return -EINVAL ; - +/* Racy, just to catch the obvious mistakes */ if (offset >i_size_read (inode))return -EINVAL ;@@ -883,9 +881,6 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume {struct uprobe *uprobe; -if (!inode || !uc) -return ; - uprobe =find_uprobe (inode, offset);if (!uprobe)return ;Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerRe: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister
Quoted 12 lines · Srikar Dronamraju
Quoted 4 lines · Oleg Nesterov
register/unregister verifies that inode/uc != NULL. For what? This really looks like "hide the potential problem", the caller should pass the valid data.
Agree that users should pass valid data.
I do understand that we expect the users to be knowledge-able. Also users are routed thro in-kernel api that does this check.
Quoted 6 lines
However from an api perspective, if a user passes invalid data, do we want the system to crash.
Esp if kernel can identify that users has indeed passed wrong info. I do agree that users can still pass invalid data that kernel maynot be able to identify in most cases.
inode != NULL can't verify that it actually points to the valid inode, NULL is only one example of invalid data.
I agree, sometimes it makes sense to protect against the stupid mistakes, but if we want to check against NULL we should do
if (WARN_ON(!inode)) return;
Especially in uprobe_unregister(). The current code is really "hide the possible problem" and nothing more. It is better to crash imho than silently return.
Quoted 6 lines
Quoted 2 lines
register()also checksuc->next== NULL, probably to prevent the double-register but the caller can do other stupid/wrong things.Users can surely do more stupid things. But this is again something that kernel can identify. By allowing a double-register of a consumer, thats already registered, we might end up allowing circular loop of consumers.
I understand. But in this case we should document that uc->next must be cleared before uprobe_register(). Or add init_consumer().
And we should change uprobe_unregister() to clear uc->next as well. I think that the code like this
uprobe_register(uc); uprobe_unregister(uc); uprobe_register(uc);
should work. Currently it doesn't because of this check.
So I still think these checks are pointless and (at least in unregister) even harmful.
But I won't insist too much, I can drop this patch if you do not change your mind.
Oleg.
Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister
Quoted 20 lines · Oleg Nesterov
Quoted 12 lines · Srikar Dronamraju
Quoted 4 lines · Oleg Nesterov
register/unregister verifies that inode/uc != NULL. For what? This really looks like "hide the potential problem", the caller should pass the valid data.
Agree that users should pass valid data.
I do understand that we expect the users to be knowledge-able. Also users are routed thro in-kernel api that does this check.Quoted 6 lines
However from an api perspective, if a user passes invalid data, do we want the system to crash.
Esp if kernel can identify that users has indeed passed wrong info. I do agree that users can still pass invalid data that kernel maynot be able to identify in most cases.
Quoted 9 lines
inode != NULL can't verify that it actually points to the valid inode, NULL is only one example of invalid data.
I agree, sometimes it makes sense to protect against the stupid mistakes, but if we want to check against NULL we should do
if (WARN_ON(!inode)) return;
agree, that warn_on is better than a simple check
Quoted 17 lines
Especially in
uprobe_unregister(). The current code is really "hide the possible problem" and nothing more. It is better to crash imho than silently return.Quoted 6 lines
Quoted 2 lines
register()also checksuc->next== NULL, probably to prevent the double-register but the caller can do other stupid/wrong things.Users can surely do more stupid things. But this is again something that kernel can identify. By allowing a double-register of a consumer, thats already registered, we might end up allowing circular loop of consumers.
I understand. But in this case we should document that
uc->nextmust be cleared beforeuprobe_register(). Or addinit_consumer().And we should change
uprobe_unregister()to clearuc->nextas well. I think that the code like this
Quoted 7 lines
uprobe_register(uc); uprobe_unregister(uc); uprobe_register(uc);should work. Currently it doesn't because of this check.
yes, these should work and makes a case to nullify ->next on unregister.
However, what if someone tries
uprobe_register(uc1); uprobe_register(uc2); uprobe_register(uc1);
i.e somebody tries to re-register uc1, while its active and has a valid next. After the re-registration of uc1, the uprobe->consumers will no more reference uc2.
Should we leave this case as a fool shooting himself?
Quoted 3 lines
So I still think these checks are pointless and (at least in unregister) even harmful.
Signature · 2 lines
thanks and regards Srikar
Oleg Nesterov
maintainerRe: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister
Quoted 26 lines · Srikar Dronamraju
Quoted 23 lines · Oleg Nesterov
Quoted 12 lines · Srikar Dronamraju
Quoted 4 lines · Oleg Nesterov
register/unregister verifies that inode/uc != NULL. For what? This really looks like "hide the potential problem", the caller should pass the valid data.
Agree that users should pass valid data.
I do understand that we expect the users to be knowledge-able. Also users are routed thro in-kernel api that does this check.Quoted 6 lines
However from an api perspective, if a user passes invalid data, do we want the system to crash.
Esp if kernel can identify that users has indeed passed wrong info. I do agree that users can still pass invalid data that kernel maynot be able to identify in most cases.
inode != NULL can't verify that it actually points to the valid inode, NULL is only one example of invalid data.
Quoted 8 lines
Quoted 6 lines
I agree, sometimes it makes sense to protect against the stupid mistakes, but if we want to check against NULL we should do
if (WARN_ON(!inode)) return;agree, that
warn_onis better than a simple check
and this one
if (WARN_ON(inode < PAGE_OFFSET))
is even better ;)
Quoted 27 lines
Quoted 17 lines
Especially in
uprobe_unregister(). The current code is really "hide the possible problem" and nothing more. It is better to crash imho than silently return.Quoted 6 lines
Quoted 2 lines
register()also checksuc->next== NULL, probably to prevent the double-register but the caller can do other stupid/wrong things.Users can surely do more stupid things. But this is again something that kernel can identify. By allowing a double-register of a consumer, thats already registered, we might end up allowing circular loop of consumers.
I understand. But in this case we should document that
uc->nextmust be cleared beforeuprobe_register(). Or addinit_consumer().And we should change
uprobe_unregister()to clearuc->nextas well. I think that the code like thisQuoted 7 lines
uprobe_register(uc); uprobe_unregister(uc); uprobe_register(uc);should work. Currently it doesn't because of this check.
yes, these should work and makes a case to nullify ->next on unregister.
Quoted 8 lines
However, what if someone tries
uprobe_register(uc1); uprobe_register(uc2); uprobe_register(uc1);i.e somebody tries to re-register uc1, while its active and has a valid next. After the re-registration of uc1, the
uprobe->consumerswill no more reference uc2.
Yes. And even without uprobe_register(uc2) the result won't be good. This is like list_add(node).
Quoted 1 line
Should we leave this case as a fool shooting himself?
IMHO yes, or we should create init_consumer() or at least document that the private ->next member should be nullified.
But let me repeat,
Quoted 2 lines
Quoted 2 lines
So I still think these checks are pointless and (at least in unregister) even harmful.
Yes, but I am not going to argue if you want to keep these checks.
Oleg.
Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
I agree, sometimes it makes sense to protect against the stupid mistakes, but if we want to check against NULL we should do
if (WARN_ON(!inode)) return;
Quoted 7 lines
Quoted 1 line
agree, that
warn_onis better than a simple checkand this one
if (
WARN_ON(inode < PAGE_OFFSET))is even better ;)
Okay.
Quoted 37 lines
Quoted 28 lines
Quoted 18 lines
Especially in
uprobe_unregister(). The current code is really "hide the possible problem" and nothing more. It is better to crash imho than silently return.Quoted 6 lines
Quoted 2 lines
register()also checksuc->next== NULL, probably to prevent the double-register but the caller can do other stupid/wrong things.Users can surely do more stupid things. But this is again something that kernel can identify. By allowing a double-register of a consumer, thats already registered, we might end up allowing circular loop of consumers.
I understand. But in this case we should document that
uc->nextmust be cleared beforeuprobe_register(). Or addinit_consumer().And we should change
uprobe_unregister()to clearuc->nextas well. I think that the code like thisQuoted 7 lines
uprobe_register(uc); uprobe_unregister(uc); uprobe_register(uc);should work. Currently it doesn't because of this check.
yes, these should work and makes a case to nullify ->next on unregister.
Quoted 8 lines
However, what if someone tries
uprobe_register(uc1); uprobe_register(uc2); uprobe_register(uc1);i.e somebody tries to re-register uc1, while its active and has a valid next. After the re-registration of uc1, the
uprobe->consumerswill no more reference uc2.
Quoted 8 lines
Yes. And even without
uprobe_register(uc2)the result won't be good. This is likelist_add(node).Quoted 1 line
Should we leave this case as a fool shooting himself?
IMHO yes, or we should create
init_consumer()or at least document that the private ->next member should be nullified.
Okay, Since we agree that its a user mistake. So lets document this and continue with what you propose.
Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister
In reply to Oleg NesterovQuoted 4 lines · Oleg Nesterov
register/unregister verifies that inode/uc != NULL. For what? This really looks like "hide the potential problem", the caller should pass the valid data.
Quoted 8 lines
register()also checksuc->next== NULL, probably to prevent the double-register but the caller can do other stupid/wrong things. If we do this check, then we should document thatuc->nextshould be cleared beforeregister()and addBUG_ON().Also add the small comment about the
i_size_read()check.Signed-off-by Oleg Nesterov <[email protected]>
We might want to add a warn .. but that shouldnt stop this patch though
Quoted 32 lines
--- kernel/events/uprobes.c | 7 +------ 1 files changed, 1 insertions(+), 6 deletions(-)kernel/events/uprobes.c +1
-6@@ -844,9 +844,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *struct uprobe *uprobe;int ret; -if (!inode || !uc || uc->next) -return -EINVAL ; - +/* Racy, just to catch the obvious mistakes */ if (offset >i_size_read (inode))return -EINVAL ;@@ -883,9 +881,6 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume {struct uprobe *uprobe; -if (!inode || !uc) -return ; - uprobe =find_uprobe (inode, offset);if (!uprobe)return ;Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 4/7 uprobes: Kill uprobe_consumer->filter()
In reply to Oleg Nesterovuprobe_consumer->filter() is pointless in its current form, kill it.
We will add it back, but with the different signature/semantics. Perhaps we will even re-introduce the callsite in handler_chain(), but not to just skip uc->handler().
--- include/linux/uprobes.h | 5 ----- kernel/events/uprobes.c | 6 ++---- kernel/trace/trace_uprobe.c | 1 - 3 files changed, 2 insertions(+), 10 deletions(-)
include/linux/uprobes.h +0 -5
@@ -37,11 +37,6 @@ struct inode;struct uprobe_consumer {int (*handler)(struct uprobe_consumer *self,struct pt_regs *regs); -/* -* filter is optional; If a filter exists, handler is run -* if and only if filter returns true. -*/ -bool (*filter)(struct uprobe_consumer *self,struct task_struct *task);struct uprobe_consumer *next; };
kernel/events/uprobes.c +2 -4
@@ -477,10 +477,8 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)return ;down_read (&uprobe->consumer_rwsem); -for (uc = uprobe->consumers; uc; uc = uc->next) { -if (!uc->filter || uc->filter (uc, current)) - uc->handler (uc, regs); - } +for (uc = uprobe->consumers; uc; uc = uc->next) + uc->handler (uc, regs);up_read (&uprobe->consumer_rwsem); }
kernel/trace/trace_uprobe.c +0 -1
@@ -546,7 +546,6 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)return -EINTR ; utc->cons.handler = uprobe_dispatcher; - utc->cons.filter =NULL ; ret =uprobe_register (tu->inode, tu->offset, &utc->cons);if (ret) {kfree (utc);
Signature · 1 line
1.5.5.1
Re: [PATCH 4/7] uprobes: Kill uprobe_consumer->filter()
Quoted 7 lines · Oleg Nesterov
uprobe_consumer->filter()is pointless in its current form, kill it.We will add it back, but with the different signature/semantics. Perhaps we will even re-introduce the callsite in
handler_chain(), but not to just skipuc->handler().Signed-off-by Oleg Nesterov <[email protected]>
Quoted 54 lines
--- include/linux/uprobes.h | 5 ----- kernel/events/uprobes.c | 6 ++---- kernel/trace/trace_uprobe.c | 1 - 3 files changed, 2 insertions(+), 10 deletions(-)include/linux/uprobes.h +0
-5@@ -37,11 +37,6 @@ struct inode;struct uprobe_consumer {int (*handler)(struct uprobe_consumer *self,struct pt_regs *regs); -/* -* filter is optional; If a filter exists, handler is run -* if and only if filter returns true. -*/ -bool (*filter)(struct uprobe_consumer *self,struct task_struct *task);struct uprobe_consumer *next; };kernel/events/uprobes.c +2
-4@@ -477,10 +477,8 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)return ;down_read (&uprobe->consumer_rwsem); -for (uc = uprobe->consumers; uc; uc = uc->next) { -if (!uc->filter || uc->filter (uc, current)) - uc->handler (uc, regs); - } +for (uc = uprobe->consumers; uc; uc = uc->next) + uc->handler (uc, regs);up_read (&uprobe->consumer_rwsem); }kernel/trace/trace_uprobe.c +0
-1@@ -546,7 +546,6 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)return -EINTR ; utc->cons.handler = uprobe_dispatcher; - utc->cons.filter =NULL ; ret =uprobe_register (tu->inode, tu->offset, &utc->cons);if (ret) {kfree (utc);Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 5/7 uprobes: Introduce filter_chain()
In reply to Oleg NesterovAdd the new helper filter_chain(). Currently it is only placeholder, the comment explains what is should do. We will change it later to consult every consumer to decide whether we need to install the swbp. Until then it works as if any consumer returns true, this matches the current behavior.
Change install_breakpoint() to call filter_chain() instead of checking uprobe->consumers != NULL. We obviously need this, and this equally closes the race with _unregister().
Change remove_breakpoint() to call this helper too. Currently this is pointless because remove_breakpoint() is only called when the last consumer goes away, but we will change this.
--- kernel/events/uprobes.c | 24 +++++++++++++++++++----- 1 files changed, 19 insertions(+), 5 deletions(-)
kernel/events/uprobes.c +19 -5
@@ -614,6 +614,18 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,return ret; } +static bool filter_chain (struct uprobe *uprobe) +{ +/* +* TODO: +* for_each_consumer(uc) +* if (uc->filter(...)) +* return true; +* return false; +*/ +return uprobe->consumers !=NULL ; +} +static int install_breakpoint (struct uprobe *uprobe,struct mm_struct *mm,struct vm_area_struct *vma,unsigned long vaddr)
@@ -624,11 +636,10 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,/* * If probe is being deleted, unregister thread could be done with * the vma-rmap-walk through. Adding a probe now can be fatal since -* nobody will be able to cleanup. Also we could be from fork or -* mremap path, where the probe might have already been inserted. -* Hence behave as if probe already existed. +* nobody will be able to cleanup. But in this case filter_chain() +* must return false, all consumers have gone away. */ -if (!uprobe->consumers) +if (!filter_chain (uprobe))return 0 ; ret =prepare_uprobe (uprobe, vma->vm_file, mm, vaddr);
@@ -655,10 +666,12 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,static int remove_breakpoint (struct uprobe *uprobe,struct mm_struct *mm,unsigned long vaddr) { -/* can happen if uprobe_register() fails */ if (!test_bit (MMF_HAS_UPROBES , &mm->flags))return 0 ; +if (filter_chain (uprobe)) +return 0 ; +set_bit (MMF_RECALC_UPROBES , &mm->flags);return set_orig_insn (&uprobe->arch, mm, vaddr); }
@@ -1382,6 +1395,7 @@ static void mmf_recalc_uprobes(struct mm_struct *mm)* This is not strictly accurate, we can race with * uprobe_unregister() and see the already removed * uprobe if delete_uprobe() was not yet called. +* Or this uprobe can be filtered out. */ if (vma_has_uprobes (vma, vma->vm_start, vma->vm_end))return ;
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerRe: [PATCH 5/7] uprobes: Introduce filter_chain()
Quoted 8 lines · Oleg Nesterov
Change
install_breakpoint()to callfilter_chain()instead of checkinguprobe->consumers!= NULL. We obviously need this, and this equally closes the race with_unregister().Change
remove_breakpoint()to call this helper too. Currently this is pointless becauseremove_breakpoint()is only called when the last consumer goes away, but we will change this.
Just in case...
This is only to make the initial change as simple as possible. filter_chain() will have more arguments and more callers, say, perhaps build_map_info(). And perhaps these 2 callsites should be moved from install/remove to the caller later.
Oleg.
Re: [PATCH 5/7] uprobes: Introduce filter_chain()
In reply to Oleg NesterovQuoted 10 lines · Oleg Nesterov
Add the new helper
filter_chain(). Currently it is only placeholder, the comment explains what is should do. We will change it later to consult every consumer to decide whether we need to install the swbp. Until then it works as if any consumer returns true, this matches the current behavior.Change
install_breakpoint()to callfilter_chain()instead of checkinguprobe->consumers!= NULL. We obviously need this, and this equally closes the race with_unregister().
Quoted 5 lines
Change
remove_breakpoint()to call this helper too. Currently this is pointless becauseremove_breakpoint()is only called when the last consumer goes away, but we will change this.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 67 lines
--- kernel/events/uprobes.c | 24 +++++++++++++++++++----- 1 files changed, 19 insertions(+), 5 deletions(-)kernel/events/uprobes.c +19
-5@@ -614,6 +614,18 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,return ret; } +static bool filter_chain (struct uprobe *uprobe) +{ +/* +* TODO: +* for_each_consumer(uc) +* if (uc->filter(...)) +* return true; +* return false; +*/ +return uprobe->consumers !=NULL ; +} +static int install_breakpoint (struct uprobe *uprobe,struct mm_struct *mm,struct vm_area_struct *vma,unsigned long vaddr)@@ -624,11 +636,10 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,/* * If probe is being deleted, unregister thread could be done with * the vma-rmap-walk through. Adding a probe now can be fatal since -* nobody will be able to cleanup. Also we could be from fork or -* mremap path, where the probe might have already been inserted. -* Hence behave as if probe already existed. +* nobody will be able to cleanup. But in this case filter_chain() +* must return false, all consumers have gone away. */ -if (!uprobe->consumers) +if (!filter_chain (uprobe))return 0 ; ret =prepare_uprobe (uprobe, vma->vm_file, mm, vaddr);@@ -655,10 +666,12 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,static int remove_breakpoint (struct uprobe *uprobe,struct mm_struct *mm,unsigned long vaddr) { -/* can happen if uprobe_register() fails */ if (!test_bit (MMF_HAS_UPROBES , &mm->flags))return 0 ; +if (filter_chain (uprobe)) +return 0 ; +set_bit (MMF_RECALC_UPROBES , &mm->flags);return set_orig_insn (&uprobe->arch, mm, vaddr); }@@ -1382,6 +1395,7 @@ static void mmf_recalc_uprobes(struct mm_struct *mm)* This is not strictly accurate, we can race with * uprobe_unregister() and see the already removed * uprobe if delete_uprobe() was not yet called. +* Or this uprobe can be filtered out. */ if (vma_has_uprobes (vma, vma->vm_start, vma->vm_end))return ;Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 6/7 uprobes: _unregister() should always do register_for_each_vma(false)
In reply to Oleg Nesterovuprobe_unregister() removes the breakpoints only if the last consumer goes away. To support the filtering it should do this every time, we want to remove the breakpoints which nobody else want to keep.
Note: given that filter_chain() is not actually implemented, this patch itself doesn't change the behaviour yet, register_for_each_vma(false) is a heavy "nop" unless there are no more consumers.
--- kernel/events/uprobes.c | 28 ++++++++++++++-------------- 1 files changed, 14 insertions(+), 14 deletions(-)
kernel/events/uprobes.c +14 -14
@@ -825,12 +825,20 @@ static int __uprobe_register(struct uprobe *uprobe)return register_for_each_vma (uprobe, true); } -static void __uprobe_unregister (struct uprobe *uprobe) +static void __uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc) { -if (!register_for_each_vma (uprobe, false)) -delete_uprobe (uprobe); +int err; + +if (!consumer_del (uprobe, uc))/* WARN? */ +return ; -/* TODO : cant unregister? schedule a worker thread */ + err =register_for_each_vma (uprobe, false); +if (!uprobe->consumers) { +clear_bit (UPROBE_RUN_HANDLER , &uprobe->flags); +/* TODO : cant unregister? schedule a worker thread */ +if (!err) +delete_uprobe (uprobe); + } }/*
@@ -868,8 +876,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer * }else if (!consumer_add (uprobe, uc)) { ret =__uprobe_register (uprobe);if (ret) { - uprobe->consumers =NULL ; -__uprobe_unregister (uprobe); +__uprobe_unregister (uprobe, uc); }else {set_bit (UPROBE_RUN_HANDLER , &uprobe->flags); }
@@ -897,14 +904,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumereturn ;mutex_lock (uprobes_hash (inode)); - -if (consumer_del (uprobe, uc)) { -if (!uprobe->consumers) { -__uprobe_unregister (uprobe); -clear_bit (UPROBE_RUN_HANDLER , &uprobe->flags); - } - } - +__uprobe_unregister (uprobe, uc);mutex_unlock (uprobes_hash (inode));put_uprobe (uprobe); }
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 7/7 uprobes: _register() should always do register_for_each_vma(true)
In reply to Oleg NesterovTo support the filtering uprobe_register() should do register_for_each_vma(true) every time the new consumer comes, we need to install the previously nacked breakpoints.
Note:
- uprobes_mutex[] should die, what is actually protects is alloc_uprobe().
- UPROBE_RUN_HANDLER should die too, obviously it can't work unless uprobe has a single consumer. The consumer should serialize with _register/_unregister itself. Or this flag should live in uprobe_consumer->state.
- Perhaps we can do some optimizations later. For example, if filter_chain() never returns false uprobe can record this fact and avoid the unnecessary register_for_each_vma().
--- kernel/events/uprobes.c | 31 +++++++++++++------------------ 1 files changed, 13 insertions(+), 18 deletions(-)
kernel/events/uprobes.c +13 -18
@@ -482,16 +482,12 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)up_read (&uprobe->consumer_rwsem); } -/* Returns the previous consumer */ -static struct uprobe_consumer * -consumer_add (struct uprobe *uprobe,struct uprobe_consumer *uc) +static void consumer_add (struct uprobe *uprobe,struct uprobe_consumer *uc) {down_write (&uprobe->consumer_rwsem); uc->next = uprobe->consumers; uprobe->consumers = uc;up_write (&uprobe->consumer_rwsem); - -return uc->next; }/*
@@ -820,9 +816,15 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)return err; } -static int __uprobe_register (struct uprobe *uprobe) +static int __uprobe_register (struct uprobe *uprobe,struct uprobe_consumer *uc) { -return register_for_each_vma (uprobe, true); +int err; + +consumer_add (uprobe, uc); + err =register_for_each_vma (uprobe, true); +if (!err)/* TODO: pointless unless the first consumer */ +set_bit (UPROBE_RUN_HANDLER , &uprobe->flags); +return err; }static void __uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc)
@@ -867,21 +869,14 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *if (offset >i_size_read (inode))return -EINVAL ; - ret =0 ; + ret = -ENOMEM ;mutex_lock (uprobes_hash (inode)); uprobe =alloc_uprobe (inode, offset); - -if (!uprobe) { - ret = -ENOMEM ; - }else if (!consumer_add (uprobe, uc)) { - ret =__uprobe_register (uprobe); -if (ret) { +if (uprobe) { + ret =__uprobe_register (uprobe, uc); +if (ret)__uprobe_unregister (uprobe, uc); - }else { -set_bit (UPROBE_RUN_HANDLER , &uprobe->flags); - } } -mutex_unlock (uprobes_hash (inode));if (uprobe)put_uprobe (uprobe);
Signature · 1 line
1.5.5.1
Re: [PATCH 7/7] uprobes: _register() should always do register_for_each_vma(true)
Quoted 13 lines · Oleg Nesterov
To support the filtering
uprobe_register()should doregister_for_each_vma(true)every time the new consumer comes, we need to install the previously nacked breakpoints.Note:
- uprobes_mutex[] should die, what is actually protects isalloc_uprobe().-
UPROBE_RUN_HANDLERshould die too, obviously it can't work unless uprobe has a single consumer. The consumer should serialize with _register/_unregister itself. Or this flag should live inuprobe_consumer->state.
Quoted 5 lines
- Perhaps we can do some optimizations later. For example, if
filter_chain()never returns false uprobe can record this fact and avoid the unnecessaryregister_for_each_vma().Signed-off-by Oleg Nesterov <[email protected]>
Quoted 73 lines
--- kernel/events/uprobes.c | 31 +++++++++++++------------------ 1 files changed, 13 insertions(+), 18 deletions(-)kernel/events/uprobes.c +13
-18@@ -482,16 +482,12 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)up_read (&uprobe->consumer_rwsem); } -/* Returns the previous consumer */ -static struct uprobe_consumer * -consumer_add (struct uprobe *uprobe,struct uprobe_consumer *uc) +static void consumer_add (struct uprobe *uprobe,struct uprobe_consumer *uc) {down_write (&uprobe->consumer_rwsem); uc->next = uprobe->consumers; uprobe->consumers = uc;up_write (&uprobe->consumer_rwsem); - -return uc->next; }/* @@ -820,9 +816,15 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)return err; } -static int __uprobe_register (struct uprobe *uprobe) +static int __uprobe_register (struct uprobe *uprobe,struct uprobe_consumer *uc) { -return register_for_each_vma (uprobe, true); +int err; + +consumer_add (uprobe, uc); + err =register_for_each_vma (uprobe, true); +if (!err)/* TODO: pointless unless the first consumer */ +set_bit (UPROBE_RUN_HANDLER , &uprobe->flags); +return err; }static void __uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc)@@ -867,21 +869,14 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *if (offset >i_size_read (inode))return -EINVAL ; - ret =0 ; + ret = -ENOMEM ;mutex_lock (uprobes_hash (inode)); uprobe =alloc_uprobe (inode, offset); - -if (!uprobe) { - ret = -ENOMEM ; - }else if (!consumer_add (uprobe, uc)) { - ret =__uprobe_register (uprobe); -if (ret) { +if (uprobe) { + ret =__uprobe_register (uprobe, uc); +if (ret)__uprobe_unregister (uprobe, uc); - }else { -set_bit (UPROBE_RUN_HANDLER , &uprobe->flags); - } } -mutex_unlock (uprobes_hash (inode));if (uprobe)put_uprobe (uprobe);Signature · 1 line
1.5.5.1