Re: [PATCH perf/core 03/22] uprobes: Move ref_ctr_offset update out of uprobe_write_opcode

Jiri Olsa
2025-05-08T22:56:32Z

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)