On Tue, May 06, 2025 at 04:01:45PM +0200, Oleg Nesterov wrote:
> I'm on PTO and traveling until May 15 without my working laptop, can't read
> the code.
>
> Quite possibly I am wrong, but let me try to recall what this code does...
>
> - So. uprobe_register() succeeds and changes ref_ctr from 0 to 1.
>
> - uprobe_unregister() fails but decrements ref_ctr back to zero. Because the
> "Revert back reference counter if instruction update failed" logic doesn't
> apply if is_register is true.
>
> Since uprobe_unregister() fails, this uprobe won't be removed. IIRC, we even
> have the warning about that.
>
> - another uprobe_register() comes and re-uses the same uprobe. In this case
> install_breakpoint() will do nothing, ref_ctr won't be updated (right ?)
right, because int3 is still in place and verify_opcode returns 0
>
> - uprobe_unregister() is called again and this time it succeeds. In this case
> ref_ctr is changed from 0 to -1. IIRC, we even have some warning for this
> case.
AFAICS that should not happen, there's check below in __update_ref_ctr:
if (unlikely(*ptr + d < 0)) {
pr_warn("ref_ctr going negative. vaddr: 0x%lx, "
"curr val: %d, delta: %d\n", vaddr, *ptr, d);
ret = -EINVAL;
goto out;
}
*ptr += d;
ret = 0;
...
but it still prevents the uprobe from 2nd register to trigger,
so I think the change you suggest makes sense
few things first..
- how do you make uprobe_unregister fail after succesful uprobe_register?
I had to instrument the code to do that for me
- I see one extra uprobe_write_opcode call during unregister (check below)
seems it does no harm, but looks strange
current code:
1st register:
- uprobe_register succeeds and changes ref_ctr_offset from 0 to 1
1st unregister:
- first there's uprobe_perf_close -> uprobe_apply call that ends up in
remove_breakpoint call that will decrement ref_ctr_offset to 0 and fail
- followed by __probe_event_disable -> uprobe_unregister_nosync call
that ends up in remove_breakpoint call that will fail to decrement
ref_ctr_offset to -1 (and ref_ctr_offset stays 0) and fail
- uprobe is leaked
2nd register:
- another uprobe_register() comes and re-uses the same uprobe. In this case
install_breakpoint() will do nothing, ref_ctr won't be updated, stays 0
so uprobe WILL NOT trigger
2nd unregister:
- both attempts (from uprobe_perf_close and __probe_event_disable as above)
to write original instruction will fail, because ref_ctr_offset
update fails and uprobe_write_opcode bails out
with the attached change we will do:
1st register:
- uprobe_register succeeds and changes ref_ctr_offset from 0 to 1
1st unregister:
- first there's uprobe_perf_close -> uprobe_apply call that ends up in
remove_breakpoint call that will decrement ref_ctr_offset to 0 and fail
and restore ref_ctr_offset to 1
- followed by __probe_event_disable -> uprobe_unregister_nosync call
that ends up in remove_breakpoint call that will do the same as
previous step, ref_ctr_offset is 1
- uprobe is leaked
2nd register:
- another uprobe_register() comes and re-uses the same uprobe. In this case
install_breakpoint() will do nothing, ref_ctr won't be updated, stays 1,
so uprobe WILL trigger
2nd unregister:
- succeeds, and ref_ctr_offset is 0
jirka
---
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 207432e92386..65bfe52ed729 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -589,8 +589,8 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct
vm_area_struct *vma,
out:
/* Revert back reference counter if instruction update failed. */
- if (ret < 0 && is_register && ref_ctr_updated)
- update_ref_ctr(uprobe, mm, -1);
+ if (ret < 0 && ref_ctr_updated)
+ update_ref_ctr(uprobe, mm, is_register ? -1 : 1);
/* try collapse pmd for compound page */
if (ret > 0)
