On Sun, Jul 7, 2024 at 7:48 AM Oleg Nesterov [off-list ref] wrote:

And I forgot to mention...

In any case __uprobe_unregister() can't ignore the error code from
register_for_each_vma(). If it fails to restore the original insn,
we should not remove this uprobe from uprobes_tree.

Otherwise the next handle_swbp() will send SIGTRAP to the (no longer)
probed application.

Yep, that would be unfortunate (just like SIGILL sent when uretprobe
detects "improper" stack pointer progression, for example), but from
what I gather it's not really expected to fail on unregistration given
we successfully registered uprobe. I guess it's a decision between
leaking memory with an uprobe stuck in the tree or killing process due
to some very rare (or buggy) condition?

On 07/05, Oleg Nesterov wrote:

quoted

Tried to read this patch, but I fail to understand it. It looks
obvioulsy wrong to me, see below.

I tend to agree with the comments from Peter, but lets ignore them
for the moment.

On 07/01, Andrii Nakryiko wrote:

quoted

 static void put_uprobe(struct uprobe *uprobe)
 {
-   if (refcount_dec_and_test(&uprobe->ref)) {
+   s64 v;
+
+   /*
+    * here uprobe instance is guaranteed to be alive, so we use Tasks
+    * Trace RCU to guarantee that uprobe won't be freed from under us, if
+    * we end up being a losing "destructor" inside uprobe_treelock'ed
+    * section double-checking uprobe->ref value below.
+    * Note call_rcu_tasks_trace() + uprobe_free_rcu below.
+    */
+   rcu_read_lock_trace();
+
+   v = atomic64_add_return(UPROBE_REFCNT_PUT, &uprobe->ref);
+
+   if (unlikely((u32)v == 0)) {

I must have missed something, but how can this ever happen?

Suppose uprobe_register(inode) is called the 1st time. To simplify, suppose
that this binary is not used, so _register() doesn't install breakpoints/etc.

IIUC, with this change (u32)uprobe->ref == 1 when uprobe_register() succeeds.

Now suppose that uprobe_unregister() is called right after that. It does

      uprobe = find_uprobe(inode, offset);

this increments the counter, (u32)uprobe->ref == 2

      __uprobe_unregister(...);

this wont't change the counter,

      put_uprobe(uprobe);

this drops the reference added by find_uprobe(), (u32)uprobe->ref == 1.

Where should the "final" put_uprobe() come from?

IIUC, this patch lacks another put_uprobe() after consumer_del(), no?

Oleg.