[PATCH net 00/12] net: shaper: fix various minor bugs

[PATCH net 00/12] net: shaper: fix various minor bugs

[Jakub Kicinski]

Fix various minor bugs in the net shaper API.

First 4 changes are a bit hairy. The readers are currently under
RCU so we need to be careful how we update and insert the nodes.
Patch 4 is a bit of a cop-out. Pre-allocating replacement nodes
is complex. If the workaround is not acceptable I'd seriously
consider making the readers take the instance lock.

Only other patch of note is patch 10. We want to add a Netlink
policy check on the handle ID. This necessitates patch 9.

The rest are simple and self-explanatory.

Jakub Kicinski (12):
  net: shaper: drop redundant xa_lock() bracketing
  net: shaper: flip the polarity of the valid flag
  net: shaper: fix trivial ordering issue in net_shaper_commit()
  net: shaper: try to avoid violating RCU
  net: shaper: reject duplicate leaves in GROUP request
  selftests: drv-net: add shaper test for duplicate leaves
  net: shaper: set ret to -ENOMEM when genlmsg_new() fails in group_doit
  net: shaper: fix undersized reply skb allocation in GROUP command
  tools: ynl: add scope qualifier for definitions
  net: shaper: reject handle IDs exceeding internal bit-width
  net: shaper: enforce singleton NETDEV scope with id 0
  net: shaper: reject QUEUE scope handle with missing id

 Documentation/netlink/genetlink-c.yaml        |   9 +
 Documentation/netlink/genetlink-legacy.yaml   |   9 +
 Documentation/netlink/genetlink.yaml          |   9 +
 Documentation/netlink/specs/net_shaper.yaml   |   7 +
 net/shaper/shaper_nl_gen.h                    |   2 +
 net/shaper/shaper.c                           | 171 +++++++++++++-----
 net/shaper/shaper_nl_gen.c                    |   7 +-
 tools/net/ynl/pyynl/ynl_gen_c.py              |  31 +++-
 tools/testing/selftests/drivers/net/shaper.py |  25 ++-
 9 files changed, 216 insertions(+), 54 deletions(-)

-- 
2.54.0

[PATCH net 01/12] net: shaper: drop redundant xa_lock() bracketing

[Jakub Kicinski]

The shaper insertion code is written in a way that suggests that
perhaps it was expecting readers to be fenced off by xa_lock.
This is not the case, readers of XArray are purely under RCU.

Remove the explicit taking of xa_lock() to simplify subsequent fixes.

All writers to hierarchy->shapers are serialized by the netdev instance
lock. For Netlink taken in net_shaper_nl_pre_doit_write().
net_shaper_set_real_num_tx_queues() has a netdev_assert_locked().
net_shaper_flush_netdev() runs after netdev is made inaccessible
to readers.

The explicit xa_lock() bracketing in pre_insert(), commit(), rollback()
and flush() therefore does not protect against any other writer.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index 94bc9c7382ea..e28d20774713 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -373,10 +373,8 @@ static int net_shaper_pre_insert(struct net_shaper_binding *binding,
 	/* Mark 'tentative' shaper inside the hierarchy container.
 	 * xa_set_mark is a no-op if the previous store fails.
 	 */
-	xa_lock(&hierarchy->shapers);
-	prev = __xa_store(&hierarchy->shapers, index, cur, GFP_KERNEL);
-	__xa_set_mark(&hierarchy->shapers, index, NET_SHAPER_NOT_VALID);
-	xa_unlock(&hierarchy->shapers);
+	prev = xa_store(&hierarchy->shapers, index, cur, GFP_KERNEL);
+	xa_set_mark(&hierarchy->shapers, index, NET_SHAPER_NOT_VALID);
 	if (xa_err(prev)) {
 		NL_SET_ERR_MSG(extack, "Can't insert shaper into device store");
 		kfree_rcu(cur, rcu);
@@ -402,7 +400,6 @@ static void net_shaper_commit(struct net_shaper_binding *binding,
 	int index;
 	int i;
 
-	xa_lock(&hierarchy->shapers);
 	for (i = 0; i < nr_shapers; ++i) {
 		index = net_shaper_handle_to_index(&shapers[i].handle);
 
@@ -413,11 +410,10 @@ static void net_shaper_commit(struct net_shaper_binding *binding,
 		/* Successful update: drop the tentative mark
 		 * and update the hierarchy container.
 		 */
-		__xa_clear_mark(&hierarchy->shapers, index,
-				NET_SHAPER_NOT_VALID);
+		xa_clear_mark(&hierarchy->shapers, index,
+			      NET_SHAPER_NOT_VALID);
 		*cur = shapers[i];
 	}
-	xa_unlock(&hierarchy->shapers);
 }
 
 /* Rollback all the tentative inserts from the hierarchy. */
@@ -430,13 +426,11 @@ static void net_shaper_rollback(struct net_shaper_binding *binding)
 	if (!hierarchy)
 		return;
 
-	xa_lock(&hierarchy->shapers);
 	xa_for_each_marked(&hierarchy->shapers, index, cur,
 			   NET_SHAPER_NOT_VALID) {
-		__xa_erase(&hierarchy->shapers, index);
+		xa_erase(&hierarchy->shapers, index);
 		kfree(cur);
 	}
-	xa_unlock(&hierarchy->shapers);
 }
 
 static int net_shaper_parse_handle(const struct nlattr *attr,
@@ -1382,12 +1376,10 @@ static void net_shaper_flush(struct net_shaper_binding *binding)
 	if (!hierarchy)
 		return;
 
-	xa_lock(&hierarchy->shapers);
 	xa_for_each(&hierarchy->shapers, index, cur) {
-		__xa_erase(&hierarchy->shapers, index);
+		xa_erase(&hierarchy->shapers, index);
 		kfree(cur);
 	}
-	xa_unlock(&hierarchy->shapers);
 
 	kfree(hierarchy);
 }
-- 
2.54.0

[PATCH net 02/12] net: shaper: flip the polarity of the valid flag

[Jakub Kicinski]

The usual way of inserting entries which are not yet fully ready
into XArray is to have a VALID flag. The shaper code has a NOT_VALID
flag. Since XArray code does not let us create entries with a marks
already set - the creation of entries is currently not atomic.

Flip the polarity of the VALID flag. This closes the tiny race
in net_shaper_pre_insert() of entries being created without
the NOT_VALID flag.

Fixes: 93954b40f6a4 ("net-shapers: implement NL set and delete operations")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index e28d20774713..c5cf10543af0 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -275,11 +275,13 @@ static void net_shaper_default_parent(const struct net_shaper_handle *handle,
 	parent->id = 0;
 }
 
-/*
- * MARK_0 is already in use due to XA_FLAGS_ALLOC, can't reuse such flag as
- * it's cleared by xa_store().
+/* MARK_0 is already in use due to XA_FLAGS_ALLOC. The VALID mark is set on
+ * an entry only after the device-side configuration has completed
+ * successfully (see net_shaper_commit()). Lookups and dumps must filter on
+ * this mark to avoid exposing tentative entries inserted by
+ * net_shaper_pre_insert() while the driver call is still in flight.
  */
-#define NET_SHAPER_NOT_VALID XA_MARK_1
+#define NET_SHAPER_VALID	XA_MARK_1
 
 static struct net_shaper *
 net_shaper_lookup(struct net_shaper_binding *binding,
@@ -289,8 +291,8 @@ net_shaper_lookup(struct net_shaper_binding *binding,
 	struct net_shaper_hierarchy *hierarchy;
 
 	hierarchy = net_shaper_hierarchy_rcu(binding);
-	if (!hierarchy || xa_get_mark(&hierarchy->shapers, index,
-				      NET_SHAPER_NOT_VALID))
+	if (!hierarchy || !xa_get_mark(&hierarchy->shapers, index,
+				       NET_SHAPER_VALID))
 		return NULL;
 
 	return xa_load(&hierarchy->shapers, index);
@@ -370,11 +372,10 @@ static int net_shaper_pre_insert(struct net_shaper_binding *binding,
 		goto free_id;
 	}
 
-	/* Mark 'tentative' shaper inside the hierarchy container.
-	 * xa_set_mark is a no-op if the previous store fails.
+	/* Insert as 'tentative' (no VALID mark). The mark will be set by
+	 * net_shaper_commit() once the driver-side configuration succeeds.
 	 */
 	prev = xa_store(&hierarchy->shapers, index, cur, GFP_KERNEL);
-	xa_set_mark(&hierarchy->shapers, index, NET_SHAPER_NOT_VALID);
 	if (xa_err(prev)) {
 		NL_SET_ERR_MSG(extack, "Can't insert shaper into device store");
 		kfree_rcu(cur, rcu);
@@ -410,8 +411,7 @@ static void net_shaper_commit(struct net_shaper_binding *binding,
 		/* Successful update: drop the tentative mark
 		 * and update the hierarchy container.
 		 */
-		xa_clear_mark(&hierarchy->shapers, index,
-			      NET_SHAPER_NOT_VALID);
+		xa_set_mark(&hierarchy->shapers, index, NET_SHAPER_VALID);
 		*cur = shapers[i];
 	}
 }
@@ -426,8 +426,9 @@ static void net_shaper_rollback(struct net_shaper_binding *binding)
 	if (!hierarchy)
 		return;
 
-	xa_for_each_marked(&hierarchy->shapers, index, cur,
-			   NET_SHAPER_NOT_VALID) {
+	xa_for_each(&hierarchy->shapers, index, cur) {
+		if (xa_get_mark(&hierarchy->shapers, index, NET_SHAPER_VALID))
+			continue;
 		xa_erase(&hierarchy->shapers, index);
 		kfree(cur);
 	}
@@ -830,7 +831,8 @@ int net_shaper_nl_get_dumpit(struct sk_buff *skb,
 		goto out_unlock;
 
 	for (; (shaper = xa_find(&hierarchy->shapers, &ctx->start_index,
-				 U32_MAX, XA_PRESENT)); ctx->start_index++) {
+				 U32_MAX, NET_SHAPER_VALID));
+	     ctx->start_index++) {
 		ret = net_shaper_fill_one(skb, binding, shaper, info);
 		if (ret)
 			break;
-- 
2.54.0

[PATCH net 03/12] net: shaper: fix trivial ordering issue in net_shaper_commit()

[Jakub Kicinski]

We should update the entry before we mark it as valid.

Fixes: 93954b40f6a4 ("net-shapers: implement NL set and delete operations")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index c5cf10543af0..692e38df42cb 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -295,6 +295,10 @@ net_shaper_lookup(struct net_shaper_binding *binding,
 				       NET_SHAPER_VALID))
 		return NULL;
 
+	/* Pairs with smp_wmb() in net_shaper_commit(): if the entry is
+	 * valid, its contents must be visible too.
+	 */
+	smp_rmb();
 	return xa_load(&hierarchy->shapers, index);
 }
 
@@ -411,8 +415,9 @@ static void net_shaper_commit(struct net_shaper_binding *binding,
 		/* Successful update: drop the tentative mark
 		 * and update the hierarchy container.
 		 */
-		xa_set_mark(&hierarchy->shapers, index, NET_SHAPER_VALID);
 		*cur = shapers[i];
+		smp_wmb();
+		xa_set_mark(&hierarchy->shapers, index, NET_SHAPER_VALID);
 	}
 }
 
@@ -833,6 +838,10 @@ int net_shaper_nl_get_dumpit(struct sk_buff *skb,
 	for (; (shaper = xa_find(&hierarchy->shapers, &ctx->start_index,
 				 U32_MAX, NET_SHAPER_VALID));
 	     ctx->start_index++) {
+		/* Pairs with smp_wmb() in net_shaper_commit(): the entry
+		 * is marked VALID, so its contents must be visible too.
+		 */
+		smp_rmb();
 		ret = net_shaper_fill_one(skb, binding, shaper, info);
 		if (ret)
 			break;
-- 
2.54.0

[PATCH net 04/12] net: shaper: try to avoid violating RCU

[Jakub Kicinski]

net_shaper_commit() overrides nodes which may be concurrently read
under RCU. This is not a huge deal since the entries only contain
config, worst case user will see inconsistent config params. But
we should try to avoid this obvious RCU violation. Try to allocate
a new node. Since commit() can't fail fall back to overriding.

Full fix is probably not worth the complexity, struct net_shaper
is around 80B, and the allocation is with GFP_KERNEL.

Fixes: 93954b40f6a4 ("net-shapers: implement NL set and delete operations")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index 692e38df42cb..a2e9adca9afc 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -406,18 +406,36 @@ static void net_shaper_commit(struct net_shaper_binding *binding,
 	int i;
 
 	for (i = 0; i < nr_shapers; ++i) {
+		struct net_shaper *new;
+
 		index = net_shaper_handle_to_index(&shapers[i].handle);
 
 		cur = xa_load(&hierarchy->shapers, index);
 		if (WARN_ON_ONCE(!cur))
 			continue;
 
-		/* Successful update: drop the tentative mark
-		 * and update the hierarchy container.
+		/* If the shaper is already visible try to allocate a new
+		 * entry so that we don't cause torn reads under RCU.
+		 * This tiny GFP_KERNEL alloc should never fail, really,
+		 * but if it does just override, the torn read is acceptable.
 		 */
-		*cur = shapers[i];
-		smp_wmb();
-		xa_set_mark(&hierarchy->shapers, index, NET_SHAPER_VALID);
+		new = NULL;
+		if (xa_get_mark(&hierarchy->shapers, index, NET_SHAPER_VALID))
+			new = kmalloc_obj(*new);
+
+		if (new) {
+			*new = shapers[i];
+			smp_wmb();
+			/* Replacing an entry, xa_store() can't fail */
+			WARN_ON(xa_err(xa_store(&hierarchy->shapers, index,
+						new, GFP_KERNEL)));
+			kfree_rcu(cur, rcu);
+		} else {
+			*cur = shapers[i];
+			smp_wmb();
+			xa_set_mark(&hierarchy->shapers, index,
+				    NET_SHAPER_VALID);
+		}
 	}
 }
 
-- 
2.54.0

[PATCH net 05/12] net: shaper: reject duplicate leaves in GROUP request

[Jakub Kicinski]

net_shaper_nl_group_doit() does not deduplicate NET_SHAPER_A_LEAVES
entries. When userspace supplies the same leaf handle twice, the same
old-parent pointer lands twice in old_nodes[]. The cleanup loop double
frees the parent. Of course the same parent may still be in old_nodes[]
twice if we are moving multiple of its leaves.

Note that this patch also implicitly fixes the fact that the
i >= leaves_count path forgets to set ret.

Fixes: 5d5d4700e75d ("net-shapers: implement NL group operation")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 60 +++++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 15 deletions(-)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index a2e9adca9afc..a27673e5919f 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -955,6 +955,46 @@ static int net_shaper_handle_cmp(const struct net_shaper_handle *a,
 	return memcmp(a, b, sizeof(*a));
 }
 
+static int net_shaper_parse_leaves(struct net_shaper_binding *binding,
+				   struct genl_info *info,
+				   const struct net_shaper *node,
+				   struct net_shaper *leaves,
+				   int leaves_count)
+{
+	struct nlattr *attr;
+	int i, j, ret, rem;
+
+	i = 0;
+	nla_for_each_attr_type(attr, NET_SHAPER_A_LEAVES,
+			       genlmsg_data(info->genlhdr),
+			       genlmsg_len(info->genlhdr), rem) {
+		if (WARN_ON_ONCE(i >= leaves_count))
+			return -EINVAL;
+
+		ret = net_shaper_parse_leaf(binding, attr, info,
+					    node, &leaves[i]);
+		if (ret)
+			return ret;
+
+		/* Reject duplicates */
+		for (j = 0; j < i; j++) {
+			if (net_shaper_handle_cmp(&leaves[i].handle,
+						  &leaves[j].handle))
+				continue;
+
+			NL_SET_ERR_MSG_ATTR_FMT(info->extack, attr,
+						"Duplicate leaf shaper %d:%d",
+						leaves[i].handle.scope,
+						leaves[i].handle.id);
+			return -EINVAL;
+		}
+
+		i++;
+	}
+
+	return 0;
+}
+
 static int net_shaper_parent_from_leaves(int leaves_count,
 					 const struct net_shaper *leaves,
 					 struct net_shaper *node,
@@ -1195,10 +1235,9 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
 	struct net_shaper **old_nodes, *leaves, node = {};
 	struct net_shaper_hierarchy *hierarchy;
 	struct net_shaper_binding *binding;
-	int i, ret, rem, leaves_count;
+	int i, ret, leaves_count;
 	int old_nodes_count = 0;
 	struct sk_buff *msg;
-	struct nlattr *attr;
 
 	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_LEAVES))
 		return -EINVAL;
@@ -1226,19 +1265,10 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
 	if (ret)
 		goto free_leaves;
 
-	i = 0;
-	nla_for_each_attr_type(attr, NET_SHAPER_A_LEAVES,
-			       genlmsg_data(info->genlhdr),
-			       genlmsg_len(info->genlhdr), rem) {
-		if (WARN_ON_ONCE(i >= leaves_count))
-			goto free_leaves;
-
-		ret = net_shaper_parse_leaf(binding, attr, info,
-					    &node, &leaves[i]);
-		if (ret)
-			goto free_leaves;
-		i++;
-	}
+	ret = net_shaper_parse_leaves(binding, info, &node,
+				      leaves, leaves_count);
+	if (ret)
+		goto free_leaves;
 
 	/* Prepare the msg reply in advance, to avoid device operation
 	 * rollback on allocation failure.
-- 
2.54.0

[PATCH net 06/12] selftests: drv-net: add shaper test for duplicate leaves

[Jakub Kicinski]

Add test exercising duplicate leaves.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/shaper.py | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/shaper.py b/tools/testing/selftests/drivers/net/shaper.py
index 11310f19bfa0..f7872c931bf6 100755
--- a/tools/testing/selftests/drivers/net/shaper.py
+++ b/tools/testing/selftests/drivers/net/shaper.py
@@ -1,7 +1,10 @@
 #!/usr/bin/env python3
 # SPDX-License-Identifier: GPL-2.0
 
-from lib.py import ksft_run, ksft_exit, ksft_eq, ksft_true, KsftSkipEx
+import errno
+
+from lib.py import ksft_run, ksft_exit
+from lib.py import ksft_eq, ksft_raises, ksft_true, KsftSkipEx
 from lib.py import EthtoolFamily, NetshaperFamily
 from lib.py import NetDrvEnv
 from lib.py import NlError
@@ -438,6 +441,21 @@ from lib.py import cmd
         nl_shaper.delete({'ifindex': cfg.ifindex,
                           'handle': {'scope': 'queue', 'id': i}})
 
+def dup_leaves(cfg, nl_shaper) -> None:
+    """ Ensure that the kernel rejects duplicate leaves. """
+    if not cfg.groups:
+        raise KsftSkipEx("device does not support node scope")
+
+    with ksft_raises(NlError) as cm:
+        nl_shaper.group({
+                   'ifindex': cfg.ifindex,
+                   'leaves':[{'handle': {'scope': 'queue', 'id': 0}},
+                             {'handle': {'scope': 'queue', 'id': 0}}],
+                   'handle': {'scope':'node'},
+                   'metric': 'bps',
+                   'bw-max': 10000})
+    ksft_eq(cm.exception.error, errno.EINVAL)
+
 def main() -> None:
     with NetDrvEnv(__file__, queue_count=4) as cfg:
         cfg.queues = False
@@ -453,7 +471,10 @@ from lib.py import cmd
                   basic_groups,
                   qgroups,
                   delegation,
-                  queue_update], args=(cfg, NetshaperFamily()))
+                  queue_update,
+                  dup_leaves
+                  ],
+                 args=(cfg, NetshaperFamily()))
     ksft_exit()
 
 
-- 
2.54.0

[PATCH net 07/12] net: shaper: set ret to -ENOMEM when genlmsg_new() fails in group_doit

[Jakub Kicinski]

genlmsg_new() alloc failure path in net_shaper_nl_group_doit() forgets
to set ret before jumping to error handling.

Fixes: 5d5d4700e75d ("net-shapers: implement NL group operation")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index a27673e5919f..2ba397fa3bfd 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -1274,8 +1274,10 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
 	 * rollback on allocation failure.
 	 */
 	msg = genlmsg_new(net_shaper_handle_size(), GFP_KERNEL);
-	if (!msg)
+	if (!msg) {
+		ret = -ENOMEM;
 		goto free_leaves;
+	}
 
 	hierarchy = net_shaper_hierarchy_setup(binding);
 	if (!hierarchy) {
-- 
2.54.0

[PATCH net 08/12] net: shaper: fix undersized reply skb allocation in GROUP command

[Jakub Kicinski]

net_shaper_group_send_reply() writes both the NET_SHAPER_A_IFINDEX
attribute (via net_shaper_fill_binding()) and the nested
NET_SHAPER_A_HANDLE attribute (via net_shaper_fill_handle()), but
the reply skb at the call site in net_shaper_nl_group_doit() is
allocated using net_shaper_handle_size(), which only accounts for
the nested handle.

The allocation is therefore short by nla_total_size(sizeof(u32))
(8 bytes) for the IFINDEX attribute.  In practice the slab allocator
rounds up the small allocation so the bug is latent, but the size
accounting is wrong and could bite if the reply grew further.

Introduce net_shaper_group_reply_size() that accounts for the full
reply payload and use it both at the genlmsg_new() call site and in
the defensive WARN_ONCE message.

Fixes: 5d5d4700e75d ("net-shapers: implement NL group operation")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index 2ba397fa3bfd..10d76f7148bf 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -90,6 +90,12 @@ static int net_shaper_handle_size(void)
 			      nla_total_size(sizeof(u32)));
 }
 
+static int net_shaper_group_reply_size(void)
+{
+	return nla_total_size(sizeof(u32)) +	/* NET_SHAPER_A_IFINDEX */
+	       net_shaper_handle_size();	/* NET_SHAPER_A_HANDLE */
+}
+
 static int net_shaper_fill_binding(struct sk_buff *msg,
 				   const struct net_shaper_binding *binding,
 				   u32 type)
@@ -1225,7 +1231,7 @@ static int net_shaper_group_send_reply(struct net_shaper_binding *binding,
 free_msg:
 	/* Should never happen as msg is pre-allocated with enough space. */
 	WARN_ONCE(true, "calculated message payload length (%d)",
-		  net_shaper_handle_size());
+		  net_shaper_group_reply_size());
 	nlmsg_free(msg);
 	return -EMSGSIZE;
 }
@@ -1273,7 +1279,7 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
 	/* Prepare the msg reply in advance, to avoid device operation
 	 * rollback on allocation failure.
 	 */
-	msg = genlmsg_new(net_shaper_handle_size(), GFP_KERNEL);
+	msg = genlmsg_new(net_shaper_group_reply_size(), GFP_KERNEL);
 	if (!msg) {
 		ret = -ENOMEM;
 		goto free_leaves;
-- 
2.54.0

[PATCH net 09/12] tools: ynl: add scope qualifier for definitions

[Jakub Kicinski]

Using definitions in kernel policies is awkward right now.
On one hand we want defines for max values and such.
On the other we don't have a way of adding kernel-only defines.
Adding unnecessary defines to uAPI is a bad idea, we won't
be able to delete them. And when it comes to policy user
space should just query it via the policy dump, not use
hard coded defines.

Add a "scope" property to definitions, which will let us tell
the codegen that a definition is for kernel use only. Support
following values:
  - uapi: render into the uAPI header (default, today's behavior)
  - kernel: render to kernel header only
  - user: same as kernel but for the user-side generated header

Definitions may have a header property (definition is "external",
provided by existing header). Extend the scope to headers, too.
If definition has both scope and header properties we will only
generate the includes in the right scope.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
This will fail yamllint, sticking to the same format as the rest
of the file (extra spaces inside brackets).

CC: donald.hunter@gmail.com
CC: ast@fiberby.net
---
 Documentation/netlink/genetlink-c.yaml      |  9 ++++++
 Documentation/netlink/genetlink-legacy.yaml |  9 ++++++
 Documentation/netlink/genetlink.yaml        |  9 ++++++
 tools/net/ynl/pyynl/ynl_gen_c.py            | 31 +++++++++++++++++++--
 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml
index 57f59fe23e3f..4ea31e8fc4d1 100644
--- a/Documentation/netlink/genetlink-c.yaml
+++ b/Documentation/netlink/genetlink-c.yaml
@@ -69,6 +69,15 @@ additionalProperties: False
         header:
           description: For C-compatible languages, header which already defines this value.
           type: string
+        scope:
+          description: |
+            Visibility of this definition. "uapi" (default) renders into
+            the uAPI header, "kernel" renders into the kernel-side
+            generated header, "user" renders into the user-side
+            generated header. When combined with `header:`, the
+            definition is not rendered, and the named header is
+            included only by code matching the scope.
+          enum: [ uapi, kernel, user ]
         type:
           enum: [ const, enum, flags ]
         doc:
diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index 66fb8653a344..f9c44747729a 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -83,6 +83,15 @@ additionalProperties: False
         header:
           description: For C-compatible languages, header which already defines this value.
           type: string
+        scope:
+          description: |
+            Visibility of this definition. "uapi" (default) renders into
+            the uAPI header, "kernel" renders into the kernel-side
+            generated header, "user" renders into the user-side
+            generated header. When combined with `header:`, the
+            definition is not rendered, and the named header is
+            included only by code matching the scope.
+          enum: [ uapi, kernel, user ]
         type:
           enum: [ const, enum, flags, struct ] # Trim
         doc:
diff --git a/Documentation/netlink/genetlink.yaml b/Documentation/netlink/genetlink.yaml
index a1194d5d93fc..d3f3f3399ddf 100644
--- a/Documentation/netlink/genetlink.yaml
+++ b/Documentation/netlink/genetlink.yaml
@@ -55,6 +55,15 @@ additionalProperties: False
         header:
           description: For C-compatible languages, header which already defines this value.
           type: string
+        scope:
+          description: |
+            Visibility of this definition. "uapi" (default) renders into
+            the uAPI header, "kernel" renders into the kernel-side
+            generated header, "user" renders into the user-side
+            generated header. When combined with `header:`, the
+            definition is not rendered, and the named header is
+            included only by code matching the scope.
+          enum: [ uapi, kernel, user ]
         type:
           enum: [ const, enum, flags ]
         doc:
diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 0e1e486c1185..cdc3646f2642 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -3212,6 +3212,8 @@ _C_KW = {
     for const in family['definitions']:
         if const.get('header'):
             continue
+        if const.get('scope', 'uapi') != 'uapi':
+            continue
 
         if const['type'] != 'const':
             cw.writes_defines(defines)
@@ -3339,6 +3341,25 @@ _C_KW = {
     cw.p(f'#endif /* {hdr_prot} */')
 
 
+def render_scoped_consts(family, cw, scope):
+    defines = []
+    for const in family['definitions']:
+        if const['type'] != 'const':
+            continue
+        if const.get('header'):
+            continue
+        if const.get('scope') != scope:
+            continue
+        name_pfx = const.get('name-prefix', f"{family.ident_name}-")
+        defines.append([
+            c_upper(family.get('c-define-name',
+                               f"{name_pfx}{const['name']}")),
+            const['value']])
+    if defines:
+        cw.writes_defines(defines)
+        cw.nl()
+
+
 def _render_user_ntf_entry(ri, op):
     if not ri.family.is_classic():
         ri.cw.block_start(line=f"[{op.enum_name}] = ")
@@ -3504,8 +3525,12 @@ _C_KW = {
             cw.p('#include "ynl.h"')
         headers = []
     for definition in parsed['definitions'] + parsed['attribute-sets']:
-        if 'header' in definition:
-            headers.append(definition['header'])
+        if 'header' not in definition:
+            continue
+        scope = definition.get('scope', 'uapi')
+        if scope != 'uapi' and scope != args.mode:
+            continue
+        headers.append(definition['header'])
     if args.mode == 'user':
         headers.append(parsed.uapi_header)
     seen_header = []
@@ -3522,6 +3547,7 @@ _C_KW = {
             for one in args.user_header:
                 cw.p(f'#include "{one}"')
         else:
+            render_scoped_consts(parsed, cw, 'user')
             cw.p('struct ynl_sock;')
             cw.nl()
             render_user_family(parsed, cw, True)
@@ -3529,6 +3555,7 @@ _C_KW = {
 
     if args.mode == "kernel":
         if args.header:
+            render_scoped_consts(parsed, cw, 'kernel')
             for _, struct in sorted(parsed.pure_nested_structs.items()):
                 if struct.request:
                     cw.p('/* Common nested types */')
-- 
2.54.0

[PATCH net 10/12] net: shaper: reject handle IDs exceeding internal bit-width

[Jakub Kicinski]

net_shaper_parse_handle() reads the user-supplied handle ID via
nla_get_u32(), accepting the full u32 range. However, the xarray key
is built by net_shaper_handle_to_index() using
FIELD_PREP(NET_SHAPER_ID_MASK, handle->id), where NET_SHAPER_ID_MASK
is GENMASK(25, 0) - only 26 bits wide. FIELD_PREP silently masks off
the upper bits at runtime. A user-supplied NODE id like 0x04000123
becomes id 0x123.

Additionally, a user-supplied id equal to NET_SHAPER_ID_UNSPEC
(0x03FFFFFF, which is NET_SHAPER_ID_MASK itself) would collide with
the sentinel used internally by the group operation to signal
"allocate a new NODE id".

Reject user-supplied IDs >= NET_SHAPER_ID_MASK (i.e., >= 0x03FFFFFF)
in the policy.

Fixes: 4b623f9f0f59 ("net-shapers: implement NL get operation")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/net_shaper.yaml | 7 +++++++
 net/shaper/shaper_nl_gen.h                  | 2 ++
 net/shaper/shaper.c                         | 4 +++-
 net/shaper/shaper_nl_gen.c                  | 7 ++++++-
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/netlink/specs/net_shaper.yaml b/Documentation/netlink/specs/net_shaper.yaml
index 3f2ad772b64b..de01f922040a 100644
--- a/Documentation/netlink/specs/net_shaper.yaml
+++ b/Documentation/netlink/specs/net_shaper.yaml
@@ -33,6 +33,11 @@ doc: |
   @cap-get operation.
 
 definitions:
+  -
+    type: const
+    name: max-handle-id
+    value: 0x3fffffe
+    scope: kernel
   -
     type: enum
     name: scope
@@ -140,6 +145,8 @@ doc: |
       -
         name: id
         type: u32
+        checks:
+          max: max-handle-id
         doc: |
           Numeric identifier of a shaper. The id semantic depends on
           the scope. For @queue scope it's the queue id and for @node
diff --git a/net/shaper/shaper_nl_gen.h b/net/shaper/shaper_nl_gen.h
index 42c46c52c775..2406652a9014 100644
--- a/net/shaper/shaper_nl_gen.h
+++ b/net/shaper/shaper_nl_gen.h
@@ -12,6 +12,8 @@
 
 #include <uapi/linux/net_shaper.h>
 
+#define NET_SHAPER_MAX_HANDLE_ID	67108862
+
 /* Common nested types */
 extern const struct nla_policy net_shaper_handle_nl_policy[NET_SHAPER_A_HANDLE_ID + 1];
 extern const struct nla_policy net_shaper_leaf_info_nl_policy[NET_SHAPER_A_WEIGHT + 1];
diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index 10d76f7148bf..16bf24d9e3ca 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -21,6 +21,8 @@
 
 #define NET_SHAPER_ID_UNSPEC NET_SHAPER_ID_MASK
 
+static_assert(NET_SHAPER_ID_UNSPEC == NET_SHAPER_MAX_HANDLE_ID + 1);
+
 struct net_shaper_hierarchy {
 	struct xarray shapers;
 };
@@ -360,7 +362,7 @@ static int net_shaper_pre_insert(struct net_shaper_binding *binding,
 	    handle->id == NET_SHAPER_ID_UNSPEC) {
 		u32 min, max;
 
-		handle->id = NET_SHAPER_ID_MASK - 1;
+		handle->id = NET_SHAPER_MAX_HANDLE_ID;
 		max = net_shaper_handle_to_index(handle);
 		handle->id = 0;
 		min = net_shaper_handle_to_index(handle);
diff --git a/net/shaper/shaper_nl_gen.c b/net/shaper/shaper_nl_gen.c
index 9b29be3ef19a..76eff85ec66d 100644
--- a/net/shaper/shaper_nl_gen.c
+++ b/net/shaper/shaper_nl_gen.c
@@ -11,10 +11,15 @@
 
 #include <uapi/linux/net_shaper.h>
 
+/* Integer value ranges */
+static const struct netlink_range_validation net_shaper_a_handle_id_range = {
+	.max	= NET_SHAPER_MAX_HANDLE_ID,
+};
+
 /* Common nested types */
 const struct nla_policy net_shaper_handle_nl_policy[NET_SHAPER_A_HANDLE_ID + 1] = {
 	[NET_SHAPER_A_HANDLE_SCOPE] = NLA_POLICY_MAX(NLA_U32, 3),
-	[NET_SHAPER_A_HANDLE_ID] = { .type = NLA_U32, },
+	[NET_SHAPER_A_HANDLE_ID] = NLA_POLICY_FULL_RANGE(NLA_U32, &net_shaper_a_handle_id_range),
 };
 
 const struct nla_policy net_shaper_leaf_info_nl_policy[NET_SHAPER_A_WEIGHT + 1] = {
-- 
2.54.0

[PATCH net 11/12] net: shaper: enforce singleton NETDEV scope with id 0

[Jakub Kicinski]

The NETDEV scope represents a singleton root shaper in the per-device
hierarchy.  All code assumes NETDEV shapers have id 0:
net_shaper_default_parent() hardcodes parent->id = 0 when returning
the NETDEV parent for QUEUE/NODE children, and the UAPI documentation
describes NETDEV scope as "the main shaper" (singular, not plural).

Make sure we reject non-0 IDs like we reject out of range queues.

Fixes: 4b623f9f0f59 ("net-shapers: implement NL get operation")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index 16bf24d9e3ca..2adf8b0e1105 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -535,6 +535,13 @@ static int net_shaper_validate_caps(struct net_shaper_binding *binding,
 		return -EOPNOTSUPP;
 	}
 
+	if (shaper->handle.scope == NET_SHAPER_SCOPE_NETDEV &&
+	    shaper->handle.id != 0) {
+		NL_SET_ERR_MSG(info->extack,
+			       "Netdev scope is a singleton, must use ID 0");
+		return -EINVAL;
+	}
+
 	if (shaper->handle.scope == NET_SHAPER_SCOPE_QUEUE &&
 	    binding->type == NET_SHAPER_BINDING_TYPE_NETDEV &&
 	    shaper->handle.id >= binding->netdev->real_num_tx_queues) {
-- 
2.54.0

[PATCH net 12/12] net: shaper: reject QUEUE scope handle with missing id

[Jakub Kicinski]

net_shaper_parse_handle() does not enforce that the user provides
the handle ID. For NODE the ID defaults to UNSPEC for all other
cases it defaults to 0.

For NETDEV 0 is the only option. For QUEUE defaulting to 0 makes
less intuitive sense. Specifically because the behavior should
(IMHO) be the same for all cases where there may be more than
one ID (QUEUE and NODE).

We should either document this as intentional or reject.
I picked the latter with no strong conviction.

Fixes: 4b623f9f0f59 ("net-shapers: implement NL get operation")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index 2adf8b0e1105..daaefc02237b 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -491,10 +491,15 @@ static int net_shaper_parse_handle(const struct nlattr *attr,
 	 * shaper (any other value).
 	 */
 	id_attr = tb[NET_SHAPER_A_HANDLE_ID];
-	if (id_attr)
+	if (id_attr) {
 		id = nla_get_u32(id_attr);
-	else if (handle->scope == NET_SHAPER_SCOPE_NODE)
+	} else if (handle->scope == NET_SHAPER_SCOPE_NODE) {
 		id = NET_SHAPER_ID_UNSPEC;
+	} else if (handle->scope == NET_SHAPER_SCOPE_QUEUE) {
+		NL_SET_ERR_ATTR_MISS(info->extack, attr,
+				     NET_SHAPER_A_HANDLE_ID);
+		return -EINVAL;
+	}
 
 	handle->id = id;
 	return 0;
-- 
2.54.0

Re: [PATCH net 09/12] tools: ynl: add scope qualifier for definitions

[Jakub Kicinski]

On Tue,  5 May 2026 17:06:25 -0700 Jakub Kicinski wrote:
> +          description: |
> +            Visibility of this definition. "uapi" (default) renders into
> +            the uAPI header, "kernel" renders into the kernel-side
> +            generated header, "user" renders into the user-side
> +            generated header. When combined with `header:`, the
> +            definition is not rendered, and the named header is
> +            included only by code matching the scope.

Clashiko says I'm not implementing enum handling even tho 
the param is generic. True, but intentional. Since this is 
primarily for policies I don't think we'll need an enum. 
We tend not to implement things we don't use in the code gen. 
I implemented header handling here (even tho it's not used)
because I think that part is tricky (read: I would have forgotten 
the plan/thinking if I didn't just implement it right away).

Re: [PATCH net 04/12] net: shaper: try to avoid violating RCU

[Paolo Abeni]

On 5/6/26 2:06 AM, Jakub Kicinski wrote:
> net_shaper_commit() overrides nodes which may be concurrently read
> under RCU. This is not a huge deal since the entries only contain
> config, worst case user will see inconsistent config params. But
> we should try to avoid this obvious RCU violation. Try to allocate
> a new node. Since commit() can't fail fall back to overriding.
> 
> Full fix is probably not worth the complexity, struct net_shaper
> is around 80B, and the allocation is with GFP_KERNEL.

I'm not sure if even this variant is worthy?!? The scheduler tree dump
could be still inconsistent, as the dump is not atomic. IMHO e.g.
inconsistent weights in the same WRR group would be as bad as
inconsistent values inside the single shaper.

/P

Re: [PATCH net 01/12] net: shaper: drop redundant xa_lock() bracketing

[Paolo Abeni]

On 5/6/26 2:06 AM, Jakub Kicinski wrote:
> The shaper insertion code is written in a way that suggests that
> perhaps it was expecting readers to be fenced off by xa_lock.

FTR, the code was shaped (pun intended :) that way to avoid acquiring
multiple times the xa_lock when not needed. Sort of evil early optimization.

/P

Re: [PATCH net 04/12] net: shaper: try to avoid violating RCU

[Paolo Abeni]

On 5/6/26 5:22 PM, Paolo Abeni wrote:
> On 5/6/26 2:06 AM, Jakub Kicinski wrote:
>> net_shaper_commit() overrides nodes which may be concurrently read
>> under RCU. This is not a huge deal since the entries only contain
>> config, worst case user will see inconsistent config params. But
>> we should try to avoid this obvious RCU violation. Try to allocate
>> a new node. Since commit() can't fail fall back to overriding.
>>
>> Full fix is probably not worth the complexity, struct net_shaper
>> is around 80B, and the allocation is with GFP_KERNEL.
> 
> I'm not sure if even this variant is worthy?!? The scheduler tree dump
> could be still inconsistent, as the dump is not atomic. IMHO e.g.
> inconsistent weights in the same WRR group would be as bad as
> inconsistent values inside the single shaper.

I mean: I would simply avoid addressing this RCU violation.

/P

Re: [PATCH net 06/12] selftests: drv-net: add shaper test for duplicate leaves

[Breno Leitao]

On Tue, May 05, 2026 at 05:06:22PM -0700, Jakub Kicinski wrote:
> Add test exercising duplicate leaves.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Breno Leitao <leitao@debian.org>

> diff --git a/tools/testing/selftests/drivers/net/shaper.py b/tools/testing/selftests/drivers/net/shaper.py
> index 11310f19bfa0..f7872c931bf6 100755
> --- a/tools/testing/selftests/drivers/net/shaper.py
> +++ b/tools/testing/selftests/drivers/net/shaper.py


>  def main() -> None:
>      with NetDrvEnv(__file__, queue_count=4) as cfg:
>          cfg.queues = False
> @@ -453,7 +471,10 @@ from lib.py import cmd
>                    basic_groups,
>                    qgroups,
>                    delegation,
> -                  queue_update], args=(cfg, NetshaperFamily()))
> +                  queue_update,
> +                  dup_leaves
> +                  ],

Nit: dup_leaves], on a single line matches the surrounding style and
avoids the dangling bracket on its own line. Not worth a respin on its
own.

Re: [PATCH net 01/12] net: shaper: drop redundant xa_lock() bracketing

[Jakub Kicinski]

On Wed, 6 May 2026 17:30:32 +0200 Paolo Abeni wrote:
> On 5/6/26 2:06 AM, Jakub Kicinski wrote:
> > The shaper insertion code is written in a way that suggests that
> > perhaps it was expecting readers to be fenced off by xa_lock.  
> 
> FTR, the code was shaped (pun intended :) that way to avoid acquiring
> multiple times the xa_lock when not needed. Sort of evil early optimization.

Ah, that makes more sense, I guess. Do you prefer me to drop this patch?
The XA lock is guaranteed not to be contended since we're under the
netdev mutex..

Re: [PATCH net 04/12] net: shaper: try to avoid violating RCU

[Jakub Kicinski]

On Wed, 6 May 2026 17:32:10 +0200 Paolo Abeni wrote:
> On 5/6/26 5:22 PM, Paolo Abeni wrote:
> > On 5/6/26 2:06 AM, Jakub Kicinski wrote:  
> >> net_shaper_commit() overrides nodes which may be concurrently read
> >> under RCU. This is not a huge deal since the entries only contain
> >> config, worst case user will see inconsistent config params. But
> >> we should try to avoid this obvious RCU violation. Try to allocate
> >> a new node. Since commit() can't fail fall back to overriding.
> >>
> >> Full fix is probably not worth the complexity, struct net_shaper
> >> is around 80B, and the allocation is with GFP_KERNEL.  
> > 
> > I'm not sure if even this variant is worthy?!? The scheduler tree dump
> > could be still inconsistent, as the dump is not atomic. IMHO e.g.
> > inconsistent weights in the same WRR group would be as bad as
> > inconsistent values inside the single shaper.  
> 
> I mean: I would simply avoid addressing this RCU violation.

Alright. I'll add a comment instead (fix just the ordering WRT 
to the VALID flag)

Re: [PATCH net 06/12] selftests: drv-net: add shaper test for duplicate leaves

[Jakub Kicinski]

On Wed, 6 May 2026 09:40:07 -0700 Breno Leitao wrote:
> > @@ -453,7 +471,10 @@ from lib.py import cmd
> >                    basic_groups,
> >                    qgroups,
> >                    delegation,
> > -                  queue_update], args=(cfg, NetshaperFamily()))
> > +                  queue_update,
> > +                  dup_leaves
> > +                  ],  
> 
> Nit: dup_leaves], on a single line matches the surrounding style and
> avoids the dangling bracket on its own line. Not worth a respin on its
> own.

It was intentional to avoid having to touch the lines when adding more
tests. Now that I think about it again I should probably keep the tests
sorted and add a trailing ,

Re: [PATCH net 01/12] net: shaper: drop redundant xa_lock() bracketing

[Paolo Abeni]

On 5/7/26 12:33 AM, Jakub Kicinski wrote:
> On Wed, 6 May 2026 17:30:32 +0200 Paolo Abeni wrote:
>> On 5/6/26 2:06 AM, Jakub Kicinski wrote:
>>> The shaper insertion code is written in a way that suggests that
>>> perhaps it was expecting readers to be fenced off by xa_lock.  
>>
>> FTR, the code was shaped (pun intended :) that way to avoid acquiring
>> multiple times the xa_lock when not needed. Sort of evil early optimization.
> 
> Ah, that makes more sense, I guess. Do you prefer me to drop this patch?
> The XA lock is guaranteed not to be contended since we're under the
> netdev mutex..

Sorry, I should have been more clear: I'm fine with the patch, just
giving more context, as far as I remembered it.

/P