linux-kernel - Re: [PATCH v4 0/3] sched_ext: Harden scx_bpf_cpu

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] [day] [month] [year] [list]

Message-ID: <aJtDU5ZSDb5hwEan@gpd4>
Date: Tue, 12 Aug 2025 15:36:19 +0200
From: Andrea Righi <arighi@...dia.com>
To: Christian Loehle <christian.loehle@....com>
Cc: Peter Zijlstra <peterz@...radead.org>, tj@...nel.org,
	void@...ifault.com, linux-kernel@...r.kernel.org,
	sched-ext@...ts.linux.dev, changwoo@...lia.com, hodgesd@...a.com,
	mingo@...hat.com, jake@...lion.co.uk
Subject: Re: [PATCH v4 0/3] sched_ext: Harden scx_bpf_cpu_rq()

On Tue, Aug 12, 2025 at 12:40:39PM +0100, Christian Loehle wrote:
> On 8/12/25 09:00, Peter Zijlstra wrote:
> > On Mon, Aug 11, 2025 at 10:21:47PM +0100, Christian Loehle wrote:
> >> scx_bpf_cpu_rq() currently allows accessing struct rq fields without
> >> holding the associated rq.
> >> It is being used by scx_cosmos, scx_flash, scx_lavd, scx_layered, and
> >> scx_tickless. Fortunately it is only ever used to fetch rq->curr.
> >> So provide an alternative scx_bpf_task_acquire_remote_curr() that
> >> doesn't expose struct rq and provide a hardened scx_bpf_cpu_rq_locked()
> >> by ensuring we hold the rq lock.
> >> Add a deprecation warning to scx_bpf_cpu_rq_locked() that mentions the
> >> two alternatives.
> >>
> >> This also simplifies scx code from:
> >>
> >> rq = scx_bpf_cpu_rq(cpu);
> >> if (!rq)
> >> 	return;
> >> p = rq->curr
> >> if (!p)
> >> 	return;
> >> /* ... Do something with p */
> >>
> >> into:
> >>
> >> p = scx_bpf_task_acquire_remote_curr(cpu);
> >> if (!p)
> >> 	return;
> >> /* ... Do something with p */
> >> bpf_task_release(p);
> > 
> > Why do that mandatory refcount dance, rather than directly exposing the
> > RCU-ness of that pointer? IIRC BPF was good with RCU.
> 
> Just because that's how
> bpf_task_from_pid()
> bpf_task_from_vpid()
> already work. I have no strong preference either way.
> Apart from the above just returning
> rcu_dereference(cpu_rq(cpu)->curr);
> is obviously a bit less cumbersome (and yes, RCUs are exposed to BPF,
> for scx most callbacks have that implicit anyway.)
> 
> I'll change it to scx_bpf_remote_curr() that does that in the next
> version, thanks!

Yeah, I suggested Christian to do the refcount dance to be consistent with
bpf_task_from_[v]pid(), but we can probably mark the kfunc as KF_RCU and
rely on RCU locking to save a bit of overhead. So, it's probably better to
follow Peter's suggestion.

Thanks,
-Andrea