[PATCH v3] net/ncsi: Fix Use-After-Free in NCSI channel and package removal

[PATCH v3] net/ncsi: Fix Use-After-Free in NCSI channel and package removal

[Wong Boon Jhee]

In net/ncsi/ncsi-manage.c, ncsi_remove_channel() and
ncsi_remove_package() remove objects from an RCU-protected linked list
using list_del_rcu() and immediately free them using kfree().

Because there is no call to synchronize_rcu() or kfree_rcu(), concurrent
readers traversing these lists under rcu_read_lock() (such as Netlink
dump handlers) can still hold a valid pointer to the object. When kfree()
executes, the reader is left holding a dangling pointer to freed memory,
resulting in a slab-use-after-free.

This patch fixes the issue by implementing a proper kref and RCU lifetime
model for NCSI objects. It replaces kfree() with kfree_rcu() / call_rcu()
to defer memory freeing until all pre-existing RCU readers have finished
their critical sections. It also introduces a kref for ncsi_dev_priv to
prevent the device structure from being freed while netlink handlers are
still operating on it. Concurrent lockless readers are now explicitly
protected with rcu_read_lock().

RCU read-side critical sections are added around lockless package and
channel traversals that may race with object removal. The KASAN
reproducer no longer reports the slab-use-after-free.

Fixes: 2d283bdd079c ("net/ncsi: Resource management")
Signed-off-by: Wong Boon Jhee <wongboonjhee52@gmail.com>
---
 net/ncsi/internal.h     |   8 ++-
 net/ncsi/ncsi-manage.c  | 113 ++++++++++++++++++++++++++++++++--------
 net/ncsi/ncsi-netlink.c |  56 ++++++++++++++++----
 3 files changed, 144 insertions(+), 33 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index adee6dcabdc3..fbd90f270f5e 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -239,6 +239,7 @@ struct ncsi_channel {
 	} monitor;
 	struct list_head            node;
 	struct list_head            link;
+	struct rcu_head             rcu; /* RCU cleanup */
 };
 
 struct ncsi_package {
@@ -253,6 +254,7 @@ struct ncsi_package {
 	bool                 multi_channel; /* Enable multiple channels  */
 	u32                  channel_whitelist; /* Channels to configure */
 	struct ncsi_channel  *preferred_channel; /* Primary channel      */
+	struct rcu_head      rcu; /* RCU cleanup */
 };
 
 struct ncsi_request {
@@ -315,6 +317,8 @@ struct vlan_vid {
 };
 
 struct ncsi_dev_priv {
+	struct rcu_head     rcu;             /* RCU cleanup */
+	struct kref         ref;
 	struct ncsi_dev     ndev;            /* Associated NCSI device     */
 	unsigned int        flags;           /* NCSI device flags          */
 #define NCSI_DEV_PROBED		1            /* Finalized NCSI topology    */
@@ -337,6 +341,7 @@ struct ncsi_dev_priv {
 	struct ncsi_channel *active_channel; /* Currently handled channel  */
 	struct list_head    channel_queue;   /* Config queue of channels   */
 	struct work_struct  work;            /* For channel management     */
+	bool                work_cancelled;  /* Work has been cancelled    */
 	struct packet_type  ptype;           /* NCSI packet Rx handler     */
 	struct list_head    node;            /* Form NCSI device list      */
 #define NCSI_MAX_VLAN_VIDS	15
@@ -406,7 +411,8 @@ int ncsi_update_tx_channel(struct ncsi_dev_priv *ndp,
 			   struct ncsi_package *np,
 			   struct ncsi_channel *disable,
 			   struct ncsi_channel *enable);
-
+struct ncsi_dev_priv *ncsi_dev_get(struct net_device *dev);
+void ncsi_dev_put(struct ncsi_dev_priv *ndp);
 /* Packet handlers */
 u32 ncsi_calculate_checksum(unsigned char *data, int len);
 int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca);
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 54d0df0a9efe..afc34c2b3613 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/rcupdate.h>
 
 #include <net/ncsi.h>
 #include <net/net_namespace.h>
@@ -35,17 +36,23 @@ bool ncsi_channel_is_last(struct ncsi_dev_priv *ndp,
 {
 	struct ncsi_package *np;
 	struct ncsi_channel *nc;
+	bool is_last = true;
 
-	NCSI_FOR_EACH_PACKAGE(ndp, np)
+	rcu_read_lock();
+	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		NCSI_FOR_EACH_CHANNEL(np, nc) {
 			if (nc == channel)
 				continue;
 			if (nc->state == NCSI_CHANNEL_ACTIVE &&
-			    ncsi_channel_has_link(nc))
-				return false;
+			    ncsi_channel_has_link(nc)) {
+				is_last = false;
+				goto out;
+			}
 		}
-
-	return true;
+	}
+ out:
+	rcu_read_unlock();
+	return is_last;
 }
 
 static void ncsi_report_link(struct ncsi_dev_priv *ndp, bool force_down)
@@ -62,6 +69,7 @@ static void ncsi_report_link(struct ncsi_dev_priv *ndp, bool force_down)
 	}
 
 	nd->link_up = 0;
+	rcu_read_lock();
 	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		NCSI_FOR_EACH_CHANNEL(np, nc) {
 			spin_lock_irqsave(&nc->lock, flags);
@@ -75,12 +83,14 @@ static void ncsi_report_link(struct ncsi_dev_priv *ndp, bool force_down)
 			if (ncsi_channel_has_link(nc)) {
 				spin_unlock_irqrestore(&nc->lock, flags);
 				nd->link_up = 1;
+				rcu_read_unlock();
 				goto report;
 			}
 
 			spin_unlock_irqrestore(&nc->lock, flags);
 		}
 	}
+	rcu_read_unlock();
 
 report:
 	nd->handler(nd);
@@ -242,28 +252,37 @@ struct ncsi_channel *ncsi_add_channel(struct ncsi_package *np, unsigned char id)
 	return nc;
 }
 
+static void ncsi_channel_rcu_free(struct rcu_head *head)
+{
+	struct ncsi_channel *nc = container_of(head, struct ncsi_channel, rcu);
+
+	kfree(nc->mac_filter.addrs);
+	kfree(nc->vlan_filter.vids);
+	kfree(nc);
+}
+
 static void ncsi_remove_channel(struct ncsi_channel *nc)
 {
 	struct ncsi_package *np = nc->package;
+	struct ncsi_dev_priv *ndp = np->ndp;
 	unsigned long flags;
 
-	spin_lock_irqsave(&nc->lock, flags);
-
-	/* Release filters */
-	kfree(nc->mac_filter.addrs);
-	kfree(nc->vlan_filter.vids);
+	ncsi_stop_channel_monitor(nc);
 
+	spin_lock_irqsave(&nc->lock, flags);
 	nc->state = NCSI_CHANNEL_INACTIVE;
 	spin_unlock_irqrestore(&nc->lock, flags);
-	ncsi_stop_channel_monitor(nc);
 
-	/* Remove and free channel */
+	spin_lock_irqsave(&ndp->lock, flags);
+	list_del_rcu(&nc->link);
+	spin_unlock_irqrestore(&ndp->lock, flags);
+
 	spin_lock_irqsave(&np->lock, flags);
 	list_del_rcu(&nc->node);
 	np->channel_num--;
 	spin_unlock_irqrestore(&np->lock, flags);
 
-	kfree(nc);
+	call_rcu(&nc->rcu, ncsi_channel_rcu_free);
 }
 
 struct ncsi_package *ncsi_find_package(struct ncsi_dev_priv *ndp,
@@ -326,7 +345,7 @@ void ncsi_remove_package(struct ncsi_package *np)
 	ndp->package_num--;
 	spin_unlock_irqrestore(&ndp->lock, flags);
 
-	kfree(np);
+	kfree_rcu(np, rcu);
 }
 
 void ncsi_find_package_and_channel(struct ncsi_dev_priv *ndp,
@@ -409,7 +428,8 @@ void ncsi_free_request(struct ncsi_request *nr)
 	spin_unlock_irqrestore(&ndp->lock, flags);
 
 	if (driven && cmd && --ndp->pending_req_num == 0)
-		schedule_work(&ndp->work);
+		if (!READ_ONCE(ndp->work_cancelled))
+			schedule_work(&ndp->work);
 
 	/* Release command and response */
 	consume_skb(cmd);
@@ -912,6 +932,7 @@ int ncsi_update_tx_channel(struct ncsi_dev_priv *ndp,
 	nca.req_flags = 0;
 
 	/* Find current channel with Tx enabled */
+	rcu_read_lock();
 	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		if (disable)
 			break;
@@ -924,8 +945,10 @@ int ncsi_update_tx_channel(struct ncsi_dev_priv *ndp,
 				break;
 			}
 	}
+	rcu_read_unlock();
 
 	/* Find a suitable channel for Tx */
+	rcu_read_lock();
 	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		if (enable)
 			break;
@@ -951,6 +974,7 @@ int ncsi_update_tx_channel(struct ncsi_dev_priv *ndp,
 			}
 		}
 	}
+	rcu_read_unlock();
 
 	if (disable == enable)
 		return -1;
@@ -1253,6 +1277,7 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
 	 */
 	found = NULL;
 	with_link = false;
+	rcu_read_lock();
 	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		if (!(ndp->package_whitelist & (0x1 << np->id)))
 			continue;
@@ -1304,6 +1329,7 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
 		if (with_link && !ndp->multi_package)
 			break;
 	}
+	rcu_read_unlock();
 
 	if (list_empty(&ndp->channel_queue) && found) {
 		netdev_info(ndp->ndev.dev,
@@ -1332,6 +1358,7 @@ static bool ncsi_check_hwa(struct ncsi_dev_priv *ndp)
 	/* The hardware arbitration is disabled if any one channel
 	 * doesn't support explicitly.
 	 */
+	rcu_read_lock();
 	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		NCSI_FOR_EACH_CHANNEL(np, nc) {
 			has_channel = true;
@@ -1345,6 +1372,7 @@ static bool ncsi_check_hwa(struct ncsi_dev_priv *ndp)
 			}
 		}
 	}
+	rcu_read_unlock();
 
 	if (has_channel) {
 		ndp->flags |= NCSI_DEV_HWA;
@@ -1782,6 +1810,7 @@ struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
 	ndp->package_whitelist = UINT_MAX;
 
 	/* Initialize private NCSI device */
+	kref_init(&ndp->ref);
 	spin_lock_init(&ndp->lock);
 	INIT_LIST_HEAD(&ndp->packages);
 	ndp->request_id = NCSI_REQ_START_IDX;
@@ -1952,23 +1981,65 @@ int ncsi_reset_dev(struct ncsi_dev *nd)
 	return 0;
 }
 
+static void ncsi_dev_release(struct kref *ref)
+{
+	struct ncsi_dev_priv *ndp;
+
+	ndp = container_of(ref, struct ncsi_dev_priv, ref);
+	kfree_rcu(ndp, rcu);
+}
+
+struct ncsi_dev_priv *ncsi_dev_get(struct net_device *dev)
+{
+	struct ncsi_dev_priv *ndp = NULL;
+	struct ncsi_dev *nd;
+
+	rcu_read_lock();
+	nd = ncsi_find_dev(dev);
+	if (nd) {
+		ndp = TO_NCSI_DEV_PRIV(nd);
+		/* Safely grab a reference while under RCU lock */
+		if (!kref_get_unless_zero(&ndp->ref))
+			ndp = NULL;
+	}
+	rcu_read_unlock();
+
+	return ndp;
+}
+
+void ncsi_dev_put(struct ncsi_dev_priv *ndp)
+{
+	if (ndp)
+		kref_put(&ndp->ref, ncsi_dev_release);
+}
+
 void ncsi_unregister_dev(struct ncsi_dev *nd)
 {
 	struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
 	struct ncsi_package *np, *tmp;
 	unsigned long flags;
 
-	dev_remove_pack(&ndp->ptype);
-
-	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
-		ncsi_remove_package(np);
-
 	spin_lock_irqsave(&ncsi_dev_lock, flags);
 	list_del_rcu(&ndp->node);
 	spin_unlock_irqrestore(&ncsi_dev_lock, flags);
 
+	spin_lock_irqsave(&ndp->lock, flags);
+	ndp->work_cancelled = true;
+	spin_unlock_irqrestore(&ndp->lock, flags);
+
+	for (int i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
+		struct ncsi_request *nr = &ndp->requests[i];
+
+		if (nr->enabled)
+			ncsi_free_request(nr);
+	}
+
+	dev_remove_pack(&ndp->ptype);
 	disable_work_sync(&ndp->work);
 
-	kfree(ndp);
+	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
+		ncsi_remove_package(np);
+
+	kref_put(&ndp->ref, ncsi_dev_release);
 }
 EXPORT_SYMBOL_GPL(ncsi_unregister_dev);
diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
index 8cc538358f6a..59fd735c367a 100644
--- a/net/ncsi/ncsi-netlink.c
+++ b/net/ncsi/ncsi-netlink.c
@@ -35,8 +35,6 @@ static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex)
 {
 	struct ncsi_dev_priv *ndp;
 	struct net_device *dev;
-	struct ncsi_dev *nd;
-	struct ncsi_dev;
 
 	if (!net)
 		return NULL;
@@ -47,9 +45,7 @@ static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex)
 		return NULL;
 	}
 
-	nd = ncsi_find_dev(dev);
-	ndp = nd ? TO_NCSI_DEV_PRIV(nd) : NULL;
-
+	ndp = ncsi_dev_get(dev);
 	dev_put(dev);
 	return ndp;
 }
@@ -175,13 +171,16 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
 		return -ENODEV;
 
 	skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
-	if (!skb)
+	if (!skb) {
+		ncsi_dev_put(ndp);
 		return -ENOMEM;
+	}
 
 	hdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
 			  &ncsi_genl_family, 0, NCSI_CMD_PKG_INFO);
 	if (!hdr) {
 		kfree_skb(skb);
+		ncsi_dev_put(ndp);
 		return -EMSGSIZE;
 	}
 
@@ -190,9 +189,13 @@ 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);
+		ncsi_dev_put(ndp);
 		return -EMSGSIZE;
 	}
+
+	rcu_read_lock();
 	rc = ncsi_write_package_info(skb, ndp, package_id);
+	rcu_read_unlock();
 
 	if (rc) {
 		nla_nest_cancel(skb, attr);
@@ -202,10 +205,12 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
 	nla_nest_end(skb, attr);
 
 	genlmsg_end(skb, hdr);
+	ncsi_dev_put(ndp);
 	return genlmsg_reply(skb, info);
 
 err:
 	kfree_skb(skb);
+	ncsi_dev_put(ndp);
 	return rc;
 }
 
@@ -235,14 +240,17 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
 		return -ENODEV;
 
 	package_id = cb->args[0];
+	rcu_read_lock();
 	package = NULL;
 	NCSI_FOR_EACH_PACKAGE(ndp, np)
 		if (np->id == package_id)
 			package = np;
 
-	if (!package)
+	if (!package) {
+		rcu_read_unlock();
+		ncsi_dev_put(ndp);
 		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) {
@@ -255,7 +263,9 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
 		rc = -EMSGSIZE;
 		goto err;
 	}
+
 	rc = ncsi_write_package_info(skb, ndp, package->id);
+
 	if (rc) {
 		nla_nest_cancel(skb, attr);
 		goto err;
@@ -266,9 +276,13 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
 
 	cb->args[0] = package_id + 1;
 
+	rcu_read_unlock();
+	ncsi_dev_put(ndp);
 	return skb->len;
 err:
+	rcu_read_unlock();
 	genlmsg_cancel(skb, hdr);
+	ncsi_dev_put(ndp);
 	return rc;
 }
 
@@ -297,11 +311,15 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
 	package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
 	package = NULL;
 
+	rcu_read_lock();
 	NCSI_FOR_EACH_PACKAGE(ndp, np)
 		if (np->id == package_id)
 			package = np;
+
 	if (!package) {
 		/* The user has set a package that does not exist */
+		rcu_read_unlock();
+		ncsi_dev_put(ndp);
 		return -ERANGE;
 	}
 
@@ -317,6 +335,8 @@ 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();
+			ncsi_dev_put(ndp);
 			return -ERANGE;
 		}
 	}
@@ -337,6 +357,7 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
 		package->preferred_channel = NULL;
 	}
 	spin_unlock_irqrestore(&package->lock, flags);
+	rcu_read_unlock();
 
 	if (channel)
 		netdev_info(ndp->ndev.dev,
@@ -350,6 +371,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);
 
+	ncsi_dev_put(ndp);
 	return 0;
 }
 
@@ -376,6 +398,7 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
 	ndp->multi_package = false;
 	spin_unlock_irqrestore(&ndp->lock, flags);
 
+	rcu_read_lock();
 	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		spin_lock_irqsave(&np->lock, flags);
 		np->multi_channel = false;
@@ -383,18 +406,21 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
 		np->preferred_channel = NULL;
 		spin_unlock_irqrestore(&np->lock, flags);
 	}
+	rcu_read_unlock();
+
 	netdev_info(ndp->ndev.dev, "NCSI: Cleared preferred package/channel\n");
 
 	/* Update channel configuration */
 	if (!(ndp->flags & NCSI_DEV_RESET))
 		ncsi_reset_dev(&ndp->ndev);
 
+	ncsi_dev_put(ndp);
 	return 0;
 }
 
 static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
 {
-	struct ncsi_dev_priv *ndp;
+	struct ncsi_dev_priv *ndp = NULL;
 	struct ncsi_pkt_hdr *hdr;
 	struct ncsi_cmd_arg nca;
 	unsigned char *data;
@@ -480,6 +506,7 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
 				      ret);
 	}
 out:
+	ncsi_dev_put(ndp);
 	return ret;
 }
 
@@ -639,6 +666,7 @@ static int ncsi_set_package_mask_nl(struct sk_buff *msg,
 			ncsi_reset_dev(&ndp->ndev);
 	}
 
+	ncsi_dev_put(ndp);
 	return rc;
 }
 
@@ -670,14 +698,17 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
 
 	package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
 	package = NULL;
+	rcu_read_lock();
 	NCSI_FOR_EACH_PACKAGE(ndp, np)
 		if (np->id == package_id) {
 			package = np;
 			break;
 		}
-	if (!package)
+	if (!package) {
+		rcu_read_unlock();
+		ncsi_dev_put(ndp);
 		return -ERANGE;
-
+	}
 	spin_lock_irqsave(&package->lock, flags);
 
 	channel = NULL;
@@ -690,6 +721,7 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
 			}
 		if (!channel) {
 			spin_unlock_irqrestore(&package->lock, flags);
+			ncsi_dev_put(ndp);
 			return -ERANGE;
 		}
 		netdev_dbg(ndp->ndev.dev,
@@ -716,11 +748,13 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
 	}
 
 	spin_unlock_irqrestore(&package->lock, flags);
+	rcu_read_unlock();
 
 	/* Update channel configuration */
 	if (!(ndp->flags & NCSI_DEV_RESET))
 		ncsi_reset_dev(&ndp->ndev);
 
+	ncsi_dev_put(ndp);
 	return 0;
 }
 
-- 
2.55.0

Re: [v3] net/ncsi: Fix Use-After-Free in NCSI channel and package removal

[netdev-bot+sashiko]

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

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

- [High] Two code paths newly created by this patch return with the RCU
  read-side critical section still held.
- [High] The patch defers package/channel frees with
  call_rcu()/kfree_rcu() but leaves the main consumers outside any RCU…
- [High] The new kref pins struct ncsi_dev_priv, but every netlink
  handler actually dereferences ndp->ndev.dev (the struct net_device),…
- [High] The new pending-request sweep in ncsi_unregister_dev() runs
  before its producers are stopped.
- [Medium] The commit message describes only (a) kfree_rcu()/call_rcu()
  deferral, (b) a kref on ncsi_dev_priv, and (c) rcu_read_lock() around…

Pre-existing issues:
- [Medium] The patch adds ncsi_dev_get()/ncsi_dev_put() and documents, in
  code, that the lookup must run 'while under RCU lock', but converts…
- [Medium] ndp->channel_queue (linked through nc->link) is read with
  list_for_each_entry_rcu() in ncsi_channel_is_tx() (net/ncsi/ncsi-…
- [Medium] Six NCSI netlink handlers call
  ndp_from_ifindex(get_net(sock_net(...)), ifindex) — net/ncsi/ncsi-…

--

net/ncsi: Fix Use-After-Free in NCSI channel and package removal

This converts the NCSI package and channel frees from kfree() to
kfree_rcu()/call_rcu(), adds a kref to struct ncsi_dev_priv with new
ncsi_dev_get()/ncsi_dev_put() helpers used by the netlink handlers, and
wraps several lockless package/channel traversals in rcu_read_lock().

A few questions about the result below.

The commit message says:

    This patch fixes the issue by implementing a proper kref and RCU
    lifetime model for NCSI objects. It replaces kfree() with kfree_rcu()
    / call_rcu() [...] Concurrent lockless readers are now explicitly
    protected with rcu_read_lock().

[Severity: Medium]
Could the message also describe the other changes folded into this patch?
The ones I could not find mentioned anywhere are:

- ncsi_unregister_dev() is reordered so that dev_remove_pack() (and its
  synchronize_net()) and disable_work_sync() now run after a new
  pending-request sweep, and the package removal loop now runs after
  disable_work_sync().
- a new request-draining loop, "if (nr->enabled) ncsi_free_request(nr)".
- a new bool work_cancelled field which gates exactly one of the nine
  schedule_work(&ndp->work) call sites in ncsi-manage.c.
- ncsi_remove_channel() now unlinks the channel from ndp->channel_queue
  with list_del_rcu(&nc->link), which looks like the most concrete
  use-after-free actually fixed here.

Also, on this sentence:

    Because there is no call to synchronize_rcu() or kfree_rcu(),
    concurrent readers traversing these lists under rcu_read_lock() (such
    as Netlink dump handlers) can still hold a valid pointer to the
    object.

Were there any such readers before this patch? ncsi_pkg_info_all_nl()
only gains its rcu_read_lock() in this same patch, so the described
readers did not exist yet.

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

[ ... ]

> @@ -242,28 +252,37 @@ struct ncsi_channel *ncsi_add_channel(struct ncsi_package *np, unsigned char id)
>  	return nc;
>  }
>  
> +static void ncsi_channel_rcu_free(struct rcu_head *head)
> +{
> +	struct ncsi_channel *nc = container_of(head, struct ncsi_channel, rcu);
> +
> +	kfree(nc->mac_filter.addrs);
> +	kfree(nc->vlan_filter.vids);
> +	kfree(nc);
> +}
> +
>  static void ncsi_remove_channel(struct ncsi_channel *nc)
>  {
>  	struct ncsi_package *np = nc->package;
> +	struct ncsi_dev_priv *ndp = np->ndp;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&nc->lock, flags);
> -
> -	/* Release filters */
> -	kfree(nc->mac_filter.addrs);
> -	kfree(nc->vlan_filter.vids);
> +	ncsi_stop_channel_monitor(nc);
>  
> +	spin_lock_irqsave(&nc->lock, flags);
>  	nc->state = NCSI_CHANNEL_INACTIVE;
>  	spin_unlock_irqrestore(&nc->lock, flags);
> -	ncsi_stop_channel_monitor(nc);
>  
> -	/* Remove and free channel */
> +	spin_lock_irqsave(&ndp->lock, flags);
> +	list_del_rcu(&nc->link);
> +	spin_unlock_irqrestore(&ndp->lock, flags);
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but ndp->channel_queue now has
three different removal primitives applied to it.

ncsi_process_next_channel() and ncsi_reset_dev() both remove entries with
list_del_init():

	list_del_init(&nc->link);

	list_for_each_entry_safe(nc, tmp, &ndp->channel_queue, link)
		list_del_init(&nc->link);

while the reader in ncsi_channel_is_tx() treats it as an RCU list:

	list_for_each_entry_rcu(channel, &ndp->channel_queue, link) {

list_del_init() makes the removed node point at itself, so a reader
positioned on it under list_for_each_entry_rcu() never reaches the head.
Should whichever semantics is intended for this list be applied at all
three removal sites, rather than only adding list_del_rcu() here?

>  	spin_lock_irqsave(&np->lock, flags);
>  	list_del_rcu(&nc->node);
>  	np->channel_num--;
>  	spin_unlock_irqrestore(&np->lock, flags);
>  
> -	kfree(nc);
> +	call_rcu(&nc->rcu, ncsi_channel_rcu_free);
>  }
>  
>  struct ncsi_package *ncsi_find_package(struct ncsi_dev_priv *ndp,

[ ... ]

> @@ -326,7 +345,7 @@ void ncsi_remove_package(struct ncsi_package *np)
>  	ndp->package_num--;
>  	spin_unlock_irqrestore(&ndp->lock, flags);
>  
> -	kfree(np);
> +	kfree_rcu(np, rcu);
>  }

[ ... ]

> @@ -409,7 +428,8 @@ void ncsi_free_request(struct ncsi_request *nr)
>  	spin_unlock_irqrestore(&ndp->lock, flags);
>  
>  	if (driven && cmd && --ndp->pending_req_num == 0)
> -		schedule_work(&ndp->work);
> +		if (!READ_ONCE(ndp->work_cancelled))
> +			schedule_work(&ndp->work);
>  
>  	/* Release command and response */
>  	consume_skb(cmd);

[ ... ]

> @@ -912,6 +932,7 @@ int ncsi_update_tx_channel(struct ncsi_dev_priv *ndp,
>  	nca.req_flags = 0;
>  
>  	/* Find current channel with Tx enabled */
> +	rcu_read_lock();
>  	NCSI_FOR_EACH_PACKAGE(ndp, np) {
>  		if (disable)
>  			break;

[ ... ]

> @@ -951,6 +974,7 @@ int ncsi_update_tx_channel(struct ncsi_dev_priv *ndp,
>  			}
>  		}
>  	}
> +	rcu_read_unlock();
>  
>  	if (disable == enable)
>  		return -1;

[Severity: High]
Are the channel pointers found inside these read sections still safe to
use after rcu_read_unlock()? Here disable and enable are looked up under
RCU, and then dereferenced after the section ends:

	nca.channel = disable->id;
	nca.package = disable->package->id;
	...
	ret = ncsi_xmit_cmd(&nca);

ncsi_choose_active_channel() has the same shape - found is picked up
inside the section and then linked into a live list afterwards:

	rcu_read_unlock();

	if (list_empty(&ndp->channel_queue) && found) {
		...
		list_add_tail_rcu(&found->link, &ndp->channel_queue);

If found has been removed and call_rcu()'d in the meantime, does this
splice a freed list node into ndp->channel_queue?

Related: ncsi_reset_dev() walks the same lists with no RCU read section
and no ndp->lock at all:

	active = NULL;
	NCSI_FOR_EACH_PACKAGE(ndp, np) {
		NCSI_FOR_EACH_CHANNEL(np, nc) {
			spin_lock_irqsave(&nc->lock, flags);

Both NCSI_FOR_EACH_PACKAGE() and NCSI_FOR_EACH_CHANNEL() expand to
list_for_each_entry_rcu(), and ncsi_reset_dev() is called by
ncsi_set_interface_nl(), ncsi_clear_interface_nl(),
ncsi_set_package_mask_nl() and ncsi_set_channel_mask_nl() after they drop
rcu_read_lock(), holding only the new ndp kref - which pins
ncsi_dev_priv, not the packages or channels. With the frees now deferred
by a grace period rather than immediate, can a concurrent
ncsi_unregister_dev() let ncsi_channel_rcu_free() run while
ncsi_reset_dev() is touching nc->state and nc->lock?

ncsi_kick_channels(), ncsi_channel_is_tx(), ncsi_stop_dev() and
ncsi_find_package()/ncsi_find_package_and_channel() (the latter reached
from the ncsi_request_timeout() timer) also still traverse these RCU
lists without a read section, so CONFIG_PROVE_RCU_LIST should complain
about them too.

[ ... ]

> @@ -1332,6 +1358,7 @@ static bool ncsi_check_hwa(struct ncsi_dev_priv *ndp)
>  	/* The hardware arbitration is disabled if any one channel
>  	 * doesn't support explicitly.
>  	 */
> +	rcu_read_lock();
>  	NCSI_FOR_EACH_PACKAGE(ndp, np) {
>  		NCSI_FOR_EACH_CHANNEL(np, nc) {
>  			has_channel = true;
> @@ -1345,6 +1372,7 @@ static bool ncsi_check_hwa(struct ncsi_dev_priv *ndp)
>  			}
>  		}
>  	}
> +	rcu_read_unlock();
>  
>  	if (has_channel) {
>  		ndp->flags |= NCSI_DEV_HWA;

[Severity: High]
Does ncsi_check_hwa() return with the read section still held? The early
exit inside the nested loop was not converted:

			cap = nc->caps[NCSI_CAP_GENERIC].cap;
			if (!(cap & NCSI_CAP_GENERIC_HWA) ||
			    (cap & NCSI_CAP_GENERIC_HWA_MASK) !=
			    NCSI_CAP_GENERIC_HWA_SUPPORT) {
				ndp->flags &= ~NCSI_DEV_HWA;
				return false;
			}

That return is taken for any channel that does not advertise hardware
arbitration, which is the common case during probing, and it skips the
new rcu_read_unlock() after the loop. ncsi_channel_is_last() in this same
patch was converted with "goto out" for exactly this reason.

There is a second one in net/ncsi/ncsi-netlink.c,
ncsi_set_channel_mask_nl(), which takes rcu_read_lock() before the
package lookup but returns without unlocking when the channel id does not
resolve:

		if (!channel) {
			spin_unlock_irqrestore(&package->lock, flags);
			ncsi_dev_put(ndp);
			return -ERANGE;
		}

The sibling "if (!package)" path a few lines above does call
rcu_read_unlock(). That one is reachable at will from user space by
passing an NCSI_ATTR_CHANNEL_ID that does not exist.

Returning to user space with rcu_read_lock_nesting (or the preempt count)
left elevated should produce scheduling-while-atomic or lockdep splats,
and since the task never leaves the read section, do grace periods stall
so that the kfree_rcu()/call_rcu() frees added by this patch never run?

[ ... ]

> @@ -1952,23 +1981,65 @@ int ncsi_reset_dev(struct ncsi_dev *nd)
>  	return 0;
>  }
>  
> +static void ncsi_dev_release(struct kref *ref)
> +{
> +	struct ncsi_dev_priv *ndp;
> +
> +	ndp = container_of(ref, struct ncsi_dev_priv, ref);
> +	kfree_rcu(ndp, rcu);
> +}
> +
> +struct ncsi_dev_priv *ncsi_dev_get(struct net_device *dev)
> +{
> +	struct ncsi_dev_priv *ndp = NULL;
> +	struct ncsi_dev *nd;
> +
> +	rcu_read_lock();
> +	nd = ncsi_find_dev(dev);
> +	if (nd) {
> +		ndp = TO_NCSI_DEV_PRIV(nd);
> +		/* Safely grab a reference while under RCU lock */
> +		if (!kref_get_unless_zero(&ndp->ref))
> +			ndp = NULL;
> +	}
> +	rcu_read_unlock();
> +
> +	return ndp;
> +}

[Severity: Medium]
This is a pre-existing pattern, but should the other ncsi_find_dev()
callers be converted too? The comment here documents that the lookup must
run "while under RCU lock", yet ncsi_vlan_rx_add_vid(),
ncsi_vlan_rx_kill_vid(), ncsi_register_dev() and ncsi_rcv_rsp() still do:

	nd = ncsi_find_dev(dev);
	...
	ndp = TO_NCSI_DEV_PRIV(nd);

and ncsi_find_dev() iterates ncsi_dev_list with:

	#define NCSI_FOR_EACH_DEV(ndp) \
		list_for_each_entry_rcu(ndp, &ncsi_dev_list, node)

with neither a reference nor (for the vlan and register paths) an
rcu_read_lock(). Since ncsi_dev_release() now frees ndp with
kfree_rcu(), the deferral does not help those callers, and internal.h now
advertises two lookup APIs with different safety contracts.

> +
> +void ncsi_dev_put(struct ncsi_dev_priv *ndp)
> +{
> +	if (ndp)
> +		kref_put(&ndp->ref, ncsi_dev_release);
> +}
> +
>  void ncsi_unregister_dev(struct ncsi_dev *nd)
>  {
>  	struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
>  	struct ncsi_package *np, *tmp;
>  	unsigned long flags;
>  
> -	dev_remove_pack(&ndp->ptype);
> -
> -	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
> -		ncsi_remove_package(np);
> -
>  	spin_lock_irqsave(&ncsi_dev_lock, flags);
>  	list_del_rcu(&ndp->node);
>  	spin_unlock_irqrestore(&ncsi_dev_lock, flags);
>  
> +	spin_lock_irqsave(&ndp->lock, flags);
> +	ndp->work_cancelled = true;
> +	spin_unlock_irqrestore(&ndp->lock, flags);
> +
> +	for (int i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
> +		struct ncsi_request *nr = &ndp->requests[i];
> +
> +		if (nr->enabled)
> +			ncsi_free_request(nr);
> +	}
> +
> +	dev_remove_pack(&ndp->ptype);
>  	disable_work_sync(&ndp->work);
>  
> -	kfree(ndp);
> +	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
> +		ncsi_remove_package(np);
> +
> +	kref_put(&ndp->ref, ncsi_dev_release);
>  }
>  EXPORT_SYMBOL_GPL(ncsi_unregister_dev);

[Severity: High]
Does the new request sweep run before its producers are stopped?

The order is now: unlink ndp from ncsi_dev_list, set work_cancelled,
free every enabled request, dev_remove_pack(), disable_work_sync(),
remove packages (the only path reaching ncsi_stop_channel_monitor() and
timer_delete_sync(&nc->monitor.timer)), then kfree_rcu(ndp).

dev_remove_pack() used to be first, and it is the only synchronize_net()
barrier here. With it moved after the sweep, ndp->ptype is still
registered while the loop runs, so ncsi_rcv_rsp() can be in progress:

net/ncsi/ncsi-rsp.c:ncsi_rcv_rsp() {
	...
	nr->rsp = skb;
	...
	spin_unlock_irqrestore(&ndp->lock, flags);
	payload = nrh->payload;
	...
	ret = ncsi_validate_rsp_pkt(nr, payload);
	...
}

Can the sweep's ncsi_free_request() clear and consume_skb() nr->rsp and
nr->cmd while ncsi_rcv_rsp() is using them after dropping ndp->lock?
Note also that both contexts then execute "--ndp->pending_req_num"
outside ndp->lock.

Second, the channel monitor timers are only stopped in the package
removal loop, which now happens after the sweep. A monitor tick landing
in that window calls ncsi_xmit_cmd():

net/ncsi/ncsi-cmd.c:ncsi_xmit_cmd() {
	...
	nr->enabled = true;
	mod_timer(&nr->timer, jiffies + 1 * HZ);
	...
}

Nothing deletes that timer afterwards, and kfree_rcu(ndp) only waits one
grace period, so does ncsi_request_timeout() then dereference nr->ndp
and ndp->lock in freed memory about a second later?

Third, the sweep only frees requests with nr->enabled set. Do requests
that are nr->used with nr->cmd or nr->rsp set, but not enabled, still
leak their skbs?

> diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
> index 8cc538358f6a..59fd735c367a 100644
> --- a/net/ncsi/ncsi-netlink.c
> +++ b/net/ncsi/ncsi-netlink.c
> @@ -35,8 +35,6 @@ static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex)
>  {
>  	struct ncsi_dev_priv *ndp;
>  	struct net_device *dev;
> -	struct ncsi_dev *nd;
> -	struct ncsi_dev;
>  
>  	if (!net)
>  		return NULL;
> @@ -47,9 +45,7 @@ static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex)
>  		return NULL;
>  	}
>  
> -	nd = ncsi_find_dev(dev);
> -	ndp = nd ? TO_NCSI_DEV_PRIV(nd) : NULL;
> -
> +	ndp = ncsi_dev_get(dev);
>  	dev_put(dev);
>  	return ndp;
>  }

[Severity: High]
The commit message says the kref exists "to prevent the device structure
from being freed while netlink handlers are still operating on it". Which
device structure does the kref protect?

It pins struct ncsi_dev_priv, but the handlers dereference
ndp->ndev.dev, the struct net_device, and this helper drops the netdev
reference with dev_put() before returning. After ncsi_unregister_dev()
returns, the driver continues:

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

ncsi_unregister_dev() now only does kref_put(), so ndp stays alive as
long as a handler holds a reference, and nothing marks ndp dead or
re-checks ndp->ndev.dev. Can a handler still holding the kref then touch
the freed netdev, for example via:

	ncsi_send_netlink_err(ndp->ndev.dev, ... );

or netdev_info()/netdev_err()/netdev_dbg() in ncsi_set_interface_nl(),
ncsi_clear_interface_nl() and ncsi_set_channel_mask_nl(), or
ncsi_reset_dev(&ndp->ndev) reaching ncsi_xmit_cmd() and dev_queue_xmit()
on that netdev? The NCSI genl ops do not take RTNL, so they are not
serialised against netdev teardown.

[Severity: Medium]
This is a pre-existing issue, but while ndp_from_ifindex() is being
touched: does every NCSI netlink command leak a struct net reference? All
six callers pass a reference in:

	ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));

and neither ndp_from_ifindex() nor any caller calls put_net(). A grep
over net/ncsi/ shows six get_net() calls and no put_net(), so
NCSI_CMD_PKG_INFO, SET_INTERFACE, CLEAR_INTERFACE, SEND_CMD,
SET_PACKAGE_MASK and SET_CHANNEL_MASK each pin the namespace
permanently, whether or not the device lookup succeeds.

[ ... ]

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