[PATCH net 1/2] net: shaper: protect late read accesses to the hierarchy

[PATCH net 1/2] net: shaper: protect late read accesses to the hierarchy

[Jakub Kicinski]

We look up a netdev during prep of Netlink ops (pre- callbacks)
and take a ref to it. Then later in the body of the callback
we take its lock or RCU which are the actual protections.

This is not proper, a conversion from a ref to a locked netdev
must include a liveness check (a check if the netdev hasn't been
unregistered already). Fix the read cases (those under RCU).
Writes needs a separate change to protect from creating the
hierarchy after flush has already run.

Fixes: 4b623f9f0f59 ("net-shapers: implement NL get operation")
Reported-by: Paul Moses <p@1g4.org>
Link: https://lore.kernel.org/20260309173450.538026-1-p@1g4.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/shaper/shaper.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index 3fd6629cb999..6b4c87e12f1f 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -65,6 +65,21 @@ net_shaper_hierarchy(struct net_shaper_binding *binding)
 	return NULL;
 }
 
+static struct net_shaper_hierarchy *
+net_shaper_hierarchy_rcu(struct net_shaper_binding *binding)
+{
+	/* Readers look up the device and take a ref, then take RCU lock
+	 * later at which point netdev may have been unregistered and flushed.
+	 * READ_ONCE() pairs with WRITE_ONCE() in net_shaper_hierarchy_setup.
+	 */
+	if (binding->type == NET_SHAPER_BINDING_TYPE_NETDEV &&
+	    READ_ONCE(binding->netdev->reg_state) <= NETREG_REGISTERED)
+		return READ_ONCE(binding->netdev->net_shaper_hierarchy);
+
+	/* No other type supported yet. */
+	return NULL;
+}
+
 static const struct net_shaper_ops *
 net_shaper_ops(struct net_shaper_binding *binding)
 {
@@ -251,9 +266,10 @@ static struct net_shaper *
 net_shaper_lookup(struct net_shaper_binding *binding,
 		  const struct net_shaper_handle *handle)
 {
-	struct net_shaper_hierarchy *hierarchy = net_shaper_hierarchy(binding);
 	u32 index = net_shaper_handle_to_index(handle);
+	struct net_shaper_hierarchy *hierarchy;
 
+	hierarchy = net_shaper_hierarchy_rcu(binding);
 	if (!hierarchy || xa_get_mark(&hierarchy->shapers, index,
 				      NET_SHAPER_NOT_VALID))
 		return NULL;
@@ -778,17 +794,19 @@ int net_shaper_nl_get_dumpit(struct sk_buff *skb,
 
 	/* Don't error out dumps performed before any set operation. */
 	binding = net_shaper_binding_from_ctx(ctx);
-	hierarchy = net_shaper_hierarchy(binding);
-	if (!hierarchy)
-		return 0;
 
 	rcu_read_lock();
+	hierarchy = net_shaper_hierarchy_rcu(binding);
+	if (!hierarchy)
+		goto out_unlock;
+
 	for (; (shaper = xa_find(&hierarchy->shapers, &ctx->start_index,
 				 U32_MAX, XA_PRESENT)); ctx->start_index++) {
 		ret = net_shaper_fill_one(skb, binding, shaper, info);
 		if (ret)
 			break;
 	}
+out_unlock:
 	rcu_read_unlock();
 
 	return ret;
-- 
2.53.0

[PATCH net 2/2] net: shaper: protect from late creation of hierarchy

[Jakub Kicinski]

We look up a netdev during prep of Netlink ops (pre- callbacks)
and take a ref to it. Then later in the body of the callback
we take its lock or RCU which are the actual protections.

The netdev may get unregistered in between the time we take
the ref and the time we lock it. We may allocate the hierarchy
after flush has already run, which would lead to a leak.

Take the instance lock in pre- already, this saves us from the race
and removes the need for dedicated lock/unlock callbacks completely.
After all, if there's any chance of write happening concurrently
with the flush - we're back to leaking the hierarchy.

We may take the lock for devices which don't support shapers but
we're only dealing with SET operations here, not taking the lock
would be optimizing for an error case.

Fixes: 93954b40f6a4 ("net-shapers: implement NL set and delete operations")
Link: https://lore.kernel.org/20260309173450.538026-1-p@1g4.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/net_shaper.yaml |  12 +-
 net/shaper/shaper_nl_gen.h                  |   5 +
 net/shaper/shaper.c                         | 134 +++++++++++---------
 net/shaper/shaper_nl_gen.c                  |  12 +-
 4 files changed, 89 insertions(+), 74 deletions(-)

diff --git a/Documentation/netlink/specs/net_shaper.yaml b/Documentation/netlink/specs/net_shaper.yaml
index 0b1b54be48f9..3f2ad772b64b 100644
--- a/Documentation/netlink/specs/net_shaper.yaml
+++ b/Documentation/netlink/specs/net_shaper.yaml
@@ -247,8 +247,8 @@ doc: |
       flags: [admin-perm]
 
       do:
-        pre: net-shaper-nl-pre-doit
-        post: net-shaper-nl-post-doit
+        pre: net-shaper-nl-pre-doit-write
+        post: net-shaper-nl-post-doit-write
         request:
           attributes:
             - ifindex
@@ -278,8 +278,8 @@ doc: |
       flags: [admin-perm]
 
       do:
-        pre: net-shaper-nl-pre-doit
-        post: net-shaper-nl-post-doit
+        pre: net-shaper-nl-pre-doit-write
+        post: net-shaper-nl-post-doit-write
         request:
           attributes: *ns-binding
 
@@ -309,8 +309,8 @@ doc: |
       flags: [admin-perm]
 
       do:
-        pre: net-shaper-nl-pre-doit
-        post: net-shaper-nl-post-doit
+        pre: net-shaper-nl-pre-doit-write
+        post: net-shaper-nl-post-doit-write
         request:
           attributes:
             - ifindex
diff --git a/net/shaper/shaper_nl_gen.h b/net/shaper/shaper_nl_gen.h
index ec41c90431a4..42c46c52c775 100644
--- a/net/shaper/shaper_nl_gen.h
+++ b/net/shaper/shaper_nl_gen.h
@@ -18,12 +18,17 @@ extern const struct nla_policy net_shaper_leaf_info_nl_policy[NET_SHAPER_A_WEIGH
 
 int net_shaper_nl_pre_doit(const struct genl_split_ops *ops,
 			   struct sk_buff *skb, struct genl_info *info);
+int net_shaper_nl_pre_doit_write(const struct genl_split_ops *ops,
+				 struct sk_buff *skb, struct genl_info *info);
 int net_shaper_nl_cap_pre_doit(const struct genl_split_ops *ops,
 			       struct sk_buff *skb, struct genl_info *info);
 void
 net_shaper_nl_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
 			struct genl_info *info);
 void
+net_shaper_nl_post_doit_write(const struct genl_split_ops *ops,
+			      struct sk_buff *skb, struct genl_info *info);
+void
 net_shaper_nl_cap_post_doit(const struct genl_split_ops *ops,
 			    struct sk_buff *skb, struct genl_info *info);
 int net_shaper_nl_pre_dumpit(struct netlink_callback *cb);
diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index 6b4c87e12f1f..94bc9c7382ea 100644
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -36,24 +36,6 @@ static struct net_shaper_binding *net_shaper_binding_from_ctx(void *ctx)
 	return &((struct net_shaper_nl_ctx *)ctx)->binding;
 }
 
-static void net_shaper_lock(struct net_shaper_binding *binding)
-{
-	switch (binding->type) {
-	case NET_SHAPER_BINDING_TYPE_NETDEV:
-		netdev_lock(binding->netdev);
-		break;
-	}
-}
-
-static void net_shaper_unlock(struct net_shaper_binding *binding)
-{
-	switch (binding->type) {
-	case NET_SHAPER_BINDING_TYPE_NETDEV:
-		netdev_unlock(binding->netdev);
-		break;
-	}
-}
-
 static struct net_shaper_hierarchy *
 net_shaper_hierarchy(struct net_shaper_binding *binding)
 {
@@ -219,12 +201,49 @@ static int net_shaper_ctx_setup(const struct genl_info *info, int type,
 	return 0;
 }
 
+/* Like net_shaper_ctx_setup(), but for "write" handlers (never for dumps!)
+ * Acquires the lock protecting the hierarchy (instance lock for netdev).
+ */
+static int net_shaper_ctx_setup_lock(const struct genl_info *info, int type,
+				     struct net_shaper_nl_ctx *ctx)
+{
+	struct net *ns = genl_info_net(info);
+	struct net_device *dev;
+	int ifindex;
+
+	if (GENL_REQ_ATTR_CHECK(info, type))
+		return -EINVAL;
+
+	ifindex = nla_get_u32(info->attrs[type]);
+	dev = netdev_get_by_index_lock(ns, ifindex);
+	if (!dev) {
+		NL_SET_BAD_ATTR(info->extack, info->attrs[type]);
+		return -ENOENT;
+	}
+
+	if (!dev->netdev_ops->net_shaper_ops) {
+		NL_SET_BAD_ATTR(info->extack, info->attrs[type]);
+		netdev_unlock(dev);
+		return -EOPNOTSUPP;
+	}
+
+	ctx->binding.type = NET_SHAPER_BINDING_TYPE_NETDEV;
+	ctx->binding.netdev = dev;
+	return 0;
+}
+
 static void net_shaper_ctx_cleanup(struct net_shaper_nl_ctx *ctx)
 {
 	if (ctx->binding.type == NET_SHAPER_BINDING_TYPE_NETDEV)
 		netdev_put(ctx->binding.netdev, &ctx->dev_tracker);
 }
 
+static void net_shaper_ctx_cleanup_unlock(struct net_shaper_nl_ctx *ctx)
+{
+	if (ctx->binding.type == NET_SHAPER_BINDING_TYPE_NETDEV)
+		netdev_unlock(ctx->binding.netdev);
+}
+
 static u32 net_shaper_handle_to_index(const struct net_shaper_handle *handle)
 {
 	return FIELD_PREP(NET_SHAPER_SCOPE_MASK, handle->scope) |
@@ -278,7 +297,7 @@ net_shaper_lookup(struct net_shaper_binding *binding,
 }
 
 /* Allocate on demand the per device shaper's hierarchy container.
- * Called under the net shaper lock
+ * Called under the lock protecting the hierarchy (instance lock for netdev)
  */
 static struct net_shaper_hierarchy *
 net_shaper_hierarchy_setup(struct net_shaper_binding *binding)
@@ -697,6 +716,22 @@ void net_shaper_nl_post_doit(const struct genl_split_ops *ops,
 	net_shaper_generic_post(info);
 }
 
+int net_shaper_nl_pre_doit_write(const struct genl_split_ops *ops,
+				struct sk_buff *skb, struct genl_info *info)
+{
+	struct net_shaper_nl_ctx *ctx = (struct net_shaper_nl_ctx *)info->ctx;
+
+	BUILD_BUG_ON(sizeof(*ctx) > sizeof(info->ctx));
+
+	return net_shaper_ctx_setup_lock(info, NET_SHAPER_A_IFINDEX, ctx);
+}
+
+void net_shaper_nl_post_doit_write(const struct genl_split_ops *ops,
+				   struct sk_buff *skb, struct genl_info *info)
+{
+	net_shaper_ctx_cleanup_unlock((struct net_shaper_nl_ctx *)info->ctx);
+}
+
 int net_shaper_nl_pre_dumpit(struct netlink_callback *cb)
 {
 	struct net_shaper_nl_ctx *ctx = (struct net_shaper_nl_ctx *)cb->ctx;
@@ -824,45 +859,38 @@ int net_shaper_nl_set_doit(struct sk_buff *skb, struct genl_info *info)
 
 	binding = net_shaper_binding_from_ctx(info->ctx);
 
-	net_shaper_lock(binding);
 	ret = net_shaper_parse_info(binding, info->attrs, info, &shaper,
 				    &exists);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	if (!exists)
 		net_shaper_default_parent(&shaper.handle, &shaper.parent);
 
 	hierarchy = net_shaper_hierarchy_setup(binding);
-	if (!hierarchy) {
-		ret = -ENOMEM;
-		goto unlock;
-	}
+	if (!hierarchy)
+		return -ENOMEM;
 
 	/* The 'set' operation can't create node-scope shapers. */
 	handle = shaper.handle;
 	if (handle.scope == NET_SHAPER_SCOPE_NODE &&
-	    !net_shaper_lookup(binding, &handle)) {
-		ret = -ENOENT;
-		goto unlock;
-	}
+	    !net_shaper_lookup(binding, &handle))
+		return -ENOENT;
 
 	ret = net_shaper_pre_insert(binding, &handle, info->extack);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	ops = net_shaper_ops(binding);
 	ret = ops->set(binding, &shaper, info->extack);
 	if (ret) {
 		net_shaper_rollback(binding);
-		goto unlock;
+		return ret;
 	}
 
 	net_shaper_commit(binding, 1, &shaper);
 
-unlock:
-	net_shaper_unlock(binding);
-	return ret;
+	return 0;
 }
 
 static int __net_shaper_delete(struct net_shaper_binding *binding,
@@ -1090,35 +1118,26 @@ int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info)
 
 	binding = net_shaper_binding_from_ctx(info->ctx);
 
-	net_shaper_lock(binding);
 	ret = net_shaper_parse_handle(info->attrs[NET_SHAPER_A_HANDLE], info,
 				      &handle);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	hierarchy = net_shaper_hierarchy(binding);
-	if (!hierarchy) {
-		ret = -ENOENT;
-		goto unlock;
-	}
+	if (!hierarchy)
+		return -ENOENT;
 
 	shaper = net_shaper_lookup(binding, &handle);
-	if (!shaper) {
-		ret = -ENOENT;
-		goto unlock;
-	}
+	if (!shaper)
+		return -ENOENT;
 
 	if (handle.scope == NET_SHAPER_SCOPE_NODE) {
 		ret = net_shaper_pre_del_node(binding, shaper, info->extack);
 		if (ret)
-			goto unlock;
+			return ret;
 	}
 
-	ret = __net_shaper_delete(binding, shaper, info->extack);
-
-unlock:
-	net_shaper_unlock(binding);
-	return ret;
+	return __net_shaper_delete(binding, shaper, info->extack);
 }
 
 static int net_shaper_group_send_reply(struct net_shaper_binding *binding,
@@ -1167,21 +1186,17 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
 	if (!net_shaper_ops(binding)->group)
 		return -EOPNOTSUPP;
 
-	net_shaper_lock(binding);
 	leaves_count = net_shaper_list_len(info, NET_SHAPER_A_LEAVES);
 	if (!leaves_count) {
 		NL_SET_BAD_ATTR(info->extack,
 				info->attrs[NET_SHAPER_A_LEAVES]);
-		ret = -EINVAL;
-		goto unlock;
+		return -EINVAL;
 	}
 
 	leaves = kcalloc(leaves_count, sizeof(struct net_shaper) +
 			 sizeof(struct net_shaper *), GFP_KERNEL);
-	if (!leaves) {
-		ret = -ENOMEM;
-		goto unlock;
-	}
+	if (!leaves)
+		return -ENOMEM;
 	old_nodes = (void *)&leaves[leaves_count];
 
 	ret = net_shaper_parse_node(binding, info->attrs, info, &node);
@@ -1258,9 +1273,6 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
 
 free_leaves:
 	kfree(leaves);
-
-unlock:
-	net_shaper_unlock(binding);
 	return ret;
 
 free_msg:
@@ -1370,14 +1382,12 @@ static void net_shaper_flush(struct net_shaper_binding *binding)
 	if (!hierarchy)
 		return;
 
-	net_shaper_lock(binding);
 	xa_lock(&hierarchy->shapers);
 	xa_for_each(&hierarchy->shapers, index, cur) {
 		__xa_erase(&hierarchy->shapers, index);
 		kfree(cur);
 	}
 	xa_unlock(&hierarchy->shapers);
-	net_shaper_unlock(binding);
 
 	kfree(hierarchy);
 }
diff --git a/net/shaper/shaper_nl_gen.c b/net/shaper/shaper_nl_gen.c
index e8cccc4c1180..9b29be3ef19a 100644
--- a/net/shaper/shaper_nl_gen.c
+++ b/net/shaper/shaper_nl_gen.c
@@ -99,27 +99,27 @@ static const struct genl_split_ops net_shaper_nl_ops[] = {
 	},
 	{
 		.cmd		= NET_SHAPER_CMD_SET,
-		.pre_doit	= net_shaper_nl_pre_doit,
+		.pre_doit	= net_shaper_nl_pre_doit_write,
 		.doit		= net_shaper_nl_set_doit,
-		.post_doit	= net_shaper_nl_post_doit,
+		.post_doit	= net_shaper_nl_post_doit_write,
 		.policy		= net_shaper_set_nl_policy,
 		.maxattr	= NET_SHAPER_A_IFINDEX,
 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
 	},
 	{
 		.cmd		= NET_SHAPER_CMD_DELETE,
-		.pre_doit	= net_shaper_nl_pre_doit,
+		.pre_doit	= net_shaper_nl_pre_doit_write,
 		.doit		= net_shaper_nl_delete_doit,
-		.post_doit	= net_shaper_nl_post_doit,
+		.post_doit	= net_shaper_nl_post_doit_write,
 		.policy		= net_shaper_delete_nl_policy,
 		.maxattr	= NET_SHAPER_A_IFINDEX,
 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
 	},
 	{
 		.cmd		= NET_SHAPER_CMD_GROUP,
-		.pre_doit	= net_shaper_nl_pre_doit,
+		.pre_doit	= net_shaper_nl_pre_doit_write,
 		.doit		= net_shaper_nl_group_doit,
-		.post_doit	= net_shaper_nl_post_doit,
+		.post_doit	= net_shaper_nl_post_doit_write,
 		.policy		= net_shaper_group_nl_policy,
 		.maxattr	= NET_SHAPER_A_LEAVES,
 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
-- 
2.53.0

Re: [PATCH net 2/2] net: shaper: protect from late creation of hierarchy

[Paul Moses]

Congrats, now tell everyone that I did close the memory safety
issue, you just added a cleanup. ;)



On Tuesday, March 17th, 2026 at 11:10 AM, Jakub Kicinski <kuba@kernel.org> wrote:

> We look up a netdev during prep of Netlink ops (pre- callbacks)
> and take a ref to it. Then later in the body of the callback
> we take its lock or RCU which are the actual protections.
> 
> The netdev may get unregistered in between the time we take
> the ref and the time we lock it. We may allocate the hierarchy
> after flush has already run, which would lead to a leak.
> 
> Take the instance lock in pre- already, this saves us from the race
> and removes the need for dedicated lock/unlock callbacks completely.
> After all, if there's any chance of write happening concurrently
> with the flush - we're back to leaking the hierarchy.
> 
> We may take the lock for devices which don't support shapers but
> we're only dealing with SET operations here, not taking the lock
> would be optimizing for an error case.
> 
> Fixes: 93954b40f6a4 ("net-shapers: implement NL set and delete operations")
> Link: https://lore.kernel.org/20260309173450.538026-1-p@1g4.org
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  Documentation/netlink/specs/net_shaper.yaml |  12 +-
>  net/shaper/shaper_nl_gen.h                  |   5 +
>  net/shaper/shaper.c                         | 134 +++++++++++---------
>  net/shaper/shaper_nl_gen.c                  |  12 +-
>  4 files changed, 89 insertions(+), 74 deletions(-)
> 
> diff --git a/Documentation/netlink/specs/net_shaper.yaml b/Documentation/netlink/specs/net_shaper.yaml
> index 0b1b54be48f9..3f2ad772b64b 100644
> --- a/Documentation/netlink/specs/net_shaper.yaml
> +++ b/Documentation/netlink/specs/net_shaper.yaml
> @@ -247,8 +247,8 @@ doc: |
>        flags: [admin-perm]
> 
>        do:
> -        pre: net-shaper-nl-pre-doit
> -        post: net-shaper-nl-post-doit
> +        pre: net-shaper-nl-pre-doit-write
> +        post: net-shaper-nl-post-doit-write
>          request:
>            attributes:
>              - ifindex
> @@ -278,8 +278,8 @@ doc: |
>        flags: [admin-perm]
> 
>        do:
> -        pre: net-shaper-nl-pre-doit
> -        post: net-shaper-nl-post-doit
> +        pre: net-shaper-nl-pre-doit-write
> +        post: net-shaper-nl-post-doit-write
>          request:
>            attributes: *ns-binding
> 
> @@ -309,8 +309,8 @@ doc: |
>        flags: [admin-perm]
> 
>        do:
> -        pre: net-shaper-nl-pre-doit
> -        post: net-shaper-nl-post-doit
> +        pre: net-shaper-nl-pre-doit-write
> +        post: net-shaper-nl-post-doit-write
>          request:
>            attributes:
>              - ifindex
> diff --git a/net/shaper/shaper_nl_gen.h b/net/shaper/shaper_nl_gen.h
> index ec41c90431a4..42c46c52c775 100644
> --- a/net/shaper/shaper_nl_gen.h
> +++ b/net/shaper/shaper_nl_gen.h
> @@ -18,12 +18,17 @@ extern const struct nla_policy net_shaper_leaf_info_nl_policy[NET_SHAPER_A_WEIGH
> 
>  int net_shaper_nl_pre_doit(const struct genl_split_ops *ops,
>  			   struct sk_buff *skb, struct genl_info *info);
> +int net_shaper_nl_pre_doit_write(const struct genl_split_ops *ops,
> +				 struct sk_buff *skb, struct genl_info *info);
>  int net_shaper_nl_cap_pre_doit(const struct genl_split_ops *ops,
>  			       struct sk_buff *skb, struct genl_info *info);
>  void
>  net_shaper_nl_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>  			struct genl_info *info);
>  void
> +net_shaper_nl_post_doit_write(const struct genl_split_ops *ops,
> +			      struct sk_buff *skb, struct genl_info *info);
> +void
>  net_shaper_nl_cap_post_doit(const struct genl_split_ops *ops,
>  			    struct sk_buff *skb, struct genl_info *info);
>  int net_shaper_nl_pre_dumpit(struct netlink_callback *cb);
> diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
> index 6b4c87e12f1f..94bc9c7382ea 100644
> --- a/net/shaper/shaper.c
> +++ b/net/shaper/shaper.c
> @@ -36,24 +36,6 @@ static struct net_shaper_binding *net_shaper_binding_from_ctx(void *ctx)
>  	return &((struct net_shaper_nl_ctx *)ctx)->binding;
>  }
> 
> -static void net_shaper_lock(struct net_shaper_binding *binding)
> -{
> -	switch (binding->type) {
> -	case NET_SHAPER_BINDING_TYPE_NETDEV:
> -		netdev_lock(binding->netdev);
> -		break;
> -	}
> -}
> -
> -static void net_shaper_unlock(struct net_shaper_binding *binding)
> -{
> -	switch (binding->type) {
> -	case NET_SHAPER_BINDING_TYPE_NETDEV:
> -		netdev_unlock(binding->netdev);
> -		break;
> -	}
> -}
> -
>  static struct net_shaper_hierarchy *
>  net_shaper_hierarchy(struct net_shaper_binding *binding)
>  {
> @@ -219,12 +201,49 @@ static int net_shaper_ctx_setup(const struct genl_info *info, int type,
>  	return 0;
>  }
> 
> +/* Like net_shaper_ctx_setup(), but for "write" handlers (never for dumps!)
> + * Acquires the lock protecting the hierarchy (instance lock for netdev).
> + */
> +static int net_shaper_ctx_setup_lock(const struct genl_info *info, int type,
> +				     struct net_shaper_nl_ctx *ctx)
> +{
> +	struct net *ns = genl_info_net(info);
> +	struct net_device *dev;
> +	int ifindex;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, type))
> +		return -EINVAL;
> +
> +	ifindex = nla_get_u32(info->attrs[type]);
> +	dev = netdev_get_by_index_lock(ns, ifindex);
> +	if (!dev) {
> +		NL_SET_BAD_ATTR(info->extack, info->attrs[type]);
> +		return -ENOENT;
> +	}
> +
> +	if (!dev->netdev_ops->net_shaper_ops) {
> +		NL_SET_BAD_ATTR(info->extack, info->attrs[type]);
> +		netdev_unlock(dev);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ctx->binding.type = NET_SHAPER_BINDING_TYPE_NETDEV;
> +	ctx->binding.netdev = dev;
> +	return 0;
> +}
> +
>  static void net_shaper_ctx_cleanup(struct net_shaper_nl_ctx *ctx)
>  {
>  	if (ctx->binding.type == NET_SHAPER_BINDING_TYPE_NETDEV)
>  		netdev_put(ctx->binding.netdev, &ctx->dev_tracker);
>  }
> 
> +static void net_shaper_ctx_cleanup_unlock(struct net_shaper_nl_ctx *ctx)
> +{
> +	if (ctx->binding.type == NET_SHAPER_BINDING_TYPE_NETDEV)
> +		netdev_unlock(ctx->binding.netdev);
> +}
> +
>  static u32 net_shaper_handle_to_index(const struct net_shaper_handle *handle)
>  {
>  	return FIELD_PREP(NET_SHAPER_SCOPE_MASK, handle->scope) |
> @@ -278,7 +297,7 @@ net_shaper_lookup(struct net_shaper_binding *binding,
>  }
> 
>  /* Allocate on demand the per device shaper's hierarchy container.
> - * Called under the net shaper lock
> + * Called under the lock protecting the hierarchy (instance lock for netdev)
>   */
>  static struct net_shaper_hierarchy *
>  net_shaper_hierarchy_setup(struct net_shaper_binding *binding)
> @@ -697,6 +716,22 @@ void net_shaper_nl_post_doit(const struct genl_split_ops *ops,
>  	net_shaper_generic_post(info);
>  }
> 
> +int net_shaper_nl_pre_doit_write(const struct genl_split_ops *ops,
> +				struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct net_shaper_nl_ctx *ctx = (struct net_shaper_nl_ctx *)info->ctx;
> +
> +	BUILD_BUG_ON(sizeof(*ctx) > sizeof(info->ctx));
> +
> +	return net_shaper_ctx_setup_lock(info, NET_SHAPER_A_IFINDEX, ctx);
> +}
> +
> +void net_shaper_nl_post_doit_write(const struct genl_split_ops *ops,
> +				   struct sk_buff *skb, struct genl_info *info)
> +{
> +	net_shaper_ctx_cleanup_unlock((struct net_shaper_nl_ctx *)info->ctx);
> +}
> +
>  int net_shaper_nl_pre_dumpit(struct netlink_callback *cb)
>  {
>  	struct net_shaper_nl_ctx *ctx = (struct net_shaper_nl_ctx *)cb->ctx;
> @@ -824,45 +859,38 @@ int net_shaper_nl_set_doit(struct sk_buff *skb, struct genl_info *info)
> 
>  	binding = net_shaper_binding_from_ctx(info->ctx);
> 
> -	net_shaper_lock(binding);
>  	ret = net_shaper_parse_info(binding, info->attrs, info, &shaper,
>  				    &exists);
>  	if (ret)
> -		goto unlock;
> +		return ret;
> 
>  	if (!exists)
>  		net_shaper_default_parent(&shaper.handle, &shaper.parent);
> 
>  	hierarchy = net_shaper_hierarchy_setup(binding);
> -	if (!hierarchy) {
> -		ret = -ENOMEM;
> -		goto unlock;
> -	}
> +	if (!hierarchy)
> +		return -ENOMEM;
> 
>  	/* The 'set' operation can't create node-scope shapers. */
>  	handle = shaper.handle;
>  	if (handle.scope == NET_SHAPER_SCOPE_NODE &&
> -	    !net_shaper_lookup(binding, &handle)) {
> -		ret = -ENOENT;
> -		goto unlock;
> -	}
> +	    !net_shaper_lookup(binding, &handle))
> +		return -ENOENT;
> 
>  	ret = net_shaper_pre_insert(binding, &handle, info->extack);
>  	if (ret)
> -		goto unlock;
> +		return ret;
> 
>  	ops = net_shaper_ops(binding);
>  	ret = ops->set(binding, &shaper, info->extack);
>  	if (ret) {
>  		net_shaper_rollback(binding);
> -		goto unlock;
> +		return ret;
>  	}
> 
>  	net_shaper_commit(binding, 1, &shaper);
> 
> -unlock:
> -	net_shaper_unlock(binding);
> -	return ret;
> +	return 0;
>  }
> 
>  static int __net_shaper_delete(struct net_shaper_binding *binding,
> @@ -1090,35 +1118,26 @@ int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info)
> 
>  	binding = net_shaper_binding_from_ctx(info->ctx);
> 
> -	net_shaper_lock(binding);
>  	ret = net_shaper_parse_handle(info->attrs[NET_SHAPER_A_HANDLE], info,
>  				      &handle);
>  	if (ret)
> -		goto unlock;
> +		return ret;
> 
>  	hierarchy = net_shaper_hierarchy(binding);
> -	if (!hierarchy) {
> -		ret = -ENOENT;
> -		goto unlock;
> -	}
> +	if (!hierarchy)
> +		return -ENOENT;
> 
>  	shaper = net_shaper_lookup(binding, &handle);
> -	if (!shaper) {
> -		ret = -ENOENT;
> -		goto unlock;
> -	}
> +	if (!shaper)
> +		return -ENOENT;
> 
>  	if (handle.scope == NET_SHAPER_SCOPE_NODE) {
>  		ret = net_shaper_pre_del_node(binding, shaper, info->extack);
>  		if (ret)
> -			goto unlock;
> +			return ret;
>  	}
> 
> -	ret = __net_shaper_delete(binding, shaper, info->extack);
> -
> -unlock:
> -	net_shaper_unlock(binding);
> -	return ret;
> +	return __net_shaper_delete(binding, shaper, info->extack);
>  }
> 
>  static int net_shaper_group_send_reply(struct net_shaper_binding *binding,
> @@ -1167,21 +1186,17 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
>  	if (!net_shaper_ops(binding)->group)
>  		return -EOPNOTSUPP;
> 
> -	net_shaper_lock(binding);
>  	leaves_count = net_shaper_list_len(info, NET_SHAPER_A_LEAVES);
>  	if (!leaves_count) {
>  		NL_SET_BAD_ATTR(info->extack,
>  				info->attrs[NET_SHAPER_A_LEAVES]);
> -		ret = -EINVAL;
> -		goto unlock;
> +		return -EINVAL;
>  	}
> 
>  	leaves = kcalloc(leaves_count, sizeof(struct net_shaper) +
>  			 sizeof(struct net_shaper *), GFP_KERNEL);
> -	if (!leaves) {
> -		ret = -ENOMEM;
> -		goto unlock;
> -	}
> +	if (!leaves)
> +		return -ENOMEM;
>  	old_nodes = (void *)&leaves[leaves_count];
> 
>  	ret = net_shaper_parse_node(binding, info->attrs, info, &node);
> @@ -1258,9 +1273,6 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
> 
>  free_leaves:
>  	kfree(leaves);
> -
> -unlock:
> -	net_shaper_unlock(binding);
>  	return ret;
> 
>  free_msg:
> @@ -1370,14 +1382,12 @@ static void net_shaper_flush(struct net_shaper_binding *binding)
>  	if (!hierarchy)
>  		return;
> 
> -	net_shaper_lock(binding);
>  	xa_lock(&hierarchy->shapers);
>  	xa_for_each(&hierarchy->shapers, index, cur) {
>  		__xa_erase(&hierarchy->shapers, index);
>  		kfree(cur);
>  	}
>  	xa_unlock(&hierarchy->shapers);
> -	net_shaper_unlock(binding);
> 
>  	kfree(hierarchy);
>  }
> diff --git a/net/shaper/shaper_nl_gen.c b/net/shaper/shaper_nl_gen.c
> index e8cccc4c1180..9b29be3ef19a 100644
> --- a/net/shaper/shaper_nl_gen.c
> +++ b/net/shaper/shaper_nl_gen.c
> @@ -99,27 +99,27 @@ static const struct genl_split_ops net_shaper_nl_ops[] = {
>  	},
>  	{
>  		.cmd		= NET_SHAPER_CMD_SET,
> -		.pre_doit	= net_shaper_nl_pre_doit,
> +		.pre_doit	= net_shaper_nl_pre_doit_write,
>  		.doit		= net_shaper_nl_set_doit,
> -		.post_doit	= net_shaper_nl_post_doit,
> +		.post_doit	= net_shaper_nl_post_doit_write,
>  		.policy		= net_shaper_set_nl_policy,
>  		.maxattr	= NET_SHAPER_A_IFINDEX,
>  		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>  	},
>  	{
>  		.cmd		= NET_SHAPER_CMD_DELETE,
> -		.pre_doit	= net_shaper_nl_pre_doit,
> +		.pre_doit	= net_shaper_nl_pre_doit_write,
>  		.doit		= net_shaper_nl_delete_doit,
> -		.post_doit	= net_shaper_nl_post_doit,
> +		.post_doit	= net_shaper_nl_post_doit_write,
>  		.policy		= net_shaper_delete_nl_policy,
>  		.maxattr	= NET_SHAPER_A_IFINDEX,
>  		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>  	},
>  	{
>  		.cmd		= NET_SHAPER_CMD_GROUP,
> -		.pre_doit	= net_shaper_nl_pre_doit,
> +		.pre_doit	= net_shaper_nl_pre_doit_write,
>  		.doit		= net_shaper_nl_group_doit,
> -		.post_doit	= net_shaper_nl_post_doit,
> +		.post_doit	= net_shaper_nl_post_doit_write,
>  		.policy		= net_shaper_group_nl_policy,
>  		.maxattr	= NET_SHAPER_A_LEAVES,
>  		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> --
> 2.53.0
> 
>

Re: [PATCH net 1/2] net: shaper: protect late read accesses to the hierarchy

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 17 Mar 2026 09:10:13 -0700 you wrote:
> We look up a netdev during prep of Netlink ops (pre- callbacks)
> and take a ref to it. Then later in the body of the callback
> we take its lock or RCU which are the actual protections.
> 
> This is not proper, a conversion from a ref to a locked netdev
> must include a liveness check (a check if the netdev hasn't been
> unregistered already). Fix the read cases (those under RCU).
> Writes needs a separate change to protect from creating the
> hierarchy after flush has already run.
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: shaper: protect late read accesses to the hierarchy
    https://git.kernel.org/netdev/net/c/0f9ea7141f36
  - [net,2/2] net: shaper: protect from late creation of hierarchy
    https://git.kernel.org/netdev/net/c/d75ec7e8ba19

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html