On Tue, Sep 10, 2024 at 8:56 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Sep 9, 2024 at 10:13 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Mon, Sep 9, 2024 at 7:52 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Mon, Sep 9, 2024 at 3:49 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > > >
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > > 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.
Ah, that part. You are right, it's my bad. I copied that lock to a
work queue callback, but didn't remove it from the put_uprobe(). Will
fix it in v2, thanks for catching!
