[PATCH v7] net/ncsi: Fix Use-After-Free in NCSI teardown using synchronize_rcu

[PATCH v7] net/ncsi: Fix Use-After-Free in NCSI teardown using synchronize_rcu

[Wong Boon Jhee]

NCSI device teardown can free packages and channels while RCU readers,
including Netlink handlers, are still using them. Unlink the device from
the global list and wait for RCU readers before freeing its state.

Protect Netlink lookups and VLAN callbacks against concurrent teardown.
Synchronously stop channel-monitor timers before freeing their channels,
including when a timer callback has already cleared the enabled flag.
Remove VLAN entries with RCU-safe list deletion and deferred freeing.

Fixes: 2d283bdd079c ("net/ncsi: Resource management")

Signed-off-by: Wong Boon Jhee <wongboonjhee52@gmail.com>
---
v6 -> v7:
- Protect VLAN callbacks from concurrent device teardown and defer VLAN
  entry freeing until RCU readers have completed.
- Synchronize channel-monitor timers even if the callback already cleared
  the enabled flag.
- Use borrowed socket network namespaces in Netlink handlers; remove
  unrelated VLAN and request cleanup from teardown.

 net/ncsi/internal.h     |  1 +
 net/ncsi/ncsi-manage.c  | 96 +++++++++++++++++++++++++++++++----------
 net/ncsi/ncsi-netlink.c | 91 ++++++++++++++++++++++++++++----------
 3 files changed, 142 insertions(+), 46 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index adee6dcabdc3..d9f0eadc7a24 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -310,6 +310,7 @@ enum {
 
 struct vlan_vid {
 	struct list_head list;
+	struct rcu_head rcu;
 	__be16 proto;
 	u16 vid;
 };
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 54d0df0a9efe..c3d3a448a07c 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -10,6 +10,7 @@
 #include <linux/skbuff.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/mutex.h>
 
 #include <net/ncsi.h>
 #include <net/net_namespace.h>
@@ -24,6 +25,7 @@
 
 LIST_HEAD(ncsi_dev_list);
 DEFINE_SPINLOCK(ncsi_dev_lock);
+static DEFINE_MUTEX(ncsi_dev_mutex);
 
 bool ncsi_channel_has_link(struct ncsi_channel *channel)
 {
@@ -182,13 +184,10 @@ void ncsi_stop_channel_monitor(struct ncsi_channel *nc)
 	unsigned long flags;
 
 	spin_lock_irqsave(&nc->lock, flags);
-	if (!nc->monitor.enabled) {
-		spin_unlock_irqrestore(&nc->lock, flags);
-		return;
-	}
 	nc->monitor.enabled = false;
 	spin_unlock_irqrestore(&nc->lock, flags);
 
+	/* The callback may have cleared enabled and still be running. */
 	timer_delete_sync(&nc->monitor.timer);
 }
 
@@ -1669,37 +1668,49 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
 	struct vlan_vid *vlan;
 	struct ncsi_dev *nd;
 	bool found = false;
+	int ret;
 
 	if (vid == 0)
 		return 0;
 
+	mutex_lock(&ncsi_dev_mutex);
+	rcu_read_lock();
 	nd = ncsi_find_dev(dev);
+	rcu_read_unlock();
 	if (!nd) {
 		netdev_warn(dev, "NCSI: No net_device?\n");
-		return 0;
+		ret = 0;
+		goto out_unlock;
 	}
 
 	ndp = TO_NCSI_DEV_PRIV(nd);
 
 	/* Add the VLAN id to our internal list */
+	rcu_read_lock();
 	list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
 		n_vids++;
 		if (vlan->vid == vid) {
 			netdev_dbg(dev, "NCSI: vid %u already registered\n",
 				   vid);
-			return 0;
+			rcu_read_unlock();
+			ret = 0;
+			goto out_unlock;
 		}
 	}
+	rcu_read_unlock();
 	if (n_vids >= NCSI_MAX_VLAN_VIDS) {
 		netdev_warn(dev,
 			    "tried to add vlan id %u but NCSI max already registered (%u)\n",
 			    vid, NCSI_MAX_VLAN_VIDS);
-		return -ENOSPC;
+		ret = -ENOSPC;
+		goto out_unlock;
 	}
 
 	vlan = kzalloc_obj(*vlan);
-	if (!vlan)
-		return -ENOMEM;
+	if (!vlan) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
 
 	vlan->proto = proto;
 	vlan->vid = vid;
@@ -1709,45 +1720,60 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
 
 	found = ncsi_kick_channels(ndp) != 0;
 
-	return found ? ncsi_process_next_channel(ndp) : 0;
+	ret = found ? ncsi_process_next_channel(ndp) : 0;
+out_unlock:
+	mutex_unlock(&ncsi_dev_mutex);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ncsi_vlan_rx_add_vid);
 
 int ncsi_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
 {
-	struct vlan_vid *vlan, *tmp;
+	struct vlan_vid *vlan;
 	struct ncsi_dev_priv *ndp;
 	struct ncsi_dev *nd;
 	bool found = false;
+	int ret;
 
 	if (vid == 0)
 		return 0;
 
+	mutex_lock(&ncsi_dev_mutex);
+	rcu_read_lock();
 	nd = ncsi_find_dev(dev);
+	rcu_read_unlock();
 	if (!nd) {
 		netdev_warn(dev, "NCSI: no net_device?\n");
-		return 0;
+		ret = 0;
+		goto out_unlock;
 	}
 
 	ndp = TO_NCSI_DEV_PRIV(nd);
 
 	/* Remove the VLAN id from our internal list */
-	list_for_each_entry_safe(vlan, tmp, &ndp->vlan_vids, list)
+	rcu_read_lock();
+	list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list)
 		if (vlan->vid == vid) {
 			netdev_dbg(dev, "NCSI: vid %u found, removing\n", vid);
 			list_del_rcu(&vlan->list);
 			found = true;
-			kfree(vlan);
+			kfree_rcu(vlan, rcu);
+			break;
 		}
+	rcu_read_unlock();
 
 	if (!found) {
 		netdev_err(dev, "NCSI: vid %u wasn't registered!\n", vid);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_unlock;
 	}
 
 	found = ncsi_kick_channels(ndp) != 0;
 
-	return found ? ncsi_process_next_channel(ndp) : 0;
+	ret = found ? ncsi_process_next_channel(ndp) : 0;
+out_unlock:
+	mutex_unlock(&ncsi_dev_mutex);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ncsi_vlan_rx_kill_vid);
 
@@ -1762,14 +1788,21 @@ struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
 	int i;
 
 	/* Check if the device has been registered or not */
+	mutex_lock(&ncsi_dev_mutex);
+	rcu_read_lock();
 	nd = ncsi_find_dev(dev);
-	if (nd)
+	rcu_read_unlock();
+	if (nd) {
+		mutex_unlock(&ncsi_dev_mutex);
 		return nd;
+	}
 
 	/* Create NCSI device */
 	ndp = kzalloc_obj(*ndp, GFP_ATOMIC);
-	if (!ndp)
+	if (!ndp) {
+		mutex_unlock(&ncsi_dev_mutex);
 		return NULL;
+	}
 
 	nd = &ndp->ndev;
 	nd->state = ncsi_dev_state_registered;
@@ -1810,6 +1843,7 @@ struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
 			ndp->mlx_multi_host = true;
 	}
 
+	mutex_unlock(&ncsi_dev_mutex);
 	return nd;
 }
 EXPORT_SYMBOL_GPL(ncsi_register_dev);
@@ -1956,19 +1990,35 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
 {
 	struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
 	struct ncsi_package *np, *tmp;
+	struct ncsi_channel *nc;
 	unsigned long flags;
+	int i;
 
-	dev_remove_pack(&ndp->ptype);
-
-	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
-		ncsi_remove_package(np);
-
+	mutex_lock(&ncsi_dev_mutex);
 	spin_lock_irqsave(&ncsi_dev_lock, flags);
 	list_del_rcu(&ndp->node);
 	spin_unlock_irqrestore(&ncsi_dev_lock, flags);
+	mutex_unlock(&ncsi_dev_mutex);
 
+	dev_remove_pack(&ndp->ptype);
+
+	/* Wait for readers that found this device before it was unlinked. */
+	synchronize_rcu();
 	disable_work_sync(&ndp->work);
 
+	/* Stop all request producers before draining request state. */
+	list_for_each_entry(np, &ndp->packages, node) {
+		list_for_each_entry(nc, &np->channels, node)
+			ncsi_stop_channel_monitor(nc);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
+		timer_delete_sync(&ndp->requests[i].timer);
+	}
+
+	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
+		ncsi_remove_package(np);
+
 	kfree(ndp);
 }
 EXPORT_SYMBOL_GPL(ncsi_unregister_dev);
diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
index 8cc538358f6a..202d4547d31d 100644
--- a/net/ncsi/ncsi-netlink.c
+++ b/net/ncsi/ncsi-netlink.c
@@ -169,19 +169,24 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
 	if (!info->attrs[NCSI_ATTR_PACKAGE_ID])
 		return -EINVAL;
 
-	ndp = ndp_from_ifindex(genl_info_net(info),
-			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
-	if (!ndp)
-		return -ENODEV;
-
 	skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!skb)
 		return -ENOMEM;
 
+	rcu_read_lock();
+	ndp = ndp_from_ifindex(genl_info_net(info),
+			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
+	if (!ndp) {
+		rcu_read_unlock();
+		kfree_skb(skb);
+		return -ENODEV;
+	}
+
 	hdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
 			  &ncsi_genl_family, 0, NCSI_CMD_PKG_INFO);
 	if (!hdr) {
 		kfree_skb(skb);
+		rcu_read_unlock();
 		return -EMSGSIZE;
 	}
 
@@ -190,6 +195,7 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
 	attr = nla_nest_start_noflag(skb, NCSI_ATTR_PACKAGE_LIST);
 	if (!attr) {
 		kfree_skb(skb);
+		rcu_read_unlock();
 		return -EMSGSIZE;
 	}
 	rc = ncsi_write_package_info(skb, ndp, package_id);
@@ -202,10 +208,12 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
 	nla_nest_end(skb, attr);
 
 	genlmsg_end(skb, hdr);
+	rcu_read_unlock();
 	return genlmsg_reply(skb, info);
 
 err:
 	kfree_skb(skb);
+	rcu_read_unlock();
 	return rc;
 }
 
@@ -228,11 +236,14 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
 	if (!attrs[NCSI_ATTR_IFINDEX])
 		return -EINVAL;
 
-	ndp = ndp_from_ifindex(get_net(sock_net(skb->sk)),
+	rcu_read_lock();
+	ndp = ndp_from_ifindex(sock_net(skb->sk),
 			       nla_get_u32(attrs[NCSI_ATTR_IFINDEX]));
 
-	if (!ndp)
+	if (!ndp) {
+		rcu_read_unlock();
 		return -ENODEV;
+	}
 
 	package_id = cb->args[0];
 	package = NULL;
@@ -240,20 +251,23 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
 		if (np->id == package_id)
 			package = np;
 
-	if (!package)
+	if (!package) {
+		rcu_read_unlock();
 		return 0; /* done */
+	}
 
 	hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			  &ncsi_genl_family, NLM_F_MULTI,  NCSI_CMD_PKG_INFO);
 	if (!hdr) {
-		rc = -EMSGSIZE;
-		goto err;
+		rcu_read_unlock();
+		return -EMSGSIZE;
 	}
 
 	attr = nla_nest_start_noflag(skb, NCSI_ATTR_PACKAGE_LIST);
 	if (!attr) {
-		rc = -EMSGSIZE;
-		goto err;
+		genlmsg_cancel(skb, hdr);
+		rcu_read_unlock();
+		return -EMSGSIZE;
 	}
 	rc = ncsi_write_package_info(skb, ndp, package->id);
 	if (rc) {
@@ -266,9 +280,12 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
 
 	cb->args[0] = package_id + 1;
 
-	return skb->len;
+	rc = skb->len;
+	rcu_read_unlock();
+	return rc;
 err:
 	genlmsg_cancel(skb, hdr);
+	rcu_read_unlock();
 	return rc;
 }
 
@@ -289,10 +306,13 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
 	if (!info->attrs[NCSI_ATTR_PACKAGE_ID])
 		return -EINVAL;
 
-	ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+	rcu_read_lock();
+	ndp = ndp_from_ifindex(sock_net(msg->sk),
 			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
-	if (!ndp)
+	if (!ndp) {
+		rcu_read_unlock();
 		return -ENODEV;
+	}
 
 	package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
 	package = NULL;
@@ -302,6 +322,7 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
 			package = np;
 	if (!package) {
 		/* The user has set a package that does not exist */
+		rcu_read_unlock();
 		return -ERANGE;
 	}
 
@@ -317,6 +338,7 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
 			netdev_info(ndp->ndev.dev,
 				    "NCSI: Channel %u does not exist!\n",
 				    channel_id);
+			rcu_read_unlock();
 			return -ERANGE;
 		}
 	}
@@ -350,6 +372,7 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
 	if (!(ndp->flags & NCSI_DEV_RESET))
 		ncsi_reset_dev(&ndp->ndev);
 
+	rcu_read_unlock();
 	return 0;
 }
 
@@ -365,10 +388,13 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
 	if (!info->attrs[NCSI_ATTR_IFINDEX])
 		return -EINVAL;
 
-	ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+	rcu_read_lock();
+	ndp = ndp_from_ifindex(sock_net(msg->sk),
 			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
-	if (!ndp)
+	if (!ndp) {
+		rcu_read_unlock();
 		return -ENODEV;
+	}
 
 	/* Reset any whitelists and disable multi mode */
 	spin_lock_irqsave(&ndp->lock, flags);
@@ -389,6 +415,7 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
 	if (!(ndp->flags & NCSI_DEV_RESET))
 		ncsi_reset_dev(&ndp->ndev);
 
+	rcu_read_unlock();
 	return 0;
 }
 
@@ -398,6 +425,7 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
 	struct ncsi_pkt_hdr *hdr;
 	struct ncsi_cmd_arg nca;
 	unsigned char *data;
+	bool rcu_locked = false;
 	u32 package_id;
 	u32 channel_id;
 	int len, ret;
@@ -427,10 +455,14 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
 		goto out;
 	}
 
-	ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+	rcu_read_lock();
+	rcu_locked = true;
+	ndp = ndp_from_ifindex(sock_net(msg->sk),
 			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
 	if (!ndp) {
 		ret = -ENODEV;
+		rcu_read_unlock();
+		rcu_locked = false;
 		goto out;
 	}
 
@@ -480,6 +512,8 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
 				      ret);
 	}
 out:
+	if (rcu_locked)
+		rcu_read_unlock();
 	return ret;
 }
 
@@ -608,10 +642,13 @@ static int ncsi_set_package_mask_nl(struct sk_buff *msg,
 	if (!info->attrs[NCSI_ATTR_PACKAGE_MASK])
 		return -EINVAL;
 
-	ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+	rcu_read_lock();
+	ndp = ndp_from_ifindex(sock_net(msg->sk),
 			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
-	if (!ndp)
+	if (!ndp) {
+		rcu_read_unlock();
 		return -ENODEV;
+	}
 
 	spin_lock_irqsave(&ndp->lock, flags);
 	if (nla_get_flag(info->attrs[NCSI_ATTR_MULTI_FLAG])) {
@@ -639,6 +676,7 @@ static int ncsi_set_package_mask_nl(struct sk_buff *msg,
 			ncsi_reset_dev(&ndp->ndev);
 	}
 
+	rcu_read_unlock();
 	return rc;
 }
 
@@ -663,10 +701,13 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
 	if (!info->attrs[NCSI_ATTR_CHANNEL_MASK])
 		return -EINVAL;
 
-	ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+	rcu_read_lock();
+	ndp = ndp_from_ifindex(sock_net(msg->sk),
 			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
-	if (!ndp)
+	if (!ndp) {
+		rcu_read_unlock();
 		return -ENODEV;
+	}
 
 	package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
 	package = NULL;
@@ -675,8 +716,10 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
 			package = np;
 			break;
 		}
-	if (!package)
+	if (!package) {
+		rcu_read_unlock();
 		return -ERANGE;
+	}
 
 	spin_lock_irqsave(&package->lock, flags);
 
@@ -690,6 +733,7 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
 			}
 		if (!channel) {
 			spin_unlock_irqrestore(&package->lock, flags);
+			rcu_read_unlock();
 			return -ERANGE;
 		}
 		netdev_dbg(ndp->ndev.dev,
@@ -721,6 +765,7 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
 	if (!(ndp->flags & NCSI_DEV_RESET))
 		ncsi_reset_dev(&ndp->ndev);
 
+	rcu_read_unlock();
 	return 0;
 }
 

base-commit: a7bfaba4823e3c165bb2004c74eff7c096672bc7
-- 
2.55.0

Re: [PATCH v7] net/ncsi: Fix Use-After-Free in NCSI teardown using synchronize_rcu

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 (4) · Low: 2 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] A deterministic use-after-free in NCSI teardown survives this
  patch.
- [Medium] In ncsi_unregister_dev(), the new comment says "Stop all
  request producers before draining request state." The code after it…
- [Low] The patch fixes a second, unrelated bug, a struct net refcount
  leak in the NCSI generic-netlink handlers, and the commit message body…
- [Low] The commit message mischaracterizes the defect and leaves out
  several behaviour changes.

Pre-existing issues:
- [Medium] ncsi_unregister_dev() does not free the struct vlan_vid
  entries on ndp->vlan_vids before kfree(ndp).
- [Medium] The concern says that making timer_delete_sync() unconditional
  in ncsi_stop_channel_monitor() introduces a double list_add of…
- [Medium] The package-info netlink operations assume package IDs are
  contiguous from 0.
- [Medium] set_one_vid() declares `u16 vid;` without initializing it and
  checks list_empty(&ndp->vlan_vids) without synchronization before…
- [Low] In ncsi_vlan_rx_add_vid()/ncsi_vlan_rx_kill_vid() the new
  rcu_read_lock() covers only the ncsi_find_dev() lookup and the…

--

net/ncsi: Fix Use-After-Free in NCSI teardown using synchronize_rcu

This patch reorders ncsi_unregister_dev() so the device is unlinked from
ncsi_dev_list and synchronize_rcu() runs before packages and channels are
freed. It also stops the channel monitor and request timers
synchronously. A global ncsi_dev_mutex now covers registration,
unregistration and the VLAN callbacks. The NCSI netlink handlers now take
rcu_read_lock() around their lookups, and VLAN entries are freed with
kfree_rcu().

> NCSI device teardown can free packages and channels while RCU readers,
> including Netlink handlers, are still using them. Unlink the device from
> the global list and wait for RCU readers before freeing its state.

[Severity: Low]
Is this an accurate description of the bug? Before this patch, none of
the NCSI netlink handlers took rcu_read_lock().

ndp_from_ifindex() called dev_get_by_index(), ncsi_find_dev() and
dev_put(), then returned a bare ndp pointer with no protection. The
handlers were never RCU readers outliving a grace period. They had no
reader-side protection at all, and this patch is what adds it.

The message also leaves out several behaviour changes:

- ncsi_register_dev() now takes a new global ncsi_dev_mutex. It still
  allocates with GFP_ATOMIC but can now sleep on the mutex. Its only
  in-tree caller, ftgmac100_probe(), can sleep, so this is not a
  functional problem.

- ncsi_stop_channel_monitor() no longer returns early when
  !monitor.enabled. That affects every caller, including
  ncsi_aen_handler_lsc() and ncsi_aen_handler_cr() in the RX softirq
  path, not only teardown.

- ncsi_unregister_dev() now deletes all 256 request timers
  synchronously.

- ncsi_vlan_rx_kill_vid() now stops after the first match.

- The netlink error paths are reworked. ncsi_pkg_info_nl() now
  allocates before the lookup, and ncsi_pkg_info_all_nl() now calls
  genlmsg_cancel() when the nest start fails.

Could the changelog describe these changes?

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 54d0df0a9efe6..c3d3a448a07cb 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c

[ ... ]

> @@ -182,13 +184,10 @@ void ncsi_stop_channel_monitor(struct ncsi_channel *nc)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&nc->lock, flags);
> -	if (!nc->monitor.enabled) {
> -		spin_unlock_irqrestore(&nc->lock, flags);
> -		return;
> -	}
>  	nc->monitor.enabled = false;
>  	spin_unlock_irqrestore(&nc->lock, flags);
>  
> +	/* The callback may have cleared enabled and still be running. */
>  	timer_delete_sync(&nc->monitor.timer);
>  }

[Severity: Medium]
This is a pre-existing issue, but the AEN handlers still race with the
monitor timeout path: they check the channel state, then act on it
later. ncsi_aen_handler_lsc() follows the same pattern as this one:

net/ncsi/ncsi-aen.c:ncsi_aen_handler_cr() {
    ...
	if (!list_empty(&nc->link) ||
	    nc->state != NCSI_CHANNEL_ACTIVE) {
		spin_unlock_irqrestore(&nc->lock, flags);
		return 0;
	}
	spin_unlock_irqrestore(&nc->lock, flags);

	ncsi_stop_channel_monitor(nc);
    ...
	list_add_tail_rcu(&nc->link, &ndp->channel_queue);
    ...
}

If ncsi_channel_monitor() on another CPU takes its timeout branch at
the same time, it also queues nc->link on ndp->channel_queue. Neither
handler rechecks nc->link or nc->state after ncsi_stop_channel_monitor()
returns.

With timer_delete_sync() now unconditional, the handler waits for the
callback to finish. By then ncsi_process_next_channel() has usually
removed the entry with list_del_init(), so this patch narrows the
window rather than creating it.

Can nc->link still be added twice, and corrupt ndp->channel_queue,
when nc sits behind another entry in the queue?

[ ... ]

>  	/* Add the VLAN id to our internal list */
> +	rcu_read_lock();
>  	list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
>  		n_vids++;
>  		if (vlan->vid == vid) {
>  			netdev_dbg(dev, "NCSI: vid %u already registered\n",
>  				   vid);
> -			return 0;
> +			rcu_read_unlock();
> +			ret = 0;
> +			goto out_unlock;
>  		}
>  	}
> +	rcu_read_unlock();

[ ... ]

> @@ -1709,45 +1720,60 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
>  
>  	found = ncsi_kick_channels(ndp) != 0;
>  
> -	return found ? ncsi_process_next_channel(ndp) : 0;
> +	ret = found ? ncsi_process_next_channel(ndp) : 0;

[Severity: Low]
This is a pre-existing issue, but ncsi_kick_channels() runs here after
rcu_read_unlock(). It walks ndp->packages and np->channels with
NCSI_FOR_EACH_PACKAGE() and NCSI_FOR_EACH_CHANNEL(). Both expand to
list_for_each_entry_rcu() with no lockdep condition.

Could this trigger the "RCU-list traversed in non-reader section"
warning with CONFIG_PROVE_RCU_LIST? The objects themselves look safe:
ncsi_dev_mutex is held for the whole callback, and ncsi_unregister_dev()
needs that mutex to unlink the device. So only the lockdep annotation
is wrong. ncsi_vlan_rx_kill_vid() has the same pattern.

> +out_unlock:
> +	mutex_unlock(&ncsi_dev_mutex);
> +	return ret;
>  }

[ ... ]

>  	/* Remove the VLAN id from our internal list */
> -	list_for_each_entry_safe(vlan, tmp, &ndp->vlan_vids, list)
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list)
>  		if (vlan->vid == vid) {
>  			netdev_dbg(dev, "NCSI: vid %u found, removing\n", vid);
>  			list_del_rcu(&vlan->list);
>  			found = true;
> -			kfree(vlan);
> +			kfree_rcu(vlan, rcu);
> +			break;
>  		}
> +	rcu_read_unlock();

[Severity: Medium]
This isn't a bug introduced by this patch. kfree_rcu() fixes the entry
lifetime, but set_one_vid() still checks for an empty list with no
synchronization before walking it:

net/ncsi/ncsi-manage.c:set_one_vid() {
    ...
	u16 vid;

	if (list_empty(&ndp->vlan_vids))
		return -1;
    ...
	list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
		vid = vlan->vid;
    ...
	if (!vid) {
    ...
}

ncsi_vlan_rx_kill_vid() runs under RTNL and ncsi_dev_mutex, but nothing
serializes it against the NCSI work and configuration path.

If this removes the last entry between the list_empty() check and the
walk, the loop body never runs. Would vid then be read uninitialized,
so that a garbage VLAN ID is written into ncf->vids[] and sent in an
SVF command?

[ ... ]

> @@ -1956,19 +1990,35 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
>  {
>  	struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
>  	struct ncsi_package *np, *tmp;
> +	struct ncsi_channel *nc;
>  	unsigned long flags;
> +	int i;
>  
> -	dev_remove_pack(&ndp->ptype);
> -
> -	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
> -		ncsi_remove_package(np);
> -
> +	mutex_lock(&ncsi_dev_mutex);
>  	spin_lock_irqsave(&ncsi_dev_lock, flags);
>  	list_del_rcu(&ndp->node);
>  	spin_unlock_irqrestore(&ncsi_dev_lock, flags);
> +	mutex_unlock(&ncsi_dev_mutex);
>  
> +	dev_remove_pack(&ndp->ptype);
> +
> +	/* Wait for readers that found this device before it was unlinked. */
> +	synchronize_rcu();
>  	disable_work_sync(&ndp->work);
>  
> +	/* Stop all request producers before draining request state. */
> +	list_for_each_entry(np, &ndp->packages, node) {
> +		list_for_each_entry(nc, &np->channels, node)
> +			ncsi_stop_channel_monitor(nc);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
> +		timer_delete_sync(&ndp->requests[i].timer);
> +	}

[Severity: Medium]
The comment above says request state is drained, but is anything
actually drained here? This loop only cancels the request timers.

A request can still be in flight: used, with nr->cmd set and no
response yet. The only path that releases it is ncsi_request_timeout()
calling ncsi_free_request():

net/ncsi/ncsi-manage.c:ncsi_free_request() {
    ...
	cmd = nr->cmd;
	rsp = nr->rsp;
    ...
	consume_skb(cmd);
	consume_skb(rsp);
}

ncsi_xmit_cmd() takes its own reference with skb_get(nr->cmd), and
only ncsi_free_request() drops it. After dev_remove_pack() no response
can arrive, and timer_delete_sync() stops the timeout from running.
kfree(ndp) then frees the requests[] table, which held the only pointer
to the skb.

Does this leak the command skb, and any stored response, for every
outstanding request? The channel monitor sends a GLS command every
second, so a request is often in flight at removal. A netlink-driven
request would also never send its timeout reply to user space.

Before this patch the pending timer fired after kfree(ndp), so the
change turns a use-after-free into a leak. Would it work to call
ncsi_free_request() for each requests[i] that is still used? The work
is already disabled here, and ncsi_free_request() already calls
timer_delete_sync() when nr->enabled is set.

The v6 -> v7 notes say request cleanup was dropped as unrelated, so
the comment may be left over from an earlier version.

> +
> +	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
> +		ncsi_remove_package(np);
> +
>  	kfree(ndp);

[Severity: High]
Is the teardown use-after-free still reachable through the only
in-tree user? ftgmac100_remove() frees ndp before it closes the netdev,
and it never clears priv->ndev:

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_remove() {
    ...
	if (priv->ndev)
		ncsi_unregister_dev(priv->ndev);
	unregister_netdev(netdev);
    ...
}

If the interface is up, unregister_netdev() closes it and runs
ftgmac100_stop(), which does:

	if (priv->use_ncsi)
		ncsi_stop_dev(priv->ndev);

That gives this call chain on freed memory:

ftgmac100_remove()
  ncsi_unregister_dev()
    kfree(ndp)
  unregister_netdev()
    ftgmac100_stop()
      ncsi_stop_dev(priv->ndev)
        NCSI_FOR_EACH_PACKAGE(ndp, np) ...
        ncsi_report_link(ndp, true)
          nd->state = ncsi_dev_state_functional;
          nd->link_up = 0;
          nd->handler(nd);

ncsi_report_link() writes into the freed ncsi_dev_priv. It then makes
an indirect call through nd->handler, which is loaded from freed
memory.

The new sequence (ncsi_dev_mutex, list_del_rcu(), synchronize_rcu(),
disable_work_sync(), timer deletion) only fences users that find the
device through ncsi_find_dev() or ndp_from_ifindex().
ncsi_start_dev(), ncsi_stop_dev() and ncsi_reset_dev() use the nd
pointer directly.

The patch protects VLAN callbacks that arrive in this same window during
unregister_netdev(), but it does not protect ndo_stop. The call order in
ftgmac100_remove() is older than this patch. Should it still be fixed
here, given that this patch is meant to close NCSI teardown
use-after-frees?

[Severity: Medium]
This is a pre-existing issue, but does kfree(ndp) leak the struct
vlan_vid entries that are still on ndp->vlan_vids?

ftgmac100_remove() calls ncsi_unregister_dev() before
unregister_netdev(). As a result, the ndo_vlan_rx_kill_vid calls made
while unregister_netdev() tears down VLAN children cannot find the
device:

	nd = ncsi_find_dev(dev);
	rcu_read_unlock();
	if (!nd) {
		netdev_warn(dev, "NCSI: no net_device?\n");
		ret = 0;
		goto out_unlock;
	}

So none of the entries added through ncsi_vlan_rx_add_vid() are ever
freed. The v6 -> v7 notes say VLAN cleanup was left out of this patch
as unrelated.

>  }
>  EXPORT_SYMBOL_GPL(ncsi_unregister_dev);
> diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
> index 8cc538358f6a3..202d4547d31d2 100644
> --- a/net/ncsi/ncsi-netlink.c
> +++ b/net/ncsi/ncsi-netlink.c

[ ... ]

> @@ -228,11 +236,14 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
>  	if (!attrs[NCSI_ATTR_IFINDEX])
>  		return -EINVAL;
>  
> -	ndp = ndp_from_ifindex(get_net(sock_net(skb->sk)),
> +	rcu_read_lock();
> +	ndp = ndp_from_ifindex(sock_net(skb->sk),
>  			       nla_get_u32(attrs[NCSI_ATTR_IFINDEX]));

[Severity: Low]
Dropping get_net() here also fixes a separate netns reference leak. The
same change is made in ncsi_set_interface_nl(),
ncsi_clear_interface_nl(), ncsi_send_cmd_nl(),
ncsi_set_package_mask_nl() and ncsi_set_channel_mask_nl().

ndp_from_ifindex() never calls put_net(), and there is no put_net()
anywhere in net/ncsi/. Each of these requests therefore leaked one
struct net reference.

The only hint of this fix is in the v6 -> v7 notes ("Use borrowed
socket network namespaces"), and git am drops those. Could it go in
its own patch, with a Fixes: tag for the commit that added the NCSI
netlink family? Otherwise, could the commit message at least describe
it? The current Fixes: tag (2d283bdd079c) points at a different root
cause.

>  
> -	if (!ndp)
> +	if (!ndp) {
> +		rcu_read_unlock();
>  		return -ENODEV;
> +	}
>  
>  	package_id = cb->args[0];
>  	package = NULL;
> @@ -240,20 +251,23 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
>  		if (np->id == package_id)
>  			package = np;
>  
> -	if (!package)
> +	if (!package) {
> +		rcu_read_unlock();
>  		return 0; /* done */
> +	}

[Severity: Medium]
This isn't a bug introduced by this patch, but this dump assumes package
IDs are contiguous from 0. cb->args[0] starts at 0, and the dump ends
as soon as no package has that exact ID.

ncsi_write_package_info() makes the same assumption:

	if (id > ndp->package_num - 1) {
		netdev_info(ndp->ndev.dev, "NCSI: No package with id %u\n", id);
		return -ENODEV;
	}

ncsi_add_package() keeps the package ID from the SP response and counts
package_num separately. Suppose the only package found has ID 1. Would
the dump then come back empty, and NCSI_CMD_PKG_INFO for package 1
return -ENODEV?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260926133201.689877-1-wongboonjhee52%40gmail.com