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.
