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

[Jiri Olsa]

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)