On Mon, Feb 24, 2025 at 4:23 AM Breno Leitao <lei...@debian.org> wrote:
>
> Hello Andrii,
>
> On Wed, Oct 23, 2024 at 09:41:59PM -0700, Andrii Nakryiko wrote:
> >
> > +static struct uprobe* hprobe_expire(struct hprobe *hprobe, bool get)
> > +{
> > + enum hprobe_state hstate;
> > +
> > + /*
> > + * return_instance's hprobe is protected by RCU.
> > + * Underlying uprobe is itself protected from reuse by SRCU.
> > + */
> > + lockdep_assert(rcu_read_lock_held() &&
> > srcu_read_lock_held(&uretprobes_srcu));
>
> I am hitting this warning in d082ecbc71e9e ("Linux 6.14-rc4") on
> aarch64. I suppose this might happen on x86 as well, but I haven't
> tested.
>
> WARNING: CPU: 28 PID: 158906 at kernel/events/uprobes.c:768
> hprobe_expire (kernel/events/uprobes.c:825)
>
> Call trace:
> hprobe_expire (kernel/events/uprobes.c:825) (P)
> uprobe_copy_process (kernel/events/uprobes.c:691
> kernel/events/uprobes.c:2103 kernel/events/uprobes.c:2142)
> copy_process (kernel/fork.c:2636)
> kernel_clone (kernel/fork.c:2815)
> __arm64_sys_clone (kernel/fork.c:? kernel/fork.c:2926
> kernel/fork.c:2926)
> invoke_syscall (arch/arm64/kernel/syscall.c:35
> arch/arm64/kernel/syscall.c:49)
> do_el0_svc (arch/arm64/kernel/syscall.c:139
> arch/arm64/kernel/syscall.c:151)
> el0_svc (arch/arm64/kernel/entry-common.c:165
> arch/arm64/kernel/entry-common.c:178 arch/arm64/kernel/entry-common.c:745)
> el0t_64_sync_handler (arch/arm64/kernel/entry-common.c:797)
> el0t_64_sync (arch/arm64/kernel/entry.S:600)
>
> I broke down that warning, and the problem is on related to
> rcu_read_lock_held(), since RCU read lock does not seem to be held in
> this path.
>
> Reading this code, RCU read lock seems to protect old hprobe, which
> doesn't seem so.
>
> I am wondering if we need to protect it properly, something as:
>
> @@ -2089,7 +2092,9 @@ static int dup_utask(struct task_struct *t,
> struct uprobe_task *o_utask)
> return -ENOMEM;
>
> /* if uprobe is non-NULL, we'll have an extra
> refcount for uprobe */
> + rcu_read_lock();
> uprobe = hprobe_expire(&o->hprobe, true);
> + rcu_write_lock();
>
I think this is not good enough. rcu_read_lock/unlock should be around
the entire for loop, because, technically, that return_instance can be
freed before we even get to hprobe_expire.
So, just like we have guard(srcu)(&uretprobes_srcu); we should have
guard(rcu)();
Except, there is that kmemdup() hidden inside dup_return_instance(),
so we can't really do that.
So that leaves us with an option to split memory allocation and
initialization. Number of return instances can't grow (but can
shrink), so we can first count them, allocate memory. Then do another
iteration to do hprobe_expire().
I'll try to send a patch some time in the next day or two, but maybe
someone has some better ideas meanwhile.
> /*
> * New utask will have stable properly refcounted
> uprobe or
