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

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

[Stephen Hemminger]

Cleanup and improvement patches for netem, done as follow-on
to the bug fix series found during AI-assisted review.

- replace pr_info() with netlink extack error reporting
- validate slot min/max delay range on configuration
- fix slot delay calculation overflow for large ranges
- remove unused loss model struct fields
- remove stale VERSION string
- add per-impairment extended statistics (delayed, dropped,
  corrupted, duplicated, reordered, ecn_marked)

The xstats patch requires a corresponding iproute2 change
to display the new counters in tc -s qdisc show. This will go
as separate patch after this.

Stephen Hemminger (6):
  net/sched: netem: replace pr_info with netlink extack error messages
  net/sched: netem: check for invalid slot range
  net/sched: netem: fix slot delay calculation overflow
  net/sched: netem: remove unused loss model fields
  net/sched: netem: remove useless VERSION
  net/sched: netem: add per-impairment extended statistics

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

-- 
2.53.0

[PATCH net-next 1/6] 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 5de1c932944a..73d0e85eeadc 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -804,19 +804,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(extack, "invalid distribution table length");
 		return -EINVAL;
+	}
 
 	d = kvmalloc_flex(*d, table, n);
-	if (!d)
+	if (!d) {
+		NL_SET_ERR_MSG(extack, "allocation of distribution table failed");
 		return -ENOMEM;
+	}
 
 	d->size = n;
 	for (i = 0; i < n; i++)
@@ -887,7 +892,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;
@@ -900,7 +906,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(extack, "incorrect gi model size");
 				return -EINVAL;
 			}
 
@@ -919,7 +925,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(extack, "incorrect ge model size");
 				return -EINVAL;
 			}
 
@@ -933,7 +939,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_FMT(extack, "unknown loss type %u", type);
 			return -EINVAL;
 		}
 	}
@@ -956,12 +962,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;
 	}
 
@@ -1010,8 +1017,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];
@@ -1023,18 +1029,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;
 	}
@@ -1045,7 +1051,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;
@@ -1134,13 +1140,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/6] net/sched: netem: check for invalid slot range

[Stephen Hemminger]

Reject slot configuration where min_delay exceeds max_delay.
The delay range computation in get_slot_next() underflows in
this case, producing bogus results.

Fixes: 0a9fe5c375b5 ("netem: slotting with non-uniform distribution")

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

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 73d0e85eeadc..de22d754cb79 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -831,6 +831,17 @@ static int get_dist_table(struct disttable **tbl, const struct nlattr *attr,
 	return 0;
 }
 
+static int validate_slot(const struct nlattr *attr, struct netlink_ext_ack *extack)
+{
+	const struct tc_netem_slot *c = nla_data(attr);
+
+	if (c->min_delay > c->max_delay) {
+		NL_SET_ERR_MSG(extack, "slot min delay greater than max delay");
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static void get_slot(struct netem_sched_data *q, const struct nlattr *attr)
 {
 	const struct tc_netem_slot *c = nla_data(attr);
@@ -1045,6 +1056,12 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt, struct netlink_ex
 			goto table_free;
 	}
 
+	if (tb[TCA_NETEM_SLOT]) {
+		ret = validate_slot(tb[TCA_NETEM_SLOT], extack);
+		if (ret)
+			goto table_free;
+	}
+
 	sch_tree_lock(sch);
 	/* backup q->clg and q->loss_model */
 	old_clg = q->clg;
-- 
2.53.0

[PATCH net-next 3/6] net/sched: netem: fix slot delay calculation overflow

[Stephen Hemminger]

get_slot_next() computes a random delay between min_delay and
max_delay using:

  get_random_u32() * (max_delay - min_delay) >> 32

This overflows signed 64-bit arithmetic when the delay range exceeds
approximately 2.1 seconds (2^31 nanoseconds), producing a negative
result that effectively disables slot-based pacing. This is a
realistic configuration for WAN emulation (e.g., slot 1s 5s).

Use mul_u64_u32_shr() which handles the widening multiply without
overflow.

Fixes: 0a9fe5c375b5 ("netem: slotting with non-uniform distribution")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/sched/sch_netem.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index de22d754cb79..69c93f7ade62 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -658,9 +658,8 @@ static void get_slot_next(struct netem_sched_data *q, u64 now)
 
 	if (!q->slot_dist)
 		next_delay = q->slot_config.min_delay +
-				(get_random_u32() *
-				 (q->slot_config.max_delay -
-				  q->slot_config.min_delay) >> 32);
+			mul_u64_u32_shr(q->slot_config.max_delay - q->slot_config.min_delay,
+					get_random_u32(), 32);
 	else
 		next_delay = tabledist(q->slot_config.dist_delay,
 				       (s32)(q->slot_config.dist_jitter),
-- 
2.53.0

[PATCH net-next 4/6] 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 69c93f7ade62..87caf1b9a4a7 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 5/6] 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 87caf1b9a4a7..26fc68e34b91 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.
 	====================================
 
@@ -1379,16 +1377,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 6/6] 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 26fc68e34b91..755e8d009f85 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;
@@ -495,6 +503,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	 * do it now in software before we mangle it.
 	 */
 	if (q->corrupt && q->corrupt >= get_crandom(&q->corrupt_cor, &q->prng)) {
+		q->xstats.corrupted++;
 		if (skb_is_gso(skb)) {
 			skb = netem_segment(skb, sch, to_free);
 			if (!skb)
@@ -597,6 +606,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);
@@ -605,6 +617,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		 * Do re-ordering by putting one out of N packets at the front
 		 * of the queue.
 		 */
+		q->xstats.reordered++;
 		cb->time_to_send = ktime_get_ns();
 		q->counter = 0;
 
@@ -1311,6 +1324,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)
 {
@@ -1373,6 +1393,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