linux-kernel - Re: [PATCH v3 tip/perf/core 2/2] uprobes: SRCU-protect uretprobe lifetime (with timeout)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzbupJe10k0MROG5iZq6cYu6PRoN3sHhNK=L7eDLOULvNQ@mail.gmail.com>
Date: Mon, 24 Feb 2025 14:23:51 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Andrii Nakryiko <andrii@...nel.org>, linux-trace-kernel@...r.kernel.org, 
	peterz@...radead.org, oleg@...hat.com, rostedt@...dmis.org, 
	mhiramat@...nel.org, mingo@...nel.org, bpf@...r.kernel.org, 
	linux-kernel@...r.kernel.org, jolsa@...nel.org, paulmck@...nel.org
Subject: Re: [PATCH v3 tip/perf/core 2/2] uprobes: SRCU-protect uretprobe
 lifetime (with timeout)

On Mon, Feb 24, 2025 at 4:23 AM Breno Leitao <leitao@...ian.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