[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aB02m4ZdPGJOWatx@krava>
Date: Fri, 9 May 2025 00:56:27 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Jiri Olsa <olsajiri@...il.com>, Peter Zijlstra <peterz@...radead.org>,
Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
x86@...nel.org, Song Liu <songliubraving@...com>,
Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
Hao Luo <haoluo@...gle.com>, Steven Rostedt <rostedt@...dmis.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Alan Maguire <alan.maguire@...cle.com>,
David Laight <David.Laight@...lab.com>,
Thomas Weißschuh <thomas@...ch.de>,
Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH perf/core 03/22] uprobes: Move ref_ctr_offset update out
of uprobe_write_opcode
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)
Powered by blists - more mailing lists