[PATCH net-next 0/4] net/sched: sch_htb: first round of fixes

[PATCH net-next 0/4] net/sched: sch_htb: first round of fixes

[Eric Dumazet]

First round of fixes in sch_htb.

I chose to send a small series, to reduce chances of multiple versions.

Eric Dumazet (4):
  net/sched: sch_htb: do not change sch->flags in htb_dump()
  net/sched: sch_htb: annotate data-races (I)
  net/sched: sch_htb: annotate data-races (II)
  net/sched: sch_htb: annotate data-races (III)

 net/sched/sch_htb.c | 54 +++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

-- 
2.54.0.563.g4f69b47b94-goog

[PATCH net-next 1/4] net/sched: sch_htb: do not change sch->flags in htb_dump()

[Eric Dumazet]

htb_dump() runs without holding qdisc spinlock.

It is illegal to touch sch->flags with non locked RMW,
as concurrent readers might see intermediate wrong values.

Set TCQ_F_OFFLOADED in control path (htb_init()) instead.

Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/sch_htb.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 1e600f65c8769a74286c4f060b0d45da9a13eeeb..873337ac1cca28ea1a32778d8e91cacb5bd3af9c 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1147,6 +1147,7 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
 	 * parts (especially calling ndo_setup_tc) on errors.
 	 */
 	q->offload = true;
+	sch->flags |= TCQ_F_OFFLOADED;
 
 	return 0;
 }
@@ -1207,11 +1208,6 @@ static int htb_dump(struct Qdisc *sch, struct sk_buff *skb)
 	struct nlattr *nest;
 	struct tc_htb_glob gopt;
 
-	if (q->offload)
-		sch->flags |= TCQ_F_OFFLOADED;
-	else
-		sch->flags &= ~TCQ_F_OFFLOADED;
-
 	sch->qstats.overlimits = q->overlimits;
 	/* Its safe to not acquire qdisc lock. As we hold RTNL,
 	 * no change can happen on the qdisc parameters.
-- 
2.54.0.563.g4f69b47b94-goog

[PATCH net-next 2/4] net/sched: sch_htb: annotate data-races (I)

[Eric Dumazet]

htb_dump() runs without holding qdisc spinlock.

Add missing READ_ONCE()/WRITE_ONCE() annotations around
q->overlimits and q->direct_pkts.

Fixes: edb09eb17ed8 ("net: sched: do not acquire qdisc spinlock in qdisc/class stats dump")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/sch_htb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 873337ac1cca28ea1a32778d8e91cacb5bd3af9c..5bf1889bc0300e8e3a61d56ddcf1bb1402955107 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -568,7 +568,7 @@ htb_change_class_mode(struct htb_sched *q, struct htb_class *cl, s64 *diff)
 
 	if (new_mode == HTB_CANT_SEND) {
 		cl->overlimits++;
-		q->overlimits++;
+		WRITE_ONCE(q->overlimits, q->overlimits + 1);
 	}
 
 	if (cl->prio_activity) {	/* not necessary: speed optimization */
@@ -628,7 +628,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		/* enqueue to helper queue */
 		if (q->direct_queue.qlen < q->direct_qlen) {
 			__qdisc_enqueue_tail(skb, &q->direct_queue);
-			q->direct_pkts++;
+			WRITE_ONCE(q->direct_pkts, q->direct_pkts + 1);
 		} else {
 			return qdisc_drop(skb, sch, to_free);
 		}
@@ -1208,12 +1208,12 @@ static int htb_dump(struct Qdisc *sch, struct sk_buff *skb)
 	struct nlattr *nest;
 	struct tc_htb_glob gopt;
 
-	sch->qstats.overlimits = q->overlimits;
+	sch->qstats.overlimits = READ_ONCE(q->overlimits);
 	/* Its safe to not acquire qdisc lock. As we hold RTNL,
 	 * no change can happen on the qdisc parameters.
 	 */
 
-	gopt.direct_pkts = q->direct_pkts;
+	gopt.direct_pkts = READ_ONCE(q->direct_pkts);
 	gopt.version = HTB_VER;
 	gopt.rate2quantum = q->rate2quantum;
 	gopt.defcls = q->defcls;
-- 
2.54.0.563.g4f69b47b94-goog

[PATCH net-next 3/4] net/sched: sch_htb: annotate data-races (II)

[Eric Dumazet]

htb_dump_class_stats() will soon run locklessly.
(no RTNL, not qdisc spinlock).

Remove cl->xstats and replace it with two fields:

-	xstats_lends
-	xstats_borrows

Then use READ_ONCE()/WRITE_ONCE() annotations on them, and change
htb_dump_class_stats to use a private struct tc_htb_xstats.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/sch_htb.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 5bf1889bc0300e8e3a61d56ddcf1bb1402955107..353467eb7611be45c6a9c92a0dcd4357d136a232 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -114,7 +114,8 @@ struct htb_class {
 	 */
 	struct gnet_stats_basic_sync bstats;
 	struct gnet_stats_basic_sync bstats_bias;
-	struct tc_htb_xstats	xstats;	/* our special stats */
+	u32			xstats_lends;
+	u32			xstats_borrows;
 
 	/* token bucket parameters */
 	s64			tokens, ctokens;/* current number of tokens */
@@ -707,10 +708,10 @@ static void htb_charge_class(struct htb_sched *q, struct htb_class *cl,
 		diff = min_t(s64, q->now - cl->t_c, cl->mbuffer);
 		if (cl->level >= level) {
 			if (cl->level == level)
-				cl->xstats.lends++;
+				WRITE_ONCE(cl->xstats_lends, cl->xstats_lends + 1);
 			htb_accnt_tokens(cl, bytes, diff);
 		} else {
-			cl->xstats.borrows++;
+			WRITE_ONCE(cl->xstats_borrows, cl->xstats_borrows + 1);
 			cl->tokens += diff;	/* we moved t_c; update tokens */
 		}
 		htb_accnt_ctokens(cl, bytes, diff);
@@ -1319,6 +1320,10 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 {
 	struct htb_class *cl = (struct htb_class *)arg;
 	struct htb_sched *q = qdisc_priv(sch);
+	struct tc_htb_xstats xstats = {
+		.lends = READ_ONCE(cl->xstats_lends),
+		.borrows = READ_ONCE(cl->xstats_borrows),
+	};
 	struct gnet_stats_queue qs = {
 		.drops = cl->drops,
 		.overlimits = cl->overlimits,
@@ -1328,10 +1333,10 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 	if (!cl->level && cl->leaf.q)
 		qdisc_qstats_qlen_backlog(cl->leaf.q, &qlen, &qs.backlog);
 
-	cl->xstats.tokens = clamp_t(s64, PSCHED_NS2TICKS(cl->tokens),
-				    INT_MIN, INT_MAX);
-	cl->xstats.ctokens = clamp_t(s64, PSCHED_NS2TICKS(cl->ctokens),
-				     INT_MIN, INT_MAX);
+	xstats.tokens = clamp_t(s64, PSCHED_NS2TICKS(cl->tokens),
+				INT_MIN, INT_MAX);
+	xstats.ctokens = clamp_t(s64, PSCHED_NS2TICKS(cl->ctokens),
+				 INT_MIN, INT_MAX);
 
 	if (q->offload) {
 		if (!cl->level) {
@@ -1352,7 +1357,7 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 	    gnet_stats_copy_queue(d, NULL, &qs, qlen) < 0)
 		return -1;
 
-	return gnet_stats_copy_app(d, &cl->xstats, sizeof(cl->xstats));
+	return gnet_stats_copy_app(d, &xstats, sizeof(xstats));
 }
 
 static struct netdev_queue *
-- 
2.54.0.563.g4f69b47b94-goog

[PATCH net-next 4/4] net/sched: sch_htb: annotate data-races (III)

[Eric Dumazet]

htb_dump_class_stats() will soon run locklessly.

(no RTNL, not qdisc spinlock).

Add READ_ONCE()/WRITE_ONCE() annotations on these fields:

- cl->overlimits
- cl->drops
- cl->tokens
- cl->ctokens

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/sch_htb.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 353467eb7611be45c6a9c92a0dcd4357d136a232..d8ef3efbe0d5eab42c535e48d95d9005ac9682be 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -568,7 +568,7 @@ htb_change_class_mode(struct htb_sched *q, struct htb_class *cl, s64 *diff)
 		return;
 
 	if (new_mode == HTB_CANT_SEND) {
-		cl->overlimits++;
+		WRITE_ONCE(cl->overlimits, cl->overlimits + 1);
 		WRITE_ONCE(q->overlimits, q->overlimits + 1);
 	}
 
@@ -644,7 +644,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 					to_free)) != NET_XMIT_SUCCESS) {
 		if (net_xmit_drop_count(ret)) {
 			qdisc_qstats_drop(sch);
-			cl->drops++;
+			WRITE_ONCE(cl->drops, cl->drops + 1);
 		}
 		return ret;
 	} else {
@@ -666,7 +666,7 @@ static inline void htb_accnt_tokens(struct htb_class *cl, int bytes, s64 diff)
 	if (toks <= -cl->mbuffer)
 		toks = 1 - cl->mbuffer;
 
-	cl->tokens = toks;
+	WRITE_ONCE(cl->tokens, toks);
 }
 
 static inline void htb_accnt_ctokens(struct htb_class *cl, int bytes, s64 diff)
@@ -679,7 +679,7 @@ static inline void htb_accnt_ctokens(struct htb_class *cl, int bytes, s64 diff)
 	if (toks <= -cl->mbuffer)
 		toks = 1 - cl->mbuffer;
 
-	cl->ctokens = toks;
+	WRITE_ONCE(cl->ctokens, toks);
 }
 
 /**
@@ -712,7 +712,8 @@ static void htb_charge_class(struct htb_sched *q, struct htb_class *cl,
 			htb_accnt_tokens(cl, bytes, diff);
 		} else {
 			WRITE_ONCE(cl->xstats_borrows, cl->xstats_borrows + 1);
-			cl->tokens += diff;	/* we moved t_c; update tokens */
+			/* we moved t_c; update tokens */
+			WRITE_ONCE(cl->tokens, cl->tokens + diff);
 		}
 		htb_accnt_ctokens(cl, bytes, diff);
 		cl->t_c = q->now;
@@ -1325,17 +1326,17 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 		.borrows = READ_ONCE(cl->xstats_borrows),
 	};
 	struct gnet_stats_queue qs = {
-		.drops = cl->drops,
-		.overlimits = cl->overlimits,
+		.drops = READ_ONCE(cl->drops),
+		.overlimits = READ_ONCE(cl->overlimits),
 	};
 	__u32 qlen = 0;
 
 	if (!cl->level && cl->leaf.q)
 		qdisc_qstats_qlen_backlog(cl->leaf.q, &qlen, &qs.backlog);
 
-	xstats.tokens = clamp_t(s64, PSCHED_NS2TICKS(cl->tokens),
+	xstats.tokens = clamp_t(s64, PSCHED_NS2TICKS(READ_ONCE(cl->tokens)),
 				INT_MIN, INT_MAX);
-	xstats.ctokens = clamp_t(s64, PSCHED_NS2TICKS(cl->ctokens),
+	xstats.ctokens = clamp_t(s64, PSCHED_NS2TICKS(READ_ONCE(cl->ctokens)),
 				 INT_MIN, INT_MAX);
 
 	if (q->offload) {
@@ -1517,8 +1518,8 @@ static void htb_parent_to_leaf(struct Qdisc *sch, struct htb_class *cl,
 	parent->level = 0;
 	memset(&parent->inner, 0, sizeof(parent->inner));
 	parent->leaf.q = new_q ? new_q : &noop_qdisc;
-	parent->tokens = parent->buffer;
-	parent->ctokens = parent->cbuffer;
+	WRITE_ONCE(parent->tokens, parent->buffer);
+	WRITE_ONCE(parent->ctokens, parent->cbuffer);
 	parent->t_c = ktime_get_ns();
 	parent->cmode = HTB_CAN_SEND;
 	if (q->offload)
-- 
2.54.0.563.g4f69b47b94-goog