sfq: deadlock in error path

PATCH Applied Stephen Hemminger 3 participants 3 messages Feb 3, 2011

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Stephen Hemminger

author

2011-02-03 01:19 UTC #0 lore raw

PATCH 1/1 sfq: deadlock in error path

The change to allow divisor to be a parameter (in 2.6.38-rc1)
commit 817fb15dfd988d8dda916ee04fa506f0c466b9d6
introduced a possible deadlock caught by sparse.

The scheduler tree lock was left locked in the case of an incorrect divisor value. Simplest fix is to move test outside of lock which also solves problem of partial update.

Signed-off-by Stephen Hemminger <[email protected]>

net/sched/sch_sfq.c +5 -4

@@ -491,17 +491,18 @@ static int sfq_change(struct Qdisc *sch,
 	if (opt->nla_len < nla_attr_size(sizeof(*ctl)))
 		return -EINVAL;
 
+	if (ctl->divisor &&
+	    (!is_power_of_2(ctl->divisor) || ctl->divisor > 65536))
+		return -EINVAL;
+
 	sch_tree_lock(sch);
 	q->quantum = ctl->quantum ? : psched_mtu(qdisc_dev(sch));
 	q->scaled_quantum = SFQ_ALLOT_SIZE(q->quantum);
 	q->perturb_period = ctl->perturb_period * HZ;
 	if (ctl->limit)
 		q->limit = min_t(u32, ctl->limit, SFQ_DEPTH - 1);
-	if (ctl->divisor) {
-		if (!is_power_of_2(ctl->divisor) || ctl->divisor > 65536)
-			return -EINVAL;
+	if (ctl->divisor)
 		q->divisor = ctl->divisor;
-	}
 	qlen = sch->q.qlen;
 	while (sch->q.qlen > q->limit)
 		sfq_drop(sch);

Eric Dumazet

Acked-by on v1 1/1

2011-02-03 01:51 UTC #1 lore raw

Quoted 9 lines · Stephen Hemminger

The change to allow divisor to be a parameter (in 2.6.38-rc1)
commit 817fb15dfd988d8dda916ee04fa506f0c466b9d6
introduced a possible deadlock caught by sparse.
The scheduler tree lock was left locked in the case of an incorrect divisor value. Simplest fix is to move test outside of lock which also solves problem of partial update.
Signed-off-by Stephen Hemminger <[email protected]>

Oops, thanks Stephen !

Acked-by Eric Dumazet <[email protected]>

But please, please, please, please, please, remove [email protected], as this address is no longer usable for me !

David Miller

maintainer Applied

2011-02-03 04:50 UTC #2 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 03 Feb 2011 02:51:02 +0100

Quoted 9 lines · Stephen Hemminger

Quoted 8 lines
The change to allow divisor to be a parameter (in 2.6.38-rc1)
commit 817fb15dfd988d8dda916ee04fa506f0c466b9d6
introduced a possible deadlock caught by sparse.
The scheduler tree lock was left locked in the case of an incorrect divisor value. Simplest fix is to move test outside of lock which also solves problem of partial update.

Quoted 5 lines

Quoted 1 line
Signed-off-by Stephen Hemminger <[email protected]>
Oops, thanks Stephen !
Acked-by Eric Dumazet <[email protected]>

Applied, thanks.