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: <20250225-transparent-bronze-cobra-bafff4@leitao>
Date: Tue, 25 Feb 2025 03:46:53 -0800
From: Breno Leitao <leitao@...ian.org>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
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)

Hello Andrii,

On Mon, Feb 24, 2025 at 02:23:51PM -0800, Andrii Nakryiko wrote:
> 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.

re you suggesting that we should use an RCU read lock to protect the
"traversal" of return_instances? In other words, is it currently being
traversed unsafely, given that return_instance can be freed at any time?

> 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.

Right. kmemdup() is using GFP_KERNEL, which might sleep, so, it cannot
be called using rcu read lock.

Thanks
--breno