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

[Oleg Nesterov <oleg@redhat.com>]

I am still traveling, will actually read your email when I get back...

On 05/09, Jiri Olsa wrote:
>
> On Tue, May 06, 2025 at 04:01:45PM +0200, Oleg Nesterov wrote:
> >
> > - 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;
>         }

OK,

> 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 guess _unregister() should not fail "in practice" after
get_user_page + verify_opcode, yet I think we should not rely on this, if possible.

But I won't argue if you think we can ignore this "impossible" failures, just
this should be documented. Same for update_ref_ctr(), iirc it should "never"
fail if ref_offset is correct.

> --- 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);

Yes, this is what I meant.

Oleg.