Re: [PATCH 1/3] uprobes: allow put_uprobe() from non-sleepable softirq context

On Mon, Sep 9, 2024 at 10:13 PM Andrii Nakryiko
[off-list ref] wrote:

On Mon, Sep 9, 2024 at 7:52 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Mon, Sep 9, 2024 at 3:49 PM Andrii Nakryiko [off-list ref] wrote:

quoted

Currently put_uprobe() might trigger mutex_lock()/mutex_unlock(), which
makes it unsuitable to be called from more restricted context like softirq.

Let's make put_uprobe() agnostic to the context in which it is called,
and use work queue to defer the mutex-protected clean up steps.

To avoid unnecessarily increasing the size of struct uprobe, we colocate
work_struct in parallel with rb_node and rcu, both of which are unused
by the time we get to schedule clean up work.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/events/uprobes.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a2e6a57f79f2..377bd524bc8b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c

@@ -27,6 +27,7 @@
 #include <linux/shmem_fs.h>
 #include <linux/khugepaged.h>
 #include <linux/rcupdate_trace.h>
+#include <linux/workqueue.h>

 #include <linux/uprobes.h>

@@ -54,14 +55,20 @@ DEFINE_STATIC_PERCPU_RWSEM(dup_mmap_sem);
 #define UPROBE_COPY_INSN       0

 struct uprobe {
-       struct rb_node          rb_node;        /* node in the rb tree */
+       union {
+               struct {
+                       struct rb_node          rb_node;        /* node in the rb tree */
+                       struct rcu_head         rcu;
+               };
+               /* work is used only during freeing, rcu and rb_node are unused at that point */
+               struct work_struct work;
+       };
        refcount_t              ref;
        struct rw_semaphore     register_rwsem;
        struct rw_semaphore     consumer_rwsem;
        struct list_head        pending_list;
        struct list_head        consumers;
        struct inode            *inode;         /* Also hold a ref to inode */
-       struct rcu_head         rcu;
        loff_t                  offset;
        loff_t                  ref_ctr_offset;
        unsigned long           flags;

@@ -620,11 +627,28 @@ static inline bool uprobe_is_active(struct uprobe *uprobe)
        return !RB_EMPTY_NODE(&uprobe->rb_node);
 }

+static void uprobe_free_deferred(struct work_struct *work)
+{
+       struct uprobe *uprobe = container_of(work, struct uprobe, work);
+
+       /*
+        * If application munmap(exec_vma) before uprobe_unregister()
+        * gets called, we don't get a chance to remove uprobe from
+        * delayed_uprobe_list from remove_breakpoint(). Do it here.
+        */
+       mutex_lock(&delayed_uprobe_lock);
+       delayed_uprobe_remove(uprobe, NULL);
+       mutex_unlock(&delayed_uprobe_lock);
+
+       kfree(uprobe);
+}
+
 static void uprobe_free_rcu(struct rcu_head *rcu)
 {
        struct uprobe *uprobe = container_of(rcu, struct uprobe, rcu);

-       kfree(uprobe);
+       INIT_WORK(&uprobe->work, uprobe_free_deferred);
+       schedule_work(&uprobe->work);
 }

 static void put_uprobe(struct uprobe *uprobe)

It seems put_uprobe hunk was lost, since the patch is not doing
what commit log describes.

Hmm, put_uprobe() has:

call_srcu(&uprobes_srcu, &uprobe->rcu, uprobe_free_rcu);

at the end (see [0], which added that), so we do schedule_work() in
RCU callback, similarly to what we do with bpf_map freeing in the BPF
subsystem.

This patch set is based on the latest tip/perf/core (and also assuming
the RCU Tasks Trace patch that mysteriously disappeared is actually
there, hopefully it will just as magically be restored).

  [0] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=perf/core&id=8617408f7a01e94ce1f73e40a7704530e5dfb25c

I'm still not following.
put_uprobe() did delayed_uprobe_remove() and this patch is doing it again.

The commit log implies that mutex+delayed_uprobe_remove should be removed
from put_uprobe(), but that's not what the patch is doing.