Re: BUG: scheduling while atomic with PREEMPT_RT=y and bpf selftests

[Bert Karwatzki <spasswolf@web.de>]

These three patches fixes all the dmesg warning (with CONFIG_LOCKDEP) issues when running the
bpf test_progs and does not cause deadlocks without CONFIG_LOCKDEP.

The warning when running the cgroup examples 

[ T2916] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
[ T2916] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 2916, name: test_progs
[ T2916] preempt_count: 1, expected: 0
[ T2916] RCU nest depth: 2, expected: 2
[...]
[ T2916]  __might_resched.cold+0xfa/0x135
[ T2916]  rt_spin_lock+0x5f/0x190
[ T2916]  ? task_get_cgroup1+0xe8/0x340
[ T2916]  task_get_cgroup1+0xe8/0x340
[ T2916]  bpf_task_get_cgroup1+0xe/0x20
[ T2916]  bpf_prog_8d22669ef1ee8049_on_enter+0x62/0x1d4
[ T2916]  bpf_trace_run2+0xd3/0x260

is fixed by this:

diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
index 183fa2aa2935..49257cb90209 100644
--- a/include/trace/bpf_probe.h
+++ b/include/trace/bpf_probe.h
@@ -58,9 +58,9 @@ static notrace void \
 __bpf_trace_##call(void *__data, proto) \
 { \
 might_fault(); \
- preempt_disable_notrace(); \
+ migrate_disable(); \
 CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args)); \
- preempt_enable_notrace(); \
+ migrate_enable(); \
 }
 
 #undef DECLARE_EVENT_SYSCALL_CLASS


The warning when running the (wq, timer, free_timer, timer_mim, timer_lockup) examples

[ T4696] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
[ T4696] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 4696, name: test_progs
[ T4696] preempt_count: 1, expected: 0
[ T4696] RCU nest depth: 0, expected: 0
[...]
[ T4696]  __kmalloc_node_noprof+0xee/0x490
[ T4696]  bpf_map_kmalloc_node+0x72/0x220
[ T4696]  __bpf_async_init+0x107/0x310
[ T4696]  bpf_timer_init+0x33/0x40
[ T4696]  bpf_prog_7e15f1bc7d1d26d0_start_cb+0x5d/0x91
[ T4696]  bpf_prog_d85f43676fabf521_start_timer+0x65/0x8a
[ T4696]  bpf_prog_test_run_syscall+0x103/0x250

is fixed by this (this might leak
memory due to the allcoation before the lock(?))

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index e3a2662f4e33..94fcd8c8661c 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1263,19 +1263,16 @@ static int __bpf_async_init(struct bpf_async_kern *async, struct bpf_map *map, u
 return -EINVAL;
 }
 
- __bpf_spin_lock_irqsave(&async->lock);
 t = async->timer;
- if (t) {
- ret = -EBUSY;
- goto out;
- }
+ if (t)
+ return -EBUSY;
 
 /* allocate hrtimer via map_kmalloc to use memcg accounting */
 cb = bpf_map_kmalloc_node(map, size, GFP_ATOMIC, map->numa_node);
- if (!cb) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!cb)
+ return -ENOMEM;
+
+ __bpf_spin_lock_irqsave(&async->lock);
 
 switch (type) {
 case BPF_ASYNC_TYPE_TIMER:
@@ -1315,7 +1312,6 @@ static int __bpf_async_init(struct bpf_async_kern *async, struct bpf_map *map, u
 kfree(cb);
 ret = -EPERM;
 }
-out:
 __bpf_spin_unlock_irqrestore(&async->lock);
 return ret;
 }

In addition to these there's a deadlock warning from lockdep when running the
timer_lockup example

[  127.373597] [      C1] ============================================
[  127.373598] [      C1] WARNING: possible recursive locking detected
[  127.373601] [      C1] 6.15.0-bpf-00006-g31cf22212ed9 #41 Tainted: G           O      
[  127.373602] [      C1] --------------------------------------------
[  127.373603] [      C1] ktimers/1/85 is trying to acquire lock:
[  127.373605] [      C1] ffff98f62e61c1b8 (&base->softirq_expiry_lock){+...}-{3:3}, at: hrtimer_cancel_wait_running+0x4d/0x80
[  127.373614] [      C1]
                          but task is already holding lock:
[  127.373615] [      C1] ffff98f62e65c1b8 (&base->softirq_expiry_lock){+...}-{3:3}, at: hrtimer_run_softirq+0x37/0x100
[  127.373621] [      C1]
                          other info that might help us debug this:
[  127.373621] [      C1]  Possible unsafe locking scenario:

[  127.373622] [      C1]        CPU0
[  127.373623] [      C1]        ----
[  127.373624] [      C1]   lock(&base->softirq_expiry_lock);
[  127.373626] [      C1]   lock(&base->softirq_expiry_lock);
[  127.373627] [      C1]
                           *** DEADLOCK ***

[  127.373628] [      C1]  May be due to missing lock nesting notation

which can by fixed (?) by reintroducing spin_lock_bh_nested()
(which was introduced in v4.0 and removed in v4.11 ...)

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index d3561c4a080e..b6635466b35f 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -219,6 +219,8 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 # define raw_spin_lock_nested(lock, subclass) \
 _raw_spin_lock_nested(lock, subclass)
+# define raw_spin_lock_bh_nested(lock, subclass) \
+ _raw_spin_lock_bh_nested(lock, subclass)
 
 # define raw_spin_lock_nest_lock(lock, nest_lock) \
 do { \
@@ -233,6 +235,8 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
  */
 # define raw_spin_lock_nested(lock, subclass) \
 _raw_spin_lock(((void)(subclass), (lock)))
+# define raw_spin_lock_bh_nested(lock, subclass) \
+ _raw_spin_lock_bh(((void)(subclass), (lock)))
 # define raw_spin_lock_nest_lock(lock, nest_lock) _raw_spin_lock(lock)
 #endif
 
@@ -366,6 +370,11 @@ do { \
 raw_spin_lock_nested(spinlock_check(lock), subclass); \
 } while (0)
 
+#define spin_lock_bh_nested(lock, subclass) \
+do { \
+ raw_spin_lock_bh_nested(spinlock_check(lock), subclass); \
+} while (0)
+
 #define spin_lock_nest_lock(lock, nest_lock) \
 do { \
 raw_spin_lock_nest_lock(spinlock_check(lock), nest_lock); \
@@ -552,6 +561,10 @@ DEFINE_LOCK_GUARD_1(raw_spinlock_bh, raw_spinlock_t,
 raw_spin_lock_bh(_T->lock),
 raw_spin_unlock_bh(_T->lock))
 
+DEFINE_LOCK_GUARD_1(raw_spinlock_bh_nested, raw_spinlock_t,
+ raw_spin_lock_bh_nested(_T->lock, SINGLE_DEPTH_NESTING),
+ raw_spin_unlock_bh(_T->lock))
+
 DEFINE_LOCK_GUARD_1_COND(raw_spinlock_bh, _try, raw_spin_trylock_bh(_T->lock))
 
 DEFINE_LOCK_GUARD_1(raw_spinlock_irqsave, raw_spinlock_t,
diff --git a/include/linux/spinlock_api_smp.h b/include/linux/spinlock_api_smp.h
index 9ecb0ab504e3..791867efad04 100644
--- a/include/linux/spinlock_api_smp.h
+++ b/include/linux/spinlock_api_smp.h
@@ -22,6 +22,8 @@ int in_lock_functions(unsigned long addr);
 void __lockfunc _raw_spin_lock(raw_spinlock_t *lock) __acquires(lock);
 void __lockfunc _raw_spin_lock_nested(raw_spinlock_t *lock, int subclass)
 __acquires(lock);
+void __lockfunc _raw_spin_lock_bh_nested(raw_spinlock_t *lock, int subclass)
+ __acquires(lock);
 void __lockfunc
 _raw_spin_lock_nest_lock(raw_spinlock_t *lock, struct lockdep_map *map)
 __acquires(lock);
diff --git a/include/linux/spinlock_api_up.h b/include/linux/spinlock_api_up.h
index 819aeba1c87e..a42887a0c846 100644
--- a/include/linux/spinlock_api_up.h
+++ b/include/linux/spinlock_api_up.h
@@ -57,6 +57,7 @@
 
 #define _raw_spin_lock(lock) __LOCK(lock)
 #define _raw_spin_lock_nested(lock, subclass) __LOCK(lock)
+#define _raw_spin_lock_bh_nested(lock, subclass) __LOCK(lock)
 #define _raw_read_lock(lock) __LOCK(lock)
 #define _raw_write_lock(lock) __LOCK(lock)
 #define _raw_write_lock_nested(lock, subclass) __LOCK(lock)
diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h
index f6499c37157d..b89dc9e272cc 100644
--- a/include/linux/spinlock_rt.h
+++ b/include/linux/spinlock_rt.h
@@ -88,6 +88,13 @@ static __always_inline void spin_lock_bh(spinlock_t *lock)
 rt_spin_lock(lock);
 }
 
+static __always_inline void spin_lock_bh_nested(spinlock_t *lock, int subclass)
+{
+ /* Investigate: Drop bh when blocking ? */
+ local_bh_disable();
+ rt_spin_lock_nested(lock, subclass);
+}
+
 static __always_inline void spin_lock_irq(spinlock_t *lock)
 {
 rt_spin_lock(lock);
diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c
index 7685defd7c52..c4aa80441a1d 100644
--- a/kernel/locking/spinlock.c
+++ b/kernel/locking/spinlock.c
@@ -380,6 +380,14 @@ void __lockfunc _raw_spin_lock_nested(raw_spinlock_t *lock, int subclass)
 }
 EXPORT_SYMBOL(_raw_spin_lock_nested);
 
+void __lockfunc _raw_spin_lock_bh_nested(raw_spinlock_t *lock, int subclass)
+{
+ __local_bh_disable_ip(_RET_IP_, SOFTIRQ_LOCK_OFFSET);
+ spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
+ LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
+}
+EXPORT_SYMBOL(_raw_spin_lock_bh_nested);
+
 unsigned long __lockfunc _raw_spin_lock_irqsave_nested(raw_spinlock_t *lock,
 int subclass)
 {
diff --git a/kernel/locking/spinlock_rt.c b/kernel/locking/spinlock_rt.c
index db1e11b45de6..5b76072c4838 100644
--- a/kernel/locking/spinlock_rt.c
+++ b/kernel/locking/spinlock_rt.c
@@ -58,7 +58,6 @@ void __sched rt_spin_lock(spinlock_t *lock) __acquires(RCU)
 }
 EXPORT_SYMBOL(rt_spin_lock);
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
 void __sched rt_spin_lock_nested(spinlock_t *lock, int subclass)
 {
 spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
@@ -66,6 +65,7 @@ void __sched rt_spin_lock_nested(spinlock_t *lock, int subclass)
 }
 EXPORT_SYMBOL(rt_spin_lock_nested);
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
 void __sched rt_spin_lock_nest_lock(spinlock_t *lock,
 struct lockdep_map *nest_lock)
 {
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 30899a8cc52c..d62455f62fc4 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1457,7 +1457,7 @@ void hrtimer_cancel_wait_running(const struct hrtimer *timer)
 * unlikely and just causes another wait loop.
 */
 atomic_inc(&base->cpu_base->timer_waiters);
- spin_lock_bh(&base->cpu_base->softirq_expiry_lock);
+ spin_lock_bh_nested(&base->cpu_base->softirq_expiry_lock, SINGLE_DEPTH_NESTING);
 atomic_dec(&base->cpu_base->timer_waiters);
 spin_unlock_bh(&base->cpu_base->softirq_expiry_lock);
 }
diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/tools/testing/selftests/bpf/progs/dynptr_success.c
index e1fba28e4a86..7cfb9473a526 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -7,6 +7,7 @@
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
 #include "bpf_misc.h"
+#include "bpf_kfuncs.h"
 #include "errno.h"
 
 char _license[] SEC("license") = "GPL";


Bert Karwatzki