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

[PATCH v8] 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>
---
v7 -> v8:
- Stop channel monitors and requests synchronously, free outstanding requests and VLAN entries, and keep ftgmac100 teardown ordered around unregister_netdev().
- Recheck channel state and queue membership after stopping the monitor; serialize queue dequeue/state changes to avoid duplicate insertion.
- Protect VLAN channel traversal with RCU read-side sections; handle non-contiguous package IDs and initialize the VLAN selection value.
- Fix Netlink error paths and use borrowed socket network namespaces.

 drivers/net/ethernet/faraday/ftgmac100.c |  10 +-
 net/ncsi/internal.h                      |   1 +
 net/ncsi/ncsi-aen.c                      |  21 ++-
 net/ncsi/ncsi-manage.c                   | 162 +++++++++++++++++------
 net/ncsi/ncsi-netlink.c                  | 101 +++++++++-----
 5 files changed, 218 insertions(+), 77 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 6d2fe5c2f390..193f5147ef7a 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -2094,8 +2094,10 @@ static int ftgmac100_probe(struct platform_device *pdev)
 
 err:
 	ftgmac100_phy_disconnect(netdev);
-	if (priv->ndev)
+	if (priv->ndev) {
 		ncsi_unregister_dev(priv->ndev);
+		priv->ndev = NULL;
+	}
 	return err;
 }
 
@@ -2107,9 +2109,11 @@ static void ftgmac100_remove(struct platform_device *pdev)
 	netdev = platform_get_drvdata(pdev);
 	priv = netdev_priv(netdev);
 
-	if (priv->ndev)
-		ncsi_unregister_dev(priv->ndev);
 	unregister_netdev(netdev);
+	if (priv->ndev) {
+		ncsi_unregister_dev(priv->ndev);
+		priv->ndev = NULL;
+	}
 
 	/* There's a small chance the reset task will have been re-queued,
 	 * during stop, make sure it's gone before we free the structure.
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-aen.c b/net/ncsi/ncsi-aen.c
index 040a31557201..c4025e7f2cf2 100644
--- a/net/ncsi/ncsi-aen.c
+++ b/net/ncsi/ncsi-aen.c
@@ -93,8 +93,16 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
 		if (had_link) {
 			ndp->flags |= NCSI_DEV_RESHUFFLE;
 			ncsi_stop_channel_monitor(nc);
+			spin_lock_irqsave(&nc->lock, flags);
+			if (nc->state != NCSI_CHANNEL_ACTIVE) {
+				spin_unlock_irqrestore(&nc->lock, flags);
+				return 0;
+			}
+			nc->state = NCSI_CHANNEL_INACTIVE;
+			spin_unlock_irqrestore(&nc->lock, flags);
 			spin_lock_irqsave(&ndp->lock, flags);
-			list_add_tail_rcu(&nc->link, &ndp->channel_queue);
+			if (list_empty(&nc->link))
+				list_add_tail_rcu(&nc->link, &ndp->channel_queue);
 			spin_unlock_irqrestore(&ndp->lock, flags);
 			return ncsi_process_next_channel(ndp);
 		}
@@ -158,12 +166,15 @@ static int ncsi_aen_handler_cr(struct ncsi_dev_priv *ndp,
 
 	ncsi_stop_channel_monitor(nc);
 	spin_lock_irqsave(&nc->lock, flags);
-	nc->state = NCSI_CHANNEL_INVISIBLE;
+	if (nc->state != NCSI_CHANNEL_ACTIVE) {
+		spin_unlock_irqrestore(&nc->lock, flags);
+		return 0;
+	}
+	nc->state = NCSI_CHANNEL_INACTIVE;
 	spin_unlock_irqrestore(&nc->lock, flags);
-
 	spin_lock_irqsave(&ndp->lock, flags);
-	nc->state = NCSI_CHANNEL_INACTIVE;
-	list_add_tail_rcu(&nc->link, &ndp->channel_queue);
+	if (list_empty(&nc->link))
+		list_add_tail_rcu(&nc->link, &ndp->channel_queue);
 	spin_unlock_irqrestore(&ndp->lock, flags);
 	nc->modes[NCSI_MODE_TX_ENABLE].enable = 0;
 
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 54d0df0a9efe..266b1c4209ba 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)
 {
@@ -152,7 +154,8 @@ static void ncsi_channel_monitor(struct timer_list *t)
 
 		spin_lock_irqsave(&ndp->lock, flags);
 		nc->state = NCSI_CHANNEL_ACTIVE;
-		list_add_tail_rcu(&nc->link, &ndp->channel_queue);
+		if (list_empty(&nc->link))
+			list_add_tail_rcu(&nc->link, &ndp->channel_queue);
 		spin_unlock_irqrestore(&ndp->lock, flags);
 		ncsi_process_next_channel(ndp);
 		return;
@@ -182,13 +185,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);
 }
 
@@ -640,7 +640,7 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
 	unsigned long flags;
 	int i, index;
 	void *bitmap;
-	u16 vid;
+	u16 vid = 0;
 
 	if (list_empty(&ndp->vlan_vids))
 		return -1;
@@ -1567,13 +1567,12 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
 		goto out;
 	}
 
+	spin_lock(&nc->lock);
 	list_del_init(&nc->link);
-	spin_unlock_irqrestore(&ndp->lock, flags);
-
-	spin_lock_irqsave(&nc->lock, flags);
 	old_state = nc->state;
 	nc->state = NCSI_CHANNEL_INVISIBLE;
-	spin_unlock_irqrestore(&nc->lock, flags);
+	spin_unlock(&nc->lock);
+	spin_unlock_irqrestore(&ndp->lock, flags);
 
 	ndp->active_channel = nc;
 	ndp->active_package = nc->package;
@@ -1618,11 +1617,13 @@ static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
 	struct ncsi_channel *nc;
 	struct ncsi_package *np;
 	unsigned long flags;
+	bool kicked;
 	unsigned int n = 0;
 
 	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		NCSI_FOR_EACH_CHANNEL(np, nc) {
-			spin_lock_irqsave(&nc->lock, flags);
+			spin_lock_irqsave(&ndp->lock, flags);
+			spin_lock(&nc->lock);
 
 			/* Channels may be busy, mark dirty instead of
 			 * kicking if;
@@ -1639,23 +1640,30 @@ static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
 						   nc);
 					nc->reconfigure_needed = true;
 				}
-				spin_unlock_irqrestore(&nc->lock, flags);
+				spin_unlock(&nc->lock);
+				spin_unlock_irqrestore(&ndp->lock, flags);
 				continue;
 			}
 
-			spin_unlock_irqrestore(&nc->lock, flags);
+			spin_unlock(&nc->lock);
+			spin_unlock_irqrestore(&ndp->lock, flags);
 
 			ncsi_stop_channel_monitor(nc);
-			spin_lock_irqsave(&nc->lock, flags);
-			nc->state = NCSI_CHANNEL_INACTIVE;
-			spin_unlock_irqrestore(&nc->lock, flags);
-
 			spin_lock_irqsave(&ndp->lock, flags);
-			list_add_tail_rcu(&nc->link, &ndp->channel_queue);
+			spin_lock(&nc->lock);
+			kicked = false;
+			if (nc->state == NCSI_CHANNEL_ACTIVE &&
+			    list_empty(&nc->link)) {
+				nc->state = NCSI_CHANNEL_INACTIVE;
+				list_add_tail_rcu(&nc->link, &ndp->channel_queue);
+				kicked = true;
+				n++;
+			}
+			spin_unlock(&nc->lock);
 			spin_unlock_irqrestore(&ndp->lock, flags);
 
-			netdev_dbg(nd->dev, "NCSI: kicked channel %p\n", nc);
-			n++;
+			if (kicked)
+				netdev_dbg(nd->dev, "NCSI: kicked channel %p\n", nc);
 		}
 	}
 
@@ -1669,37 +1677,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;
@@ -1707,47 +1727,66 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
 
 	netdev_dbg(dev, "NCSI: Added new vid %u\n", vid);
 
+	rcu_read_lock();
 	found = ncsi_kick_channels(ndp) != 0;
+	rcu_read_unlock();
 
-	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;
 	}
 
+	rcu_read_lock();
 	found = ncsi_kick_channels(ndp) != 0;
+	rcu_read_unlock();
 
-	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 +1801,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 +1856,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 +2003,56 @@ 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;
+	struct vlan_vid *vlan, *vlan_tmp;
 	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 channel monitors before releasing 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++) {
+		struct ncsi_request *nr = &ndp->requests[i];
+
+		timer_delete_sync(&nr->timer);
+		if (!nr->used)
+			continue;
+
+		if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN && nr->cmd) {
+			struct ncsi_cmd_pkt *cmd = (struct ncsi_cmd_pkt *)
+				skb_network_header(nr->cmd);
+
+			ncsi_find_package_and_channel(ndp,
+						      cmd->cmd.common.channel,
+						      &np, &nc);
+			ncsi_send_netlink_timeout(nr, np, nc);
+		}
+		ncsi_free_request(nr);
+	}
+
+	list_for_each_entry_safe(vlan, vlan_tmp, &ndp->vlan_vids, list) {
+		list_del_rcu(&vlan->list);
+		kfree_rcu(vlan, rcu);
+	}
+
+	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..1d9c8a3ef877 100644
--- a/net/ncsi/ncsi-netlink.c
+++ b/net/ncsi/ncsi-netlink.c
@@ -100,11 +100,6 @@ static int ncsi_write_package_info(struct sk_buff *skb,
 	bool found;
 	int rc;
 
-	if (id > ndp->package_num - 1) {
-		netdev_info(ndp->ndev.dev, "NCSI: No package with id %u\n", id);
-		return -ENODEV;
-	}
-
 	found = false;
 	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		if (np->id != id)
@@ -169,19 +164,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 +190,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 +203,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,32 +231,39 @@ 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;
 	NCSI_FOR_EACH_PACKAGE(ndp, np)
-		if (np->id == package_id)
+		if (np->id >= package_id &&
+		    (!package || 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) {
@@ -264,11 +274,14 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
 	nla_nest_end(skb, attr);
 	genlmsg_end(skb, hdr);
 
-	cb->args[0] = package_id + 1;
+	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 +302,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 +318,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 +334,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 +368,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 +384,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 +411,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 +421,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 +451,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 +508,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 +638,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 +672,7 @@ static int ncsi_set_package_mask_nl(struct sk_buff *msg,
 			ncsi_reset_dev(&ndp->ndev);
 	}
 
+	rcu_read_unlock();
 	return rc;
 }
 
@@ -663,10 +697,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 +712,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 +729,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 +761,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;
 }
 
-- 
2.55.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 · Medium: 3 · Low: 1 (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] In ftgmac100_remove(), the patch moves unregister_netdev() ahead
  of ncsi_unregister_dev().
- [High] In ncsi_aen_handler_lsc() (net/ncsi/ncsi-aen.c), the single-
  channel/single-package link-down path now sets `nc->state =…
- [High] The patch introduces an ABBA lock order inversion between
  ndp->lock and nc->lock.
- [Medium] After ncsi_stop_channel_monitor(), ncsi_aen_handler_lsc() and
  ncsi_aen_handler_cr() recheck only `nc->state == NCSI_CHANNEL_ACTIVE`…
- [Medium] Because of the reorder in ftgmac100_remove(), the NCSI device
  is still on ncsi_dev_list while unregister_netdev() runs.
- [Medium] The commit message describes only RCU-safe teardown,
  protection of Netlink/VLAN lookups, and synchronous monitor-timer stop.
- [Low] The new request-cleanup loop in ncsi_unregister_dev() calls
  ncsi_find_package_and_channel() for each outstanding NETLINK_DRIVEN…

Pre-existing issues:
- [Low] ncsi_send_netlink_timeout() falls back to reading the package ID
  from `((struct ncsi_pkt_hdr *)nr->cmd->data)->channel` when np is NULL.

--

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

The patch unlinks the NCSI device from the global list and waits with
synchronize_rcu() before freeing packages and channels. It wraps the
Netlink lookups and VLAN callbacks in RCU/mutex protection and stops
channel monitors and request timers synchronously. It also reorders
ftgmac100_remove() so that unregister_netdev() runs before
ncsi_unregister_dev().

> 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.

[Severity: Medium]
The commit message only covers the RCU teardown. The patch also seems to
include several independent changes, and these are described only in the
v7 -> v8 notes below the cut line, which are dropped when the patch is
applied:

- Six Netlink handlers no longer call get_net(sock_net(...)). There was
  no matching put_net() in net/ncsi/, so this fixes a struct net
  reference leak on every command.
- list_empty() guards against duplicate list_add_tail_rcu() on
  ndp->channel_queue in ncsi_channel_monitor(), the AEN handlers and
  ncsi_kick_channels().
- AEN state handling changes in ncsi_aen_handler_lsc() and
  ncsi_aen_handler_cr().
- vid is now initialised in set_one_vid().
- User-visible Netlink changes: the id > package_num - 1 check is removed
  from ncsi_write_package_info(), the PKG_INFO dump cursor now continues
  past package ID gaps, and genlmsg_cancel() is no longer called with a
  NULL hdr.
- Outstanding ncsi_request skbs and the vlan_vids entries are now freed
  at unregister.
- A new global ncsi_dev_mutex.

The message also says "RCU readers, including Netlink handlers". Before
this patch the Netlink handlers did not take rcu_read_lock().

Could these be split out, or at least described in the commit message,
with their own Fixes: tags where appropriate?

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 6d2fe5c2f3903..193f5147ef7a6 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c

[ ... ]

> @@ -2107,9 +2109,11 @@ static void ftgmac100_remove(struct platform_device *pdev)
>  	netdev = platform_get_drvdata(pdev);
>  	priv = netdev_priv(netdev);
>
> -	if (priv->ndev)
> -		ncsi_unregister_dev(priv->ndev);
>  	unregister_netdev(netdev);
> +	if (priv->ndev) {
> +		ncsi_unregister_dev(priv->ndev);
> +		priv->ndev = NULL;
> +	}

[Severity: High]
Can this reorder trigger the BUG_ON in netdev_run_todo()?

ncsi_register_dev() binds the NCSI packet handler to this netdev:

    ndp->ptype.dev = dev;
    dev_add_pack(&ndp->ptype);

Because pt->dev is set, ptype_head() puts the handler on
dev->ptype_specific. The only removal is the dev_remove_pack() in
ncsi_unregister_dev(), and that now runs after unregister_netdev() has
returned.

unregister_netdev() -> rtnl_net_dev_unlock() -> rtnl_unlock() ->
netdev_run_todo() then checks:

    BUG_ON(!list_empty(&dev->ptype_specific));

dev_add_pack() takes no netdev reference, so netdev_wait_allrefs_any()
completes. The BUG_ON would then fire every time an NCSI-enabled
ftgmac100 is removed, whether by unbind or rmmod.

[Severity: Medium]
The NCSI device is now still on ncsi_dev_list while unregister_netdev()
runs. Does ncsi_vlan_rx_kill_vid() then restart the channel state machine
during teardown?

unregister_netdevice() first calls dev_close() -> ftgmac100_stop() ->
ncsi_stop_dev(). That function deliberately leaves the channel states
ACTIVE:

    /* Stop the channel monitor on any active channels. Don't reset the
     * channel state so we know which were active when ncsi_start_dev()
     * is next called.
     */

NETDEV_UNREGISTER handling in 8021q then removes the VLAN uppers and calls
vlan_kill_rx_filter_info() -> ncsi_vlan_rx_kill_vid(). That path checks
netif_device_present() but not whether the interface is up.
ncsi_find_dev() still succeeds, so ncsi_kick_channels() queues every
ACTIVE channel. ncsi_process_next_channel() -> ncsi_configure_channel()
then sends NC-SI commands, arms request timers and schedules ndp->work on
a closed netdev that is being unregistered.

Before this patch, ncsi_unregister_dev() ran first, so this callback
found no NCSI device and returned early.

[ ... ]

> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index 040a31557201b..c4025e7f2cf26 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c
> @@ -93,8 +93,16 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
>  		if (had_link) {
>  			ndp->flags |= NCSI_DEV_RESHUFFLE;
>  			ncsi_stop_channel_monitor(nc);
> +			spin_lock_irqsave(&nc->lock, flags);
> +			if (nc->state != NCSI_CHANNEL_ACTIVE) {
> +				spin_unlock_irqrestore(&nc->lock, flags);
> +				return 0;
> +			}
> +			nc->state = NCSI_CHANNEL_INACTIVE;
> +			spin_unlock_irqrestore(&nc->lock, flags);
>  			spin_lock_irqsave(&ndp->lock, flags);
> -			list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> +			if (list_empty(&nc->link))
> +				list_add_tail_rcu(&nc->link, &ndp->channel_queue);
>  			spin_unlock_irqrestore(&ndp->lock, flags);
>  			return ncsi_process_next_channel(ndp);
>  		}

[Severity: High]
Is setting NCSI_CHANNEL_INACTIVE here intended? Before this patch the
channel was queued with its state still ACTIVE, so
ncsi_process_next_channel() took the suspend path:

    case NCSI_CHANNEL_INACTIVE:
        ...
        ncsi_configure_channel(ndp);
        break;
    case NCSI_CHANNEL_ACTIVE:
        ...
        ncsi_suspend_channel(ndp);

With INACTIVE, the channel that has just lost link is reconfigured
instead of suspended. The skipped steps are SP, the GLS refresh on all
channels triggered by NCSI_DEV_RESHUFFLE, and DCNT/DC/DP. The failed
channel therefore stays enabled on the NIC, and its
modes[NCSI_MODE_TX_ENABLE].enable flag stays set.

When ncsi_choose_active_channel() then picks a replacement,
ncsi_channel_is_tx() sees the stale TX enable flag and returns false, so
the replacement never gets ECNT. If the replacement is in another
package and there is no HWA, the old package is never deselected either.

Does this break failover after a link loss in the default
!multi_package && !multi_channel configuration? It looks like the CR/kick
pattern may have been copied into the LSC path, and this change seems
unrelated to the teardown use-after-free.

> @@ -158,12 +166,15 @@ static int ncsi_aen_handler_cr(struct ncsi_dev_priv *ndp,
>
>  	ncsi_stop_channel_monitor(nc);
>  	spin_lock_irqsave(&nc->lock, flags);
> -	nc->state = NCSI_CHANNEL_INVISIBLE;
> +	if (nc->state != NCSI_CHANNEL_ACTIVE) {
> +		spin_unlock_irqrestore(&nc->lock, flags);
> +		return 0;
> +	}
> +	nc->state = NCSI_CHANNEL_INACTIVE;
>  	spin_unlock_irqrestore(&nc->lock, flags);
> -
>  	spin_lock_irqsave(&ndp->lock, flags);
> -	nc->state = NCSI_CHANNEL_INACTIVE;
> -	list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> +	if (list_empty(&nc->link))
> +		list_add_tail_rcu(&nc->link, &ndp->channel_queue);
>  	spin_unlock_irqrestore(&ndp->lock, flags);

[Severity: Medium]
After ncsi_stop_channel_monitor(), both ncsi_aen_handler_cr() and
ncsi_aen_handler_lsc() recheck only nc->state, set INACTIVE and drop
nc->lock. Only after that do they take ndp->lock and test list_empty().

ncsi_kick_channels() does the check and the transition in a single
critical section:

    if (nc->state == NCSI_CHANNEL_ACTIVE &&
        list_empty(&nc->link)) {
        nc->state = NCSI_CHANNEL_INACTIVE;
        list_add_tail_rcu(&nc->link, &ndp->channel_queue);

The split sequence in the AEN handlers appears to allow two races.

First, timer_delete_sync() can wait for a monitor callback whose timeout
branch has already queued nc as ACTIVE. ncsi_channel_monitor() writes
nc->state under ndp->lock only. If another channel is ahead in the
queue, nc stays queued, and the AEN handler then overwrites its state
with INACTIVE. nc is later configured instead of suspended.

Second, a concurrent ncsi_reset_dev() -> ncsi_choose_active_channel()
(for example from a Netlink set_interface command) can run between the
nc->lock release and the ndp->lock acquisition. It can queue nc, and
ncsi_process_next_channel() can dequeue it and mark it INVISIBLE. The
AEN handler's list_empty() check then passes and it re-queues a channel
that is already being configured. The next dequeue hits the "Invalid
state" path, which calls ncsi_report_link(ndp, false).

Should these handlers check queue membership and change the state in the
same critical section as the enqueue?

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

[ ... ]

> @@ -1567,13 +1567,12 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
>  		goto out;
>  	}
>
> +	spin_lock(&nc->lock);
>  	list_del_init(&nc->link);
> -	spin_unlock_irqrestore(&ndp->lock, flags);
> -
> -	spin_lock_irqsave(&nc->lock, flags);
>  	old_state = nc->state;
>  	nc->state = NCSI_CHANNEL_INVISIBLE;
> -	spin_unlock_irqrestore(&nc->lock, flags);
> +	spin_unlock(&nc->lock);
> +	spin_unlock_irqrestore(&ndp->lock, flags);

[Severity: High]
Does this create an ABBA ordering between ndp->lock and nc->lock?

Here and in ncsi_kick_channels(), nc->lock is now taken inside ndp->lock.
ncsi_choose_active_channel() is unchanged and still nests the locks the
other way round:

    spin_lock_irqsave(&nc->lock, cflags);
    ...
        spin_lock_irqsave(&ndp->lock, flags);
        list_add_tail_rcu(&nc->link,
                          &ndp->channel_queue);
        spin_unlock_irqrestore(&ndp->lock, flags);
    ...
    spin_unlock_irqrestore(&nc->lock, cflags);

For example:

CPU0
ncsi_vlan_rx_add_vid()
  ncsi_kick_channels()
    spin_lock_irqsave(&ndp->lock, flags);
    spin_lock(&nc->lock);          <- spins

CPU1
ncsi_choose_active_channel()
    spin_lock_irqsave(&nc->lock, cflags);
    spin_lock_irqsave(&ndp->lock, flags);   <- spins

CPU1 can be reached from ncsi_dev_work, from ncsi_reset_dev() via
Netlink, from an AEN in NET_RX softirq, or from the monitor timer.
ncsi_dev_mutex serializes only the VLAN, register and unregister paths.

Both CPUs have IRQs disabled, so this would be a hard lockup. Lockdep
should also see both orders during normal bring-up, even when no
deadlock occurs.

[ ... ]

> @@ -1956,19 +2003,56 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)

[ ... ]

> +	for (i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
> +		struct ncsi_request *nr = &ndp->requests[i];
> +
> +		timer_delete_sync(&nr->timer);
> +		if (!nr->used)
> +			continue;
> +
> +		if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN && nr->cmd) {
> +			struct ncsi_cmd_pkt *cmd = (struct ncsi_cmd_pkt *)
> +				skb_network_header(nr->cmd);
> +
> +			ncsi_find_package_and_channel(ndp,
> +						      cmd->cmd.common.channel,
> +						      &np, &nc);

[Severity: Low]
Will this produce an "RCU-list traversed in non-reader section!" warning
under CONFIG_PROVE_RCU_LIST?

ncsi_find_package_and_channel() goes through ncsi_find_package() and
ncsi_find_channel(). Those walk ndp->packages and np->channels with
NCSI_FOR_EACH_PACKAGE/NCSI_FOR_EACH_CHANNEL, which are
list_for_each_entry_rcu() without a lockdep condition. Here they are
called from process context with no rcu_read_lock().

There are no concurrent writers at this point, so this would only be a
debug splat. Elsewhere the patch does wrap similar helpers in
rcu_read_lock().

> +			ncsi_send_netlink_timeout(nr, np, nc);

[Severity: Low]
This isn't a bug introduced by this patch, but this new caller inherits
it. When np is NULL, ncsi_send_netlink_timeout() falls back to:

    nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID,
                NCSI_PACKAGE_INDEX((((struct ncsi_pkt_hdr *)
                                     nr->cmd->data)->channel)));

ncsi_xmit_cmd() has already done skb_push(nr->cmd, sizeof(*eh)), so
nr->cmd->data points at the Ethernet header. Offset 5 is therefore
h_dest[5], which is 0xff for broadcast, and the reported package ID
becomes 7.

The teardown path reaches this fallback when an outstanding Netlink
command targets a package that was never discovered, because
ncsi_send_cmd_nl() only range-checks the package ID. The existing
ncsi_request_timeout() caller has the same problem.

> +		}
> +		ncsi_free_request(nr);
> +	}

[ ... ]

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