Skip to contentSkip to conversation

net_sched: gen_estimator: fix lockdep splat

PATCH Applied Eric Dumazet 3 participants 3 messages Jan 27, 2018 – Jan 29, 2018

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 net_sched: gen_estimator: fix lockdep splat

From: Eric Dumazet <[email protected]>

syzbot reported a lockdep splat in gen_new_estimator() / est_fetch_counters() when attempting to lock est->stats_lock.

Since est_fetch_counters() is called from BH context from timer interrupt, we need to block BH as well when calling it from process context.

Most qdiscs use per cpu counters and are immune to the problem, but net/sched/act_api.c and net/netfilter/xt_RATEEST.c are using a spinlock to protect their data. They both call gen_new_estimator() while object is created and not yet alive, so this bug could not trigger a deadlock, only a lockdep splat.

Fixes 1c0d32fde5bd ("net_sched: gen_estimator: complete rewrite of rate estimators")
Signed-off-by Eric Dumazet <[email protected]>
Reported-by syzbot <[email protected]>

---
 net/core/gen_estimator.c |    4 ++++

 1 file changed, 4 insertions(+)
net/core/gen_estimator.c +4 -0
index 9834cfa21b21168a7654290dc2a999e41937b534..0a3f88f08727f1f1217560407ff539c8a8c17496 100644
@@ -159,7 +159,11 @@ int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
 	est->intvl_log = intvl_log;
 	est->cpu_bstats = cpu_bstats;
 
+	if (stats_lock)
+		local_bh_disable();
 	est_fetch_counters(est, &b);
+	if (stats_lock)
+		local_bh_enable();
 	est->last_bytes = b.bytes;
 	est->last_packets = b.packets;
 	old = rcu_dereference_protected(*rate_est, 1);
Acked-by on v1 1/1
#1 lore raw
Quoted 15 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>

syzbot reported a lockdep splat in gen_new_estimator() / est_fetch_counters() when attempting to lock est->stats_lock.

Since est_fetch_counters() is called from BH context from timer interrupt, we need to block BH as well when calling it from process context.

Most qdiscs use per cpu counters and are immune to the problem, but net/sched/act_api.c and net/netfilter/xt_RATEEST.c are using a spinlock to protect their data. They both call gen_new_estimator() while object is created and not yet alive, so this bug could not trigger a deadlock, only a lockdep splat.

Quoted 3 lines
Fixes 1c0d32fde5bd ("net_sched: gen_estimator: complete rewrite of rate estimators")
Signed-off-by Eric Dumazet <[email protected]>
Reported-by syzbot <[email protected]>
Acked-by Cong Wang <[email protected]>

Nit: perhaps it is better to move the spin_lock out of est_fetch_counters() and let callers decide to use spin_lock or spin_lock_bh.

maintainer Applied
#2 lore raw
In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Sat, 27 Jan 2018 10:58:43 -0800

Quoted 15 lines

From: Eric Dumazet <[email protected]>

syzbot reported a lockdep splat in gen_new_estimator() / est_fetch_counters() when attempting to lock est->stats_lock.

Since est_fetch_counters() is called from BH context from timer interrupt, we need to block BH as well when calling it from process context.

Most qdiscs use per cpu counters and are immune to the problem, but net/sched/act_api.c and net/netfilter/xt_RATEEST.c are using a spinlock to protect their data. They both call gen_new_estimator() while object is created and not yet alive, so this bug could not trigger a deadlock, only a lockdep splat.

Quoted 3 lines
Fixes 1c0d32fde5bd ("net_sched: gen_estimator: complete rewrite of rate estimators")
Signed-off-by Eric Dumazet <[email protected]>
Reported-by syzbot <[email protected]>

Applied, thanks.