[PATCH net-next 0/5] net/sched: netem: cleanups and improvements

[PATCH net-next 0/5] net/sched: netem: cleanups and improvements

[Stephen Hemminger]

This series modernizes the netem qdisc with several cleanups and
two functional improvements. It is independent of the bug fixes
that are in process on the net branch.

The first three patches are housekeeping: replacing pr_info() calls
with proper netlink extack error reporting, removing unused struct
members that only existed to declare enum constants, and dropping
a version string that was never updated.

The fourth patch adds per-impairment extended statistics
(delayed, dropped, corrupted, duplicated, reordered, ecn_marked)
reported via TCA_STATS_APP, following the pattern established by
RED and FQ_CODEL. A companion iproute2 patch for display will
follow separately.

The fifth patch improves the corruption path to handle
multi-segment skbs using skb_header_pointer()/skb_store_bits(),
replacing the previous code that only flipped bits in the linear
header region.

Stephen Hemminger (5):
  net/sched: netem: replace pr_info with netlink extack error messages
  net/sched: netem: remove unused loss model fields
  net/sched: netem: remove useless VERSION
  net/sched: netem: add per-impairment extended statistics
  net/sched: netem: handle multi-segment skb in corruption

 include/uapi/linux/pkt_sched.h |   9 +++
 net/sched/sch_netem.c          | 112 +++++++++++++++++++++------------
 2 files changed, 81 insertions(+), 40 deletions(-)

-- 
2.53.0

[PATCH net-next 1/5] net/sched: netem: replace pr_info with netlink extack error messages

[Stephen Hemminger]

netem predates the netlink extended ack mechanism and uses pr_info()
to report configuration errors. These messages go to the kernel log
where the user running tc may never see them, and in unprivileged
user namespace contexts they can be used for log spam.

Replace pr_info() with NL_SET_ERR_MSG() and NL_SET_ERR_MSG_FMT()
which return error details to the caller via netlink.
Remove the uninformative "netem: change failed" message from netem_init()
since the error is already propagated.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/sched/sch_netem.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 20df1c08b1e9..acf92cc44126 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -805,19 +805,24 @@ static void dist_free(struct disttable *d)
  * signed 16 bit values.
  */
 
-static int get_dist_table(struct disttable **tbl, const struct nlattr *attr)
+static int get_dist_table(struct disttable **tbl, const struct nlattr *attr,
+			  struct netlink_ext_ack *extack)
 {
 	size_t n = nla_len(attr)/sizeof(__s16);
 	const __s16 *data = nla_data(attr);
 	struct disttable *d;
 	int i;
 
-	if (!n || n > NETEM_DIST_MAX)
+	if (!n || n > NETEM_DIST_MAX) {
+		NL_SET_ERR_MSG_ATTR(extack, attr, "invalid distribution table length");
 		return -EINVAL;
+	}
 
 	d = kvmalloc_flex(*d, table, n);
-	if (!d)
+	if (!d) {
+		NL_SET_ERR_MSG_ATTR(extack, attr, "allocation of distribution table failed");
 		return -ENOMEM;
+	}
 
 	d->size = n;
 	for (i = 0; i < n; i++)
@@ -888,7 +893,8 @@ static void get_rate(struct netem_sched_data *q, const struct nlattr *attr)
 		q->cell_size_reciprocal = (struct reciprocal_value) { 0 };
 }
 
-static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
+static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr,
+			struct netlink_ext_ack *extack)
 {
 	const struct nlattr *la;
 	int rem;
@@ -901,7 +907,7 @@ static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
 			const struct tc_netem_gimodel *gi = nla_data(la);
 
 			if (nla_len(la) < sizeof(struct tc_netem_gimodel)) {
-				pr_info("netem: incorrect gi model size\n");
+				NL_SET_ERR_MSG_ATTR(extack, attr, "incorrect gi model size");
 				return -EINVAL;
 			}
 
@@ -920,7 +926,7 @@ static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
 			const struct tc_netem_gemodel *ge = nla_data(la);
 
 			if (nla_len(la) < sizeof(struct tc_netem_gemodel)) {
-				pr_info("netem: incorrect ge model size\n");
+				NL_SET_ERR_MSG_ATTR(extack, attr, "incorrect ge model size");
 				return -EINVAL;
 			}
 
@@ -934,7 +940,7 @@ static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
 		}
 
 		default:
-			pr_info("netem: unknown loss type %u\n", type);
+			NL_SET_ERR_MSG_ATTR_FMT(extack, attr, "unknown loss type %u", type);
 			return -EINVAL;
 		}
 	}
@@ -957,12 +963,13 @@ static const struct nla_policy netem_policy[TCA_NETEM_MAX + 1] = {
 };
 
 static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
-		      const struct nla_policy *policy, int len)
+		      const struct nla_policy *policy, size_t len,
+		      struct netlink_ext_ack *extack)
 {
 	int nested_len = nla_len(nla) - NLA_ALIGN(len);
 
 	if (nested_len < 0) {
-		pr_info("netem: invalid attributes len %d\n", nested_len);
+		NL_SET_ERR_MSG(extack, "invalid attribute len");
 		return -EINVAL;
 	}
 
@@ -1011,8 +1018,7 @@ static int check_netem_in_tree(struct Qdisc *sch, bool duplicates,
 }
 
 /* Parse netlink message to set options */
-static int netem_change(struct Qdisc *sch, struct nlattr *opt,
-			struct netlink_ext_ack *extack)
+static int netem_change(struct Qdisc *sch, struct nlattr *opt, struct netlink_ext_ack *extack)
 {
 	struct netem_sched_data *q = qdisc_priv(sch);
 	struct nlattr *tb[TCA_NETEM_MAX + 1];
@@ -1024,18 +1030,18 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 	int ret;
 
 	qopt = nla_data(opt);
-	ret = parse_attr(tb, TCA_NETEM_MAX, opt, netem_policy, sizeof(*qopt));
+	ret = parse_attr(tb, TCA_NETEM_MAX, opt, netem_policy, sizeof(*qopt), extack);
 	if (ret < 0)
 		return ret;
 
 	if (tb[TCA_NETEM_DELAY_DIST]) {
-		ret = get_dist_table(&delay_dist, tb[TCA_NETEM_DELAY_DIST]);
+		ret = get_dist_table(&delay_dist, tb[TCA_NETEM_DELAY_DIST], extack);
 		if (ret)
 			goto table_free;
 	}
 
 	if (tb[TCA_NETEM_SLOT_DIST]) {
-		ret = get_dist_table(&slot_dist, tb[TCA_NETEM_SLOT_DIST]);
+		ret = get_dist_table(&slot_dist, tb[TCA_NETEM_SLOT_DIST], extack);
 		if (ret)
 			goto table_free;
 	}
@@ -1046,7 +1052,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 	old_loss_model = q->loss_model;
 
 	if (tb[TCA_NETEM_LOSS]) {
-		ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
+		ret = get_loss_clg(q, tb[TCA_NETEM_LOSS], extack);
 		if (ret) {
 			q->loss_model = old_loss_model;
 			q->clg = old_clg;
@@ -1135,13 +1141,13 @@ static int netem_init(struct Qdisc *sch, struct nlattr *opt,
 
 	qdisc_watchdog_init(&q->watchdog, sch);
 
-	if (!opt)
+	if (!opt) {
+		NL_SET_ERR_MSG(extack, "missing parameters");
 		return -EINVAL;
+	}
 
 	q->loss_model = CLG_RANDOM;
 	ret = netem_change(sch, opt, extack);
-	if (ret)
-		pr_info("netem: change failed\n");
 	return ret;
 }
 
-- 
2.53.0

[PATCH net-next 2/5] net/sched: netem: remove unused loss model fields

[Stephen Hemminger]

The _4_state_model and GE_state_model enum definitions are declared
as struct members but are never read or written. Only the enum
constants they define (TX_IN_GAP_PERIOD, GOOD_STATE, etc.) are used.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/sched/sch_netem.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index acf92cc44126..6a234880d338 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -71,6 +71,18 @@ struct disttable {
 	s16 table[] __counted_by(size);
 };
 
+enum GE_state_model {
+	GOOD_STATE = 1,
+	BAD_STATE,
+};
+
+enum _4_state_model {
+	TX_IN_GAP_PERIOD = 1,
+	TX_IN_BURST_PERIOD,
+	LOST_IN_GAP_PERIOD,
+	LOST_IN_BURST_PERIOD,
+};
+
 struct netem_sched_data {
 	/* internal t(ime)fifo qdisc uses t_root and sch->limit */
 	struct rb_root t_root;
@@ -121,18 +133,6 @@ struct netem_sched_data {
 		CLG_GILB_ELL,
 	} loss_model;
 
-	enum {
-		TX_IN_GAP_PERIOD = 1,
-		TX_IN_BURST_PERIOD,
-		LOST_IN_GAP_PERIOD,
-		LOST_IN_BURST_PERIOD,
-	} _4_state_model;
-
-	enum {
-		GOOD_STATE = 1,
-		BAD_STATE,
-	} GE_state_model;
-
 	/* Correlated Loss Generation models */
 	struct clgstate {
 		/* state of the Markov chain */
-- 
2.53.0

[PATCH net-next 3/5] net/sched: netem: remove useless VERSION

[Stephen Hemminger]

The version printed was never updated and kernel version is
better indication of what is fixed or not.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/sched/sch_netem.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 6a234880d338..76af9bd3ecaa 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -27,8 +27,6 @@
 #include <net/pkt_sched.h>
 #include <net/inet_ecn.h>
 
-#define VERSION "1.3"
-
 /*	Network Emulation Queuing algorithm.
 	====================================
 
@@ -1364,16 +1362,15 @@ static struct Qdisc_ops netem_qdisc_ops __read_mostly = {
 };
 MODULE_ALIAS_NET_SCH("netem");
 
-
 static int __init netem_module_init(void)
 {
-	pr_info("netem: version " VERSION "\n");
 	return register_qdisc(&netem_qdisc_ops);
 }
 static void __exit netem_module_exit(void)
 {
 	unregister_qdisc(&netem_qdisc_ops);
 }
+
 module_init(netem_module_init)
 module_exit(netem_module_exit)
 MODULE_LICENSE("GPL");
-- 
2.53.0

[PATCH net-next 4/5] net/sched: netem: add per-impairment extended statistics

[Stephen Hemminger]

netem applies several impairments (delay, loss, corruption, duplication,
reordering) but exposes no counters distinguishing which impairment
affected a given packet.

Add a struct tc_netem_xstats reported via TCA_STATS_APP so that
userspace (tc -s qdisc show) can display per-impairment counters.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 include/uapi/linux/pkt_sched.h |  9 +++++++++
 net/sched/sch_netem.c          | 27 ++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 66e8072f44df..fada10cb9b7b 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -569,6 +569,15 @@ struct tc_netem_gemodel {
 #define NETEM_DIST_SCALE	8192
 #define NETEM_DIST_MAX		16384
 
+struct tc_netem_xstats {
+	__u32	delayed;	/* packets delayed */
+	__u32	dropped;	/* packets dropped by loss model      */
+	__u32	corrupted;	/* packets with bit errors injected   */
+	__u32	duplicated;	/* duplicate packets generated        */
+	__u32	reordered;	/* packets sent out of order          */
+	__u32	ecn_marked;	/* packets ECN CE-marked (not dropped)*/
+};
+
 /* DRR */
 
 enum {
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 76af9bd3ecaa..6dc1ba8e999b 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -152,6 +152,9 @@ struct netem_sched_data {
 	} slot;
 
 	struct disttable *slot_dist;
+
+	/* Per-impairment counters */
+	struct tc_netem_xstats xstats;
 };
 
 /* Time stamp put into socket buffer control block
@@ -459,17 +462,22 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	skb->prev = NULL;
 
 	/* Random duplication */
-	if (q->duplicate && q->duplicate >= get_crandom(&q->dup_cor, &q->prng))
+	if (q->duplicate && q->duplicate >= get_crandom(&q->dup_cor, &q->prng)) {
 		++count;
+		q->xstats.duplicated++;
+	}
 
 	/* Drop packet? */
 	if (loss_event(q)) {
-		if (q->ecn && INET_ECN_set_ce(skb))
+		if (q->ecn && INET_ECN_set_ce(skb)) {
 			qdisc_qstats_drop(sch); /* mark packet */
-		else
+			q->xstats.ecn_marked++;
+		} else {
 			--count;
+		}
 	}
 	if (count == 0) {
+		q->xstats.dropped++;
 		qdisc_qstats_drop(sch);
 		__qdisc_drop(skb, to_free);
 		return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
@@ -520,6 +528,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		if (skb_headlen(skb))
 			skb->data[get_random_u32_below(skb_headlen(skb))] ^=
 				1 << get_random_u32_below(8);
+		q->xstats.corrupted++;
 	}
 
 	if (unlikely(q->t_len >= sch->limit)) {
@@ -598,6 +607,9 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			delay += packet_time_ns(qdisc_pkt_len(skb), q);
 		}
 
+		if (delay > 0)
+			q->xstats.delayed++;
+
 		cb->time_to_send = now + delay;
 		++q->counter;
 		tfifo_enqueue(skb, sch);
@@ -610,6 +622,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		q->counter = 0;
 
 		__qdisc_enqueue_head(skb, &sch->q);
+		q->xstats.reordered++;
 		sch->qstats.requeues++;
 	}
 
@@ -1296,6 +1309,13 @@ static int netem_dump(struct Qdisc *sch, struct sk_buff *skb)
 	return -1;
 }
 
+static int netem_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
+{
+	struct netem_sched_data *q = qdisc_priv(sch);
+
+	return gnet_stats_copy_app(d, &q->xstats, sizeof(q->xstats));
+}
+
 static int netem_dump_class(struct Qdisc *sch, unsigned long cl,
 			  struct sk_buff *skb, struct tcmsg *tcm)
 {
@@ -1358,6 +1378,7 @@ static struct Qdisc_ops netem_qdisc_ops __read_mostly = {
 	.destroy	=	netem_destroy,
 	.change		=	netem_change,
 	.dump		=	netem_dump,
+	.dump_stats	=	netem_dump_stats,
 	.owner		=	THIS_MODULE,
 };
 MODULE_ALIAS_NET_SCH("netem");
-- 
2.53.0

[PATCH net-next 5/5] net/sched: netem: handle multi-segment skb in corruption

[Stephen Hemminger]

The packet corruption code only flipped bits in the linear
header portion of the skb, skipping corruption when
skb_headlen() was zero.

Use skb_header_pointer() and skb_store_bits() to access the
full packet data, allowing any bit in the packet to be
corrupted regardless of how the skb is laid out.

Replaces d64cb81dcbd5 ("net/sched: sch_netem: fix
out-of-bounds access in packet corruption") with a more
general solution.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/sched/sch_netem.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 6dc1ba8e999b..9b5731a8cd15 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -525,10 +525,18 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			goto finish_segs;
 		}
 
-		if (skb_headlen(skb))
-			skb->data[get_random_u32_below(skb_headlen(skb))] ^=
-				1 << get_random_u32_below(8);
-		q->xstats.corrupted++;
+		if (skb->len > 0) {
+			unsigned int offset = get_random_u32_below(skb->len);
+			u8 *ptr, val;
+
+			/* handle multi-segment skb's */
+			ptr = skb_header_pointer(skb, offset, 1, &val);
+			if (ptr) {
+				val = *ptr ^ (1 << get_random_u32_below(8));
+				skb_store_bits(skb, offset, &val, 1);
+			}
+			q->xstats.corrupted++;
+		}
 	}
 
 	if (unlikely(q->t_len >= sch->limit)) {
-- 
2.53.0