linux-kernel - Re: [PATCH bpf-next v1 14/22] rqspinlock: Add macros for rqspinlock usage

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: <CAADnVQJ=B4cdGa+OuN7d61=LCXmQgZQz=TF+nRD55m3=2EX2cA@mail.gmail.com>
Date: Wed, 8 Jan 2025 19:30:05 -0800
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Waiman Long <llong@...hat.com>
Cc: Kumar Kartikeya Dwivedi <memxor@...il.com>, bpf <bpf@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, 
	Linus Torvalds <torvalds@...ux-foundation.org>, Peter Zijlstra <peterz@...radead.org>, 
	Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Martin KaFai Lau <martin.lau@...nel.org>, 
	Eduard Zingerman <eddyz87@...il.com>, "Paul E. McKenney" <paulmck@...nel.org>, Tejun Heo <tj@...nel.org>, 
	Barret Rhoden <brho@...gle.com>, Josh Don <joshdon@...gle.com>, Dohyun Kim <dohyunkim@...gle.com>, 
	Kernel Team <kernel-team@...a.com>
Subject: Re: [PATCH bpf-next v1 14/22] rqspinlock: Add macros for rqspinlock usage

On Wed, Jan 8, 2025 at 5:11 PM Waiman Long <llong@...hat.com> wrote:
>
>
> > Most of the users use rqspinlock because it is expected a deadlock may
> > be constructed at runtime (either due to BPF programs or by attaching
> > programs to the kernel), so lockdep splats will not be helpful on
> > debug kernels.
>
> In most cases, lockdep will report a cyclic locking dependency
> (potential deadlock) before a real deadlock happens as it requires the
> right combination of events happening in a specific sequence. So lockdep
> can report a deadlock while the runtime check of rqspinlock may not see
> it and there is no locking stall. Also rqspinlock will not see the other
> locks held in the current context.
>
>
> > Say if a mix of both qspinlock and rqspinlock were involved in an ABBA
> > situation, as long as rqspinlock is being acquired on one of the
> > threads, it will still timeout even if check_deadlock fails to
> > establish presence of a deadlock. This will mean the qspinlock call on
> > the other side will make progress as long as the kernel unwinds locks
> > correctly on failures (by handling rqspinlock errors and releasing
> > held locks on the way out).
>
> That is true only if the latest lock to be acquired is a rqspinlock. If.
> all the rqspinlocks in the circular path have already been acquired, no
> unwinding is possible.

There is no 'last lock'. If it's not an AA deadlock there are more
than 1 cpu that are spinning. In a hypothetical mix of rqspinlocks
and regular raw_spinlocks at least one cpu will be spinning on
rqspinlock and despite missing the entries in the lock table it will
still exit by timeout. The execution will continue and eventually
all locks will be released.

We considered annotating rqspinlock as trylock with
raw_spin_lock_init lock class, but usefulness is quite limited.
It's trylock only. So it may appear in a circular dependency
only if it's a combination of raw_spin_locks and rqspinlocks
which is not supposed to ever happen once we convert all bpf inner
parts to rqspinlock.
Patches 17,18,19 convert the main offenders. Few remain
that need a bit more thinking.
At the end all locks at the leaves will be rqspinlocks and
no normal locks will be taken after
(unless NMIs are doing silly things).
And since rqspinlock is a trylock, lockdep will never complain
on rqspinlock.
Even if NMI handler is buggy it's unlikely that NMI's raw_spin_lock
is in a circular dependency with rqspinlock on bpf side.
So rqspinlock entries will be adding computational
overhead to lockdep engine to filter out and not much more.

This all assumes that rqspinlocks are limited to bpf, of course.

If rqspinlock has use cases beyond bpf then, sure, let's add
trylock lockdep annotations.

Note that if there is an actual bug on bpf side with rqspinlock usage
it will be reported even when lockdep is off.
This is patch 13.
Currently it's pr_info() of held rqspinlocks and dumpstack,
but in the future we plan to make it better consumable by bpf
side. Printing into something like a special trace_pipe.
This is tbd.

> That is probably not an issue with the limited rqspinlock conversion in
> this patch series. In the future when more and more locks are converted
> to use rqspinlock, this scenario may happen.

The rqspinlock usage should be limited to bpf and no other
normal lock should be taken after.
At least that was the intent.
If folks feel that it's useful beyond bpf then we need to think harder.
lockdep annotations is an easy part to add.