[PATCH net-next v2] mctp i2c: add sysfs control for mux child netdevs

[PATCH net-next v2] mctp i2c: add sysfs control for mux child netdevs

[Kenny Hsieh]

An MCTP netdev is created for an I2C bus that carries an
"mctp-controller" property in the devicetree. A bus instantiated at
runtime through the I2C sysfs interface has no devicetree node on which
to set that property, so it can never get a netdev.

This matters for a mux topology that is only known at runtime. A BMC
that ships one firmware image across several chassis cannot describe
every chassis's mux tree in its devicetree. Userspace creates the muxes
it finds over i2c sysfs instead, and the channels below them have no
node of their own. It also matters for an adapter that the devicetree
cannot describe at all, such as a USB-attached one.

Add a writable "mctp_controller" attribute on every adapter of a mux
tree whose root has an mctp-i2c-controller device bound to it, the root
adapter included. A mux tree with no such device is untouched. Writing 1
creates the netdev for that bus, writing 0 removes it, and reading
reports whether one exists. Userspace can then enable MCTP per channel
once it has created the muxes, and can control a controller that arrived
through new_device rather than through the devicetree.

The devicetree property keeps working, and both mechanisms may be used
on the same mux tree. The attribute is removed when the client is
unbound, so it does not outlive the module, and a rebind reattaches it
reading 0.

The netdev is placed in the network namespace the mctp-i2c-controller
device was probed in, not the one that writes the attribute, since the
attribute is visible in every namespace. The client holds a reference on
that namespace, and a write requires CAP_NET_ADMIN in it.

Tested on an AST2700 BMC with a nested mux tree: writing 1 to
mctp_controller on a channel that has no "mctp-controller" property
brings up the interface and writing 0 removes it, a write of 0 on the
root bus removes its netdev, and the attribute disappears on an unbind
of the mctp-i2c-controller driver and returns reading 0 on a rebind.

Signed-off-by: Kenny Hsieh <kenny_hsieh@jabil.com>
---
v2:
 - attach the attribute on root adapters too, so a controller that has no
   devicetree node can be controlled at runtime (Matt, Jeremy). This also
   drops the root special-casing and the -EINVAL a write there returned.
 - remove the attribute when the client is unbound, so it does not
   outlive the module (Matt). Removal walks a list of the adapters an
   attribute was added to, rather than the i2c bus, which keeps
   sysfs_remove_group() off i2c_for_each_dev().
 - take a reference on the stored network namespace, with a matching put,
   and require CAP_NET_ADMIN in it for a write. A stored-versus-current
   comparison is not available here, as discussed on v1.
 - move the duplicate-netdev check into mctp_i2c_add_netdev(), so all
   three callers get it and a concurrent store no longer produces an
   -EEXIST from mctp_register_netdev().
 - serialize attribute attachment through that same list, so
   sysfs_update_group() does not run twice at once on one adapter.
 - refuse a write for an adapter that is already being deleted, which
   the list membership answers.
 - do not free midev twice when mctp_register_netdev() fails, since
   register_netdevice() has already run ndo_uninit on every error path it
   takes.
 - drop the one-client-per-mux-tree check, which the existing non-root
   adapter check already covers (Matt).

v1: https://lore.kernel.org/netdev/20261002073156.759005-1-kenny_hsieh@jabil.com/

 .../ABI/testing/sysfs-bus-i2c-devices-mctp    |  26 +
 MAINTAINERS                                   |   1 +
 drivers/net/mctp/mctp-i2c.c                   | 464 ++++++++++++++++--
 3 files changed, 449 insertions(+), 42 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-mctp

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-mctp b/Documentation/ABI/testing/sysfs-bus-i2c-devices-mctp
new file mode 100644
index 0000000000000..a04bc30aa7b51
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-mctp
@@ -0,0 +1,26 @@
+What:		/sys/bus/i2c/devices/i2c-<n>/mctp_controller
+Date:		October 2026
+KernelVersion:	7.4
+Contact:	Kenny Hsieh <kenny_hsieh@jabil.com>
+Description:
+		Controls whether an MCTP network device exists for this I2C
+		bus. Writing 1 creates it, writing 0 removes it, and reading
+		returns 1 while it exists. Writing a value that matches the
+		current state is a no-op.
+
+		The attribute appears only on mux child busses whose mux tree
+		root has an mctp-i2c-controller device bound to it. The root
+		bus itself is excluded, because its network device follows
+		that mctp-i2c-controller device rather than this attribute.
+
+		A bus described in the devicetree can carry an
+		"mctp-controller" property instead, which creates the network
+		device when the mctp-i2c-controller device probes. This
+		attribute covers busses instantiated at runtime through the
+		I2C sysfs interface, which have no devicetree node on which to
+		set that property. Both mechanisms may be used on the same mux
+		tree.
+
+		Removal is synchronous and does not drain traffic: queued
+		packets are discarded and in-flight requests fail, so a
+		requester sees a timeout rather than an error.
diff --git a/MAINTAINERS b/MAINTAINERS
index 509b35b5a0e7a..2bfa5d68c6b66 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15719,6 +15719,7 @@ M:	Jeremy Kerr <jk@codeconstruct.com.au>
 M:	Matt Johnston <matt@codeconstruct.com.au>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/ABI/testing/sysfs-bus-i2c-devices-mctp
 F:	Documentation/networking/mctp.rst
 F:	drivers/net/mctp/
 F:	include/linux/usb/mctp-usb.h
diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
index b02476d338bbf..6c53bfcc50a3b 100644
--- a/drivers/net/mctp/mctp-i2c.c
+++ b/drivers/net/mctp/mctp-i2c.c
@@ -22,6 +22,8 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/if_arp.h>
+#include <linux/sysfs.h>
+#include <linux/nsproxy.h>
 #include <net/mctp.h>
 #include <net/mctpdevice.h>
 
@@ -83,17 +85,39 @@ struct mctp_i2c_dev {
 
 };
 
+/* One adapter that carries this client's mctp_controller attribute. The
+ * entry holds a reference on the adapter device, so it never points at a
+ * freed adapter.
+ */
+struct mctp_i2c_attr {
+	struct i2c_adapter *adap;
+	struct list_head list; /* For mctp_i2c_client.attrs */
+};
+
 /* The i2c client structure. One per hardware i2c bus at the top of the
  * mux tree, shared by multiple netdevs
  */
 struct mctp_i2c_client {
 	struct i2c_client *client;
 	u8 lladdr;
+	/* Namespace the controller was probed in. Netdevs are created here
+	 * regardless of which namespace writes the mctp_controller attribute.
+	 */
+	possible_net_t net;
 
 	struct mctp_i2c_dev *sel;
 	struct list_head devs;
 	spinlock_t sel_lock; /* Protects sel and devs */
 
+	/* Adapters this client has added the mctp_controller attribute to,
+	 * so removal never walks the i2c bus. attrs_lock is never held over
+	 * a sysfs group call or i2c_for_each_dev(): a group removal waits on
+	 * a kernfs reference that mctp_controller_store() holds while it
+	 * waits for driver_clients_lock.
+	 */
+	struct list_head attrs;
+	struct mutex attrs_lock; /* Protects attrs */
+
 	struct list_head list; /* For driver_clients */
 };
 
@@ -111,6 +135,11 @@ static int mctp_i2c_slave_cb(struct i2c_client *client,
 			     enum i2c_slave_event event, u8 *val);
 static void mctp_i2c_ndo_uninit(struct net_device *dev);
 static int mctp_i2c_ndo_open(struct net_device *dev);
+/* Defined below the attribute group, which the store handler it serves has
+ * to precede.
+ */
+static bool mctp_i2c_attr_tracked(struct mctp_i2c_client *mcli,
+				  struct i2c_adapter *adap);
 
 static struct i2c_adapter *mux_root_adapter(struct i2c_adapter *adap)
 {
@@ -162,7 +191,13 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
 	spin_lock_init(&mcli->sel_lock);
 	INIT_LIST_HEAD(&mcli->devs);
 	INIT_LIST_HEAD(&mcli->list);
+	INIT_LIST_HEAD(&mcli->attrs);
+	mutex_init(&mcli->attrs_lock);
 	mcli->lladdr = client->addr & 0xff;
+	/* The reference keeps the namespace alive for this client's lifetime,
+	 * and is dropped in mctp_i2c_free_client().
+	 */
+	write_pnet(&mcli->net, get_net(current->nsproxy->net_ns));
 	mcli->client = client;
 	i2c_set_clientdata(client, mcli);
 
@@ -189,14 +224,17 @@ static void mctp_i2c_free_client(struct mctp_i2c_client *mcli)
 
 	WARN_ON(!mutex_is_locked(&driver_clients_lock));
 	WARN_ON(!list_empty(&mcli->devs));
+	WARN_ON(!list_empty(&mcli->attrs));
 	WARN_ON(mcli->sel); /* sanity check, no locking */
 
 	rc = i2c_slave_unregister(mcli->client);
 	/* Leak if it fails, we can't propagate errors upwards */
-	if (rc < 0)
+	if (rc < 0) {
 		dev_err(&mcli->client->dev, "i2c unregister failed %d\n", rc);
-	else
+	} else {
+		put_net(read_pnet(&mcli->net));
 		kfree(mcli);
+	}
 }
 
 /* Switch the mctp i2c device to receive responses.
@@ -857,6 +895,25 @@ static int mctp_i2c_ndo_open(struct net_device *dev)
 	return 0;
 }
 
+/* Returns the netdev for adap, or NULL if there is none */
+static struct mctp_i2c_dev *mctp_i2c_find_dev(struct mctp_i2c_client *mcli,
+					      struct i2c_adapter *adap)
+{
+	struct mctp_i2c_dev *midev = NULL, *m = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mcli->sel_lock, flags);
+	/* List size is limited by number of MCTP netdevs on a single hardware bus */
+	list_for_each_entry(m, &mcli->devs, list)
+		if (m->adapter == adap) {
+			midev = m;
+			break;
+		}
+	spin_unlock_irqrestore(&mcli->sel_lock, flags);
+
+	return midev;
+}
+
 static int mctp_i2c_add_netdev(struct mctp_i2c_client *mcli,
 			       struct i2c_adapter *adap)
 {
@@ -876,6 +933,15 @@ static int mctp_i2c_add_netdev(struct mctp_i2c_client *mcli,
 	}
 
 	WARN_ON(!mutex_is_locked(&driver_clients_lock));
+
+	/* An existing netdev for @adap is success: all three callers can race
+	 * each other on one adapter, and only this check keeps the losers out
+	 * of mctp_register_netdev(), which would fail with -EEXIST and log an
+	 * error for a bus that already works.
+	 */
+	if (mctp_i2c_find_dev(mcli, adap))
+		return 0;
+
 	snprintf(namebuf, sizeof(namebuf), "mctpi2c%d", adap->nr);
 	ndev = alloc_netdev(sizeof(*midev), namebuf, NET_NAME_ENUM, mctp_i2c_net_setup);
 	if (!ndev) {
@@ -883,7 +949,12 @@ static int mctp_i2c_add_netdev(struct mctp_i2c_client *mcli,
 		rc = -ENOMEM;
 		goto err;
 	}
-	dev_net_set(ndev, current->nsproxy->net_ns);
+	/* Tie the netdev to the namespace the mctp-i2c-controller device was
+	 * probed in, not to whoever happens to write mctp_controller. A write
+	 * from another namespace must not place the netdev there, since the
+	 * attribute is visible in all of them.
+	 */
+	dev_net_set(ndev, read_pnet(&mcli->net));
 	SET_NETDEV_DEV(ndev, &adap->dev);
 	dev_addr_set(ndev, &mcli->lladdr);
 
@@ -900,6 +971,12 @@ static int mctp_i2c_add_netdev(struct mctp_i2c_client *mcli,
 		dev_err(&mcli->client->dev,
 			"register netdev \"%s\" failed %d\n",
 			ndev->name, rc);
+		/* register_netdevice() has already run ndo_uninit, which
+		 * frees midev. Every error path it takes reaches err_uninit
+		 * except err_free_name, and that one needs an ndo_init this
+		 * driver does not provide.
+		 */
+		midev = NULL;
 		goto err;
 	}
 
@@ -920,18 +997,10 @@ static int mctp_i2c_add_netdev(struct mctp_i2c_client *mcli,
 static void mctp_i2c_remove_netdev(struct mctp_i2c_client *mcli,
 				   struct i2c_adapter *adap)
 {
-	struct mctp_i2c_dev *midev = NULL, *m = NULL;
-	unsigned long flags;
+	struct mctp_i2c_dev *midev = NULL;
 
 	WARN_ON(!mutex_is_locked(&driver_clients_lock));
-	spin_lock_irqsave(&mcli->sel_lock, flags);
-	/* List size is limited by number of MCTP netdevs on a single hardware bus */
-	list_for_each_entry(m, &mcli->devs, list)
-		if (m->adapter == adap) {
-			midev = m;
-			break;
-		}
-	spin_unlock_irqrestore(&mcli->sel_lock, flags);
+	midev = mctp_i2c_find_dev(mcli, adap);
 
 	if (midev)
 		mctp_i2c_unregister(midev);
@@ -968,6 +1037,287 @@ static bool mctp_i2c_adapter_match(struct i2c_adapter *adap, bool match_no_of)
 	return of_property_read_bool(adap->dev.of_node, MCTP_I2C_OF_PROP);
 }
 
+/* Returns the client owning @root's mux tree, or NULL.
+ * Call with driver_clients_lock held. The returned pointer is only valid
+ * while that lock is held.
+ */
+static struct mctp_i2c_client *mctp_i2c_find_client(struct i2c_adapter *root)
+{
+	struct mctp_i2c_client *mcli = NULL;
+
+	WARN_ON(!mutex_is_locked(&driver_clients_lock));
+	list_for_each_entry(mcli, &driver_clients, list)
+		if (mcli->client->adapter == root)
+			return mcli;
+
+	return NULL;
+}
+
+/* A bus instantiated at runtime has no devicetree node on which to set the
+ * "mctp-controller" property, so this attribute is how userspace asks for its
+ * netdev once the adapter exists.
+ */
+static ssize_t mctp_controller_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct i2c_adapter *root = NULL, *adap = NULL;
+	struct mctp_i2c_client *mcli;
+	bool present = false;
+
+	adap = mctp_i2c_get_adapter(dev, &root);
+	if (!adap)
+		return -ENODEV;
+
+	mutex_lock(&driver_clients_lock);
+	mcli = mctp_i2c_find_client(root);
+	if (mcli)
+		present = mctp_i2c_find_dev(mcli, adap);
+	mutex_unlock(&driver_clients_lock);
+
+	return sysfs_emit(buf, "%d\n", present);
+}
+
+static ssize_t mctp_controller_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct i2c_adapter *root = NULL, *adap = NULL;
+	struct mctp_i2c_client *mcli;
+	bool enable;
+	int rc;
+
+	rc = kstrtobool(buf, &enable);
+	if (rc < 0)
+		return rc;
+
+	adap = mctp_i2c_get_adapter(dev, &root);
+	if (!adap)
+		return -ENODEV;
+
+	mutex_lock(&driver_clients_lock);
+	mcli = mctp_i2c_find_client(root);
+	if (!mcli) {
+		/* No mctp-i2c-controller on this mux tree's root */
+		rc = -ENODEV;
+		goto out;
+	}
+
+	/* A netdev store handler checks CAP_NET_ADMIN in the user namespace of
+	 * the netns owning the device, as netdev_store() does. The netns here
+	 * belongs to the client rather than to a netdev, so the check waits
+	 * until the lookup has found the client to read it from.
+	 */
+	if (!ns_capable(read_pnet(&mcli->net)->user_ns, CAP_NET_ADMIN)) {
+		rc = -EPERM;
+		goto out;
+	}
+
+	if (enable) {
+		/* device_del() sends BUS_NOTIFY_DEL_DEVICE well before
+		 * kobject_del() makes this attribute unreachable, so a write
+		 * can arrive after the adapter stopped being tracked. No
+		 * second notification would ever remove a netdev created here,
+		 * so refuse the write instead.
+		 */
+		if (!mctp_i2c_attr_tracked(mcli, adap)) {
+			rc = -ENODEV;
+			goto out;
+		}
+
+		rc = mctp_i2c_add_netdev(mcli, adap);
+	} else {
+		mctp_i2c_remove_netdev(mcli, adap);
+		rc = 0;
+	}
+out:
+	mutex_unlock(&driver_clients_lock);
+
+	return rc < 0 ? rc : count;
+}
+
+static DEVICE_ATTR_RW(mctp_controller);
+
+static struct attribute *mctp_i2c_adapter_attrs[] = {
+	&dev_attr_mctp_controller.attr,
+	NULL,
+};
+
+static const struct attribute_group mctp_i2c_adapter_group = {
+	.attrs = mctp_i2c_adapter_attrs,
+};
+
+/* Returns the tracking entry for @adap, or NULL.
+ * Call with mcli->attrs_lock held.
+ */
+static struct mctp_i2c_attr *__mctp_i2c_find_attr(struct mctp_i2c_client *mcli,
+						  struct i2c_adapter *adap)
+{
+	struct mctp_i2c_attr *attr = NULL;
+
+	lockdep_assert_held(&mcli->attrs_lock);
+	/* List size is limited by the number of adapters on one mux tree */
+	list_for_each_entry(attr, &mcli->attrs, list)
+		if (attr->adap == adap)
+			return attr;
+
+	return NULL;
+}
+
+/* Stops tracking @adap and frees its entry, without touching sysfs.
+ * Returns whether an entry was found, so the caller knows whether the
+ * attribute is still on the adapter.
+ */
+static bool mctp_i2c_drop_attr(struct mctp_i2c_client *mcli,
+			       struct i2c_adapter *adap)
+{
+	struct mctp_i2c_attr *attr;
+
+	mutex_lock(&mcli->attrs_lock);
+	attr = __mctp_i2c_find_attr(mcli, adap);
+	if (attr)
+		list_del(&attr->list);
+	mutex_unlock(&mcli->attrs_lock);
+
+	if (!attr)
+		return false;
+
+	put_device(&attr->adap->dev);
+	kfree(attr);
+	return true;
+}
+
+/* Reports whether the client owning @adap's mux tree still tracks the
+ * mctp_controller attribute for @adap. An adapter is untracked before its
+ * attribute goes away, so a write that finds no entry is a write to an
+ * adapter that is already being deleted, which mctp_controller_store() has
+ * to refuse rather than act on.
+ */
+static bool mctp_i2c_attr_tracked(struct mctp_i2c_client *mcli,
+				  struct i2c_adapter *adap)
+{
+	bool tracked;
+
+	mutex_lock(&mcli->attrs_lock);
+	tracked = __mctp_i2c_find_attr(mcli, adap);
+	mutex_unlock(&mcli->attrs_lock);
+
+	return tracked;
+}
+
+/* Adds the attribute to one adapter of @root's mux tree, the root included,
+ * and records it on the owning client. The entry goes in first, and only the
+ * caller that inserted it goes on to sysfs, so sysfs_update_group() never
+ * runs twice at once on one adapter. The group is never removed on its own.
+ */
+static void mctp_i2c_add_sysfs(struct i2c_adapter *adap,
+			       struct i2c_adapter *root)
+{
+	struct mctp_i2c_client *mcli;
+	struct mctp_i2c_attr *attr;
+	bool attach = false;
+	int rc;
+
+	attr = kzalloc_obj(*attr);
+	if (!attr) {
+		/* The attribute is an optional control rather than a
+		 * requirement, so skip the attach instead of failing.
+		 */
+		dev_warn(&adap->dev,
+			 "Failed tracking mctp_controller attribute\n");
+		return;
+	}
+	/* The entry must never point at a freed adapter */
+	get_device(&adap->dev);
+	attr->adap = adap;
+	INIT_LIST_HEAD(&attr->list);
+
+	mutex_lock(&driver_clients_lock);
+	mcli = mctp_i2c_find_client(root);
+	if (mcli) {
+		mutex_lock(&mcli->attrs_lock);
+		if (!__mctp_i2c_find_attr(mcli, adap)) {
+			list_add(&attr->list, &mcli->attrs);
+			attach = true;
+		}
+		mutex_unlock(&mcli->attrs_lock);
+	}
+	mutex_unlock(&driver_clients_lock);
+
+	if (!attach) {
+		/* Another attach path won, or there is no client to track it */
+		put_device(&adap->dev);
+		kfree(attr);
+		return;
+	}
+
+	/* Outside both locks: updating the group waits for a kernfs drain, and
+	 * a concurrent mctp_controller_store() holds that reference while it
+	 * waits for driver_clients_lock.
+	 */
+	rc = sysfs_update_group(&adap->dev.kobj, &mctp_i2c_adapter_group);
+	if (rc < 0) {
+		dev_warn(&adap->dev,
+			 "Failed adding mctp_controller attribute, %d\n", rc);
+		/* The list must not claim an attribute that is not there. The
+		 * entry is looked up again rather than reused, because the
+		 * client can go away while the group updates.
+		 */
+		mutex_lock(&driver_clients_lock);
+		mcli = mctp_i2c_find_client(root);
+		if (mcli)
+			mctp_i2c_drop_attr(mcli, adap);
+		mutex_unlock(&driver_clients_lock);
+	}
+}
+
+/* Removes the attribute from one adapter of @root's mux tree.
+ * Call with driver_clients_lock not held, for the reason struct
+ * mctp_i2c_client gives.
+ */
+static void mctp_i2c_remove_sysfs(struct i2c_adapter *adap,
+				  struct i2c_adapter *root)
+{
+	struct mctp_i2c_client *mcli;
+	bool tracked = false;
+
+	/* Hold @adap over the sysfs call, after the entry's reference goes */
+	get_device(&adap->dev);
+
+	mutex_lock(&driver_clients_lock);
+	mcli = mctp_i2c_find_client(root);
+	if (mcli)
+		tracked = mctp_i2c_drop_attr(mcli, adap);
+	mutex_unlock(&driver_clients_lock);
+
+	if (tracked)
+		sysfs_remove_group(&adap->dev.kobj, &mctp_i2c_adapter_group);
+
+	put_device(&adap->dev);
+}
+
+/* Removes every attribute @mcli added, as the client goes away. The list is
+ * detached first so sysfs_remove_group() runs with no lock held.
+ */
+static void mctp_i2c_remove_sysfs_all(struct mctp_i2c_client *mcli)
+{
+	struct mctp_i2c_attr *attr = NULL, *tmp = NULL;
+	LIST_HEAD(attrs);
+
+	lockdep_assert_not_held(&driver_clients_lock);
+
+	mutex_lock(&mcli->attrs_lock);
+	list_splice_init(&mcli->attrs, &attrs);
+	mutex_unlock(&mcli->attrs_lock);
+
+	list_for_each_entry_safe(attr, tmp, &attrs, list) {
+		sysfs_remove_group(&attr->adap->dev.kobj,
+				   &mctp_i2c_adapter_group);
+		list_del(&attr->list);
+		put_device(&attr->adap->dev);
+		kfree(attr);
+	}
+}
+
 /* Called for each existing i2c device (adapter or client) when a
  * new mctp-i2c client is probed.
  */
@@ -981,37 +1331,62 @@ static int mctp_i2c_client_try_attach(struct device *dev, void *data)
 		return 0;
 	if (mcli->client->adapter != root)
 		return 0;
+
+	/* Unmarked busses get the attribute too, so userspace can enable them */
+	mctp_i2c_add_sysfs(adap, root);
+
 	/* Must either have mctp-controller property on the adapter, or
 	 * be a root adapter if it's non-devicetree
 	 */
 	if (!mctp_i2c_adapter_match(adap, adap == root))
 		return 0;
 
-	return mctp_i2c_add_netdev(mcli, adap);
+	/* @mcli cannot be unbound here: the driver core holds the device lock
+	 * across probe, and an unbind needs that same lock.
+	 */
+	mutex_lock(&driver_clients_lock);
+	mctp_i2c_add_netdev(mcli, adap);
+	mutex_unlock(&driver_clients_lock);
+
+	return 0;
 }
 
 static void mctp_i2c_notify_add(struct device *dev)
 {
-	struct mctp_i2c_client *mcli = NULL, *m = NULL;
+	struct mctp_i2c_client *mcli = NULL;
 	struct i2c_adapter *root = NULL, *adap = NULL;
+	bool have_client;
+	bool match;
 	int rc;
 
 	adap = mctp_i2c_get_adapter(dev, &root);
 	if (!adap)
 		return;
+
 	/* Check for mctp-controller property on the adapter */
-	if (!mctp_i2c_adapter_match(adap, false))
-		return;
+	match = mctp_i2c_adapter_match(adap, false);
 
-	/* Find an existing mcli for adap's root */
 	mutex_lock(&driver_clients_lock);
-	list_for_each_entry(m, &driver_clients, list) {
-		if (m->client->adapter == root) {
-			mcli = m;
-			break;
-		}
-	}
+	have_client = mctp_i2c_find_client(root);
+	mutex_unlock(&driver_clients_lock);
 
+	if (!have_client)
+		return;
+
+	/* Outside the lock: updating the group waits for a kernfs drain, and a
+	 * concurrent mctp_controller_store() holds that reference while it
+	 * waits for driver_clients_lock.
+	 */
+	mctp_i2c_add_sysfs(adap, root);
+
+	if (!match)
+		return;
+
+	mutex_lock(&driver_clients_lock);
+	/* The pointer from the first lookup does not outlive the dropped
+	 * lock, so look the client up again.
+	 */
+	mcli = mctp_i2c_find_client(root);
 	if (mcli) {
 		rc = mctp_i2c_add_netdev(mcli, adap);
 		if (rc < 0)
@@ -1030,36 +1405,35 @@ static void mctp_i2c_notify_del(struct device *dev)
 		return;
 
 	mutex_lock(&driver_clients_lock);
-	list_for_each_entry(mcli, &driver_clients, list) {
-		if (mcli->client->adapter == root) {
-			mctp_i2c_remove_netdev(mcli, adap);
-			break;
-		}
-	}
+	mcli = mctp_i2c_find_client(root);
+	if (mcli)
+		mctp_i2c_remove_netdev(mcli, adap);
 	mutex_unlock(&driver_clients_lock);
+
+	/* Outside the lock, for the reason mctp_i2c_remove_sysfs() gives */
+	mctp_i2c_remove_sysfs(adap, root);
 }
 
 static int mctp_i2c_probe(struct i2c_client *client)
 {
-	struct mctp_i2c_client *mcli = NULL;
-	int rc;
+	struct mctp_i2c_client *mcli;
 
 	mutex_lock(&driver_clients_lock);
 	mcli = mctp_i2c_new_client(client);
 	if (IS_ERR(mcli)) {
-		rc = PTR_ERR(mcli);
-		mcli = NULL;
-		goto out;
-	} else {
-		list_add(&mcli->list, &driver_clients);
+		mutex_unlock(&driver_clients_lock);
+		return PTR_ERR(mcli);
 	}
+	list_add(&mcli->list, &driver_clients);
+	mutex_unlock(&driver_clients_lock);
 
-	/* Add a netdev for adapters that have a 'mctp-controller' property */
+	/* Add a netdev for adapters that have a 'mctp-controller' property.
+	 * Runs with the lock dropped, for the reason mctp_i2c_notify_add()
+	 * gives.
+	 */
 	i2c_for_each_dev(mcli, mctp_i2c_client_try_attach);
-	rc = 0;
-out:
-	mutex_unlock(&driver_clients_lock);
-	return rc;
+
+	return 0;
 }
 
 static void mctp_i2c_remove(struct i2c_client *client)
@@ -1067,6 +1441,12 @@ static void mctp_i2c_remove(struct i2c_client *client)
 	struct mctp_i2c_client *mcli = i2c_get_clientdata(client);
 	struct mctp_i2c_dev *midev = NULL, *tmp = NULL;
 
+	/* Before the lock: sysfs_remove_group() waits for a kernfs active
+	 * reference that mctp_controller_store() holds while it waits for
+	 * driver_clients_lock.
+	 */
+	mctp_i2c_remove_sysfs_all(mcli);
+
 	mutex_lock(&driver_clients_lock);
 	list_del(&mcli->list);
 	/* Remove all child adapter netdevs */
-- 
2.50.1

Re: [PATCH net-next v2] mctp i2c: add sysfs control for mux child netdevs

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 · Medium: 6 · Low: 2

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] When mctp_register_netdev() fails, mctp_i2c_add_netdev()
  (drivers/net/mctp/mctp-i2c.c) now always sets `midev = NULL`, so the…
- [High] The store-side guard against adapters being deleted does not
  close the race it claims to close.
- [High] mctp_i2c_remove() runs mctp_i2c_remove_sysfs_all() while the
  client is still on driver_clients.
- [Medium] The new ABI document Documentation/ABI/testing/sysfs-
  bus-i2c-devices-mctp says "The attribute appears only on mux child…
- [Medium] The new ABI lives on i2c adapter devices, which the I2C core
  owns, but the mctp-i2c driver attaches it with sysfs_update_group() on…
- [Medium] In the baseline, mctp_i2c_add_netdev() has a real double
  teardown.
- [Medium] mctp_i2c_new_client() now calls `write_pnet(&mcli->net,
  get_net(current->nsproxy->net_ns))` before i2c_slave_register().
- [Medium] mctp_i2c_probe() now releases driver_clients_lock after
  list_add() and before i2c_for_each_dev(mcli,…
- [Medium] Writing 0 to mctp_controller removes a channel's netdev
  through mctp_i2c_remove_netdev() -> mctp_i2c_unregister() ->…
- [Low] The commit message says "a rebind reattaches it reading 0".
- [Low] The commit message gives "an adapter that the devicetree cannot
  describe at all, such as a USB-attached one" as a use case.

--

mctp i2c: add sysfs control for mux child netdevs

This adds a writable mctp_controller sysfs attribute to every adapter of a
mux tree whose root has an mctp-i2c-controller client bound. Userspace can
use it to create or remove the MCTP netdev for busses that have no
devicetree node. Netdevs are now created in the namespace the client was
probed in, and the client holds a reference on that namespace.

> It also matters for an adapter that the devicetree
> cannot describe at all, such as a USB-attached one.

[Severity: Low]
Can the USB-attached adapter case actually happen? The attribute only
appears once an mctp-i2c-controller client binds at the root, and
mctp_i2c_new_client() needs i2c_slave_register() to succeed:

	rc = i2c_slave_register(mcli->client, mctp_i2c_slave_cb);
	if (rc < 0) {
		dev_err(&client->dev, "i2c register failed %d\n", rc);
		...
		goto err;
	}

That needs adapter->algo->reg_slave. In this tree only SoC controllers
provide it: aspeed, at91-slave, axxia, bcm-iproc, cadence, designware,
emev2, gxp, imx, mlxbf, npcm7xx, pxa, rcar and stm32f7. No USB-attached
adapter implements it, so neither a client nor the attribute can exist on
one.

Also, a root adapter without an OF node already gets its netdev
automatically through mctp_i2c_adapter_match(adap, adap == root).

> The attribute is removed when the client is
> unbound, so it does not outlive the module, and a rebind reattaches it
> reading 0.

[Severity: Low]
Does it really read 0 on every adapter after a rebind? A rebind runs
mctp_i2c_probe()->i2c_for_each_dev()->mctp_i2c_client_try_attach(). That
calls mctp_i2c_add_netdev() whenever mctp_i2c_adapter_match(adap,
adap == root) is true, which covers adapters with the "mctp-controller"
property and a root adapter without an OF node.

If userspace writes 0 on one of those and the controller is then rebound,
the netdev comes back and the attribute reads 1.

> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-mctp b/Documentation/ABI/testing/sysfs-bus-i2c-devices-mctp
> new file mode 100644
> index 000000000000..a04bc30aa7b5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-mctp
> @@ -0,0 +1,26 @@

[ ... ]

> +		The attribute appears only on mux child busses whose mux tree
> +		root has an mctp-i2c-controller device bound to it. The root
> +		bus itself is excluded, because its network device follows
> +		that mctp-i2c-controller device rather than this attribute.

[Severity: Medium]
Does this paragraph match the code? The commit message says "the root
adapter included", and mctp_i2c_client_try_attach() calls
mctp_i2c_add_sysfs() for the root as well:

	if (mcli->client->adapter != root)
		return 0;

	/* Unmarked busses get the attribute too, so userspace can enable them */
	mctp_i2c_add_sysfs(adap, root);

mctp_controller_store() also has no special case for the root, so writing
0 on the root bus removes its netdev.

Is this text left over from v1?

[ ... ]

> diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
> index b02476d338bb..6c53bfcc50a3 100644
> --- a/drivers/net/mctp/mctp-i2c.c
> +++ b/drivers/net/mctp/mctp-i2c.c

[ ... ]

> @@ -162,7 +191,13 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
>  	spin_lock_init(&mcli->sel_lock);
>  	INIT_LIST_HEAD(&mcli->devs);
>  	INIT_LIST_HEAD(&mcli->list);
> +	INIT_LIST_HEAD(&mcli->attrs);
> +	mutex_init(&mcli->attrs_lock);
>  	mcli->lladdr = client->addr & 0xff;
> +	/* The reference keeps the namespace alive for this client's lifetime,
> +	 * and is dropped in mctp_i2c_free_client().
> +	 */
> +	write_pnet(&mcli->net, get_net(current->nsproxy->net_ns));
>  	mcli->client = client;
>  	i2c_set_clientdata(client, mcli);
>  

[Severity: Medium]
Does this leak the netns reference when i2c_slave_register() fails? The
reference is taken before that call, and the error path frees mcli without
calling put_net():

	rc = i2c_slave_register(mcli->client, mctp_i2c_slave_cb);
	if (rc < 0) {
		dev_err(&client->dev, "i2c register failed %d\n", rc);
		mcli->client = NULL;
		i2c_set_clientdata(client, NULL);
		goto err;
	}
	...
err:
	if (mcli) {
		i2c_unregister_device(mcli->client);
		kfree(mcli);
	}

One way to hit this is an adapter without reg_slave, which returns
-EOPNOTSUPP. If the probe ran in a non-init namespace, that namespace can
never be freed.

[ ... ]

> @@ -900,6 +971,12 @@ static int mctp_i2c_add_netdev(struct mctp_i2c_client *mcli,
>  		dev_err(&mcli->client->dev,
>  			"register netdev \"%s\" failed %d\n",
>  			ndev->name, rc);
> +		/* register_netdevice() has already run ndo_uninit, which
> +		 * frees midev. Every error path it takes reaches err_uninit
> +		 * except err_free_name, and that one needs an ndo_init this
> +		 * driver does not provide.
> +		 */
> +		midev = NULL;
>  		goto err;
>  	}

[Severity: High]
Does this hold for every failure of mctp_register_netdev()? Some failures
return before err_uninit, so ndo_uninit never runs:

net/mctp/device.c:mctp_register_netdevice() {
	...
	mdev = mctp_add_dev(dev);
	if (IS_ERR(mdev))
		return PTR_ERR(mdev);
	...
}

net/core/dev.c:register_netdevice() {
	...
	ret = ethtool_check_ops(dev->ethtool_ops);
	if (ret)
		return ret;
	...
	ret = dev_get_valid_name(net, dev, dev->name);
	if (ret < 0)
		goto out;

	ret = -ENOMEM;
	dev->name_node = netdev_name_node_head_alloc(dev);
	if (!dev->name_node)
		goto out;
	...
}

On all of these paths, mctp_i2c_midev_init() has already done four things:

- linked midev into mcli->devs
- possibly set mcli->sel
- taken get_device() on the adapter and the client
- woken tx_thread

With midev = NULL, the err label only calls free_netdev(ndev), and that
frees the private area holding midev.

Can this leave the tx kthread running on freed memory? It would also leave
a freed node on mcli->devs, which later mctp_i2c_find_dev() and
mctp_i2c_remove() calls walk. The leaked adapter reference would make
i2c_del_adapter() wait forever on dev_released.

The name collision case is deterministic. The name is a fixed
"mctpi2c<nr>" with no '%', so dev_get_valid_name() returns -EEXIST if any
netdev in the client's namespace already has that name:

  ip link set dev X name mctpi2c7
  echo 1 > /sys/bus/i2c/devices/i2c-7/mctp_controller

Before this patch, these early paths ran mctp_i2c_midev_free() correctly.

[Severity: Medium]
Separately, the double teardown on late failures that this addresses
already exists in the baseline. On the err_uninit paths, and when a
NETDEV_REGISTER notifier fails, mctp_i2c_ndo_uninit()->mctp_i2c_midev_free()
runs first. The err label then calls mctp_i2c_midev_free() again, which
does a second list_del() and a second put_device() on both the adapter and
the client.

Could that fix be split into its own patch with a Fixes: tag, so it can go
to net and stable? Right now it is only described in the v2 notes below
the ---, and those don't end up in git history.

[ ... ]

> @@ -968,6 +1037,287 @@ static bool mctp_i2c_adapter_match(struct i2c_adapter *adap, bool match_no_of)

[ ... ]

> +static ssize_t mctp_controller_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{

[ ... ]

> +		rc = mctp_i2c_add_netdev(mcli, adap);
> +	} else {
> +		mctp_i2c_remove_netdev(mcli, adap);
> +		rc = 0;
> +	}

[Severity: Medium]
What happens to receive traffic on this channel after a write of 0? The
call chain is mctp_i2c_remove_netdev()->mctp_i2c_unregister()->
ndo_uninit->mctp_i2c_midev_free(). That releases any flow lock and points
mcli->sel at whichever midev is first on the list:

	if (mcli->sel == midev) {
		struct mctp_i2c_dev *first;

		first = list_first_entry_or_null(&mcli->devs, struct mctp_i2c_dev, list);
		__mctp_i2c_device_select(mcli, first);
	}

The physical mux is not deselected. With an idle-as-is mux such as
pca954x it can stay on the removed channel. mctp_i2c_recv() attributes all
target-mode receive data to mcli->sel->ndev, with
cb->haddr[0] = source_slave >> 1.

Could late replies or unsolicited writes from endpoints on the removed
channel then be delivered on an unrelated netdev, and a reply be sent on
the wrong mux channel? Before this patch, a netdev only went away together
with its adapter or the client.

If so, the ABI text "in-flight requests fail, so a requester sees a timeout
rather than an error" is also not accurate, because the late reply can
still arrive through another interface in the same MCTP net.

[ ... ]

> +static void mctp_i2c_add_sysfs(struct i2c_adapter *adap,
> +			       struct i2c_adapter *root)
> +{

[ ... ]

> +	/* Outside both locks: updating the group waits for a kernfs drain, and
> +	 * a concurrent mctp_controller_store() holds that reference while it
> +	 * waits for driver_clients_lock.
> +	 */
> +	rc = sysfs_update_group(&adap->dev.kobj, &mctp_i2c_adapter_group);

[Severity: Medium]
Is it ok for mctp-i2c to add an attribute group to an i2c adapter device,
which the I2C core owns, after that device has been registered? The
commit message's main case is a runtime mux whose adapters already exist
when the mctp-i2c-controller client binds. There,
mctp_i2c_client_try_attach() adds mctp_controller long after KOBJ_ADD was
sent, and no uevent is generated.

udev rules keyed on the adapter would never see the attribute appear.
Whether it exists also depends on whether, and when, a separate client
binds. Adapters added after the bind are fine, because
BUS_NOTIFY_ADD_DEVICE runs before KOBJ_ADD.

Should this ABI on the adapter device be agreed with the I2C maintainers?

[ ... ]

> +static void mctp_i2c_remove_sysfs_all(struct mctp_i2c_client *mcli)
> +{
> +	struct mctp_i2c_attr *attr = NULL, *tmp = NULL;
> +	LIST_HEAD(attrs);
> +
> +	lockdep_assert_not_held(&driver_clients_lock);
> +
> +	mutex_lock(&mcli->attrs_lock);
> +	list_splice_init(&mcli->attrs, &attrs);
> +	mutex_unlock(&mcli->attrs_lock);
> +
> +	list_for_each_entry_safe(attr, tmp, &attrs, list) {
> +		sysfs_remove_group(&attr->adap->dev.kobj,
> +				   &mctp_i2c_adapter_group);
> +		list_del(&attr->list);
> +		put_device(&attr->adap->dev);
> +		kfree(attr);
> +	}
> +}

[Severity: High]
Can this race with mctp_i2c_add_sysfs() for an adapter X being added to
the same tree? mctp_i2c_remove() calls this before list_del(&mcli->list),
so mctp_i2c_notify_add()->mctp_i2c_add_sysfs() can still find the client.

If add_sysfs inserts its entry after the splice, mctp_i2c_free_client()
hits WARN_ON(!list_empty(&mcli->attrs)) and frees mcli with the entry
still linked. The entry's reference on X leaks. A later notify_del for X
finds no client, so i2c_del_adapter(X) hangs on dev_released.

If add_sysfs inserted its entry and dropped both locks before calling
sysfs_update_group():

CPU0 mctp_i2c_add_sysfs(X)       CPU1 mctp_i2c_remove_sysfs_all()
list_add(&attr->list, ...)
unlock both locks
                                 list_splice_init(&mcli->attrs, &attrs)
                                 sysfs_remove_group()  /* nothing yet */
                                 put_device(); kfree(attr)
sysfs_update_group()

then mctp_controller is created on X with no tracking entry, and nothing
ever removes it.

After rmmod, would a read or write of i2c-X/mctp_controller go into the
freed module's dev_attr_mctp_controller and its show/store handlers? That
would contradict "so it does not outlive the module" in the commit
message.

[ ... ]

> @@ -1030,36 +1405,35 @@ static void mctp_i2c_notify_del(struct device *dev)
>  		return;
>  
>  	mutex_lock(&driver_clients_lock);
> -	list_for_each_entry(mcli, &driver_clients, list) {
> -		if (mcli->client->adapter == root) {
> -			mctp_i2c_remove_netdev(mcli, adap);
> -			break;
> -		}
> -	}
> +	mcli = mctp_i2c_find_client(root);
> +	if (mcli)
> +		mctp_i2c_remove_netdev(mcli, adap);
>  	mutex_unlock(&driver_clients_lock);
> +
> +	/* Outside the lock, for the reason mctp_i2c_remove_sysfs() gives */
> +	mctp_i2c_remove_sysfs(adap, root);
>  }

[Severity: High]
Does the mctp_i2c_attr_tracked() check in mctp_controller_store() close
the deleted-adapter race? Here the netdev is removed and
driver_clients_lock is dropped before mctp_i2c_remove_sysfs() untracks the
adapter. The attribute is still writable because kobject_del() has not run
yet:

CPU0                                CPU1
mctp_i2c_notify_del()
  mutex_lock(&driver_clients_lock)
  mctp_i2c_remove_netdev()
  mutex_unlock(&driver_clients_lock)
                                    mctp_controller_store("1")
                                      mctp_i2c_attr_tracked() == true
                                      mctp_i2c_add_netdev()
                                        get_device(&adap->dev)
mctp_i2c_remove_sysfs()
  mctp_i2c_drop_attr()
  sysfs_remove_group()
/* back in i2c_del_adapter() */
wait_for_completion(&adap->dev_released)

No second BUS_NOTIFY_DEL_DEVICE arrives, so the new netdev keeps its
reference on the dying adapter. Wouldn't i2c_del_adapter() then block
until the mctp-i2c client is unbound?

Would untracking the adapter before mctp_i2c_remove_netdev(), in the same
critical section, avoid this?

>  
>  static int mctp_i2c_probe(struct i2c_client *client)
>  {
> -	struct mctp_i2c_client *mcli = NULL;
> -	int rc;
> +	struct mctp_i2c_client *mcli;
>  
>  	mutex_lock(&driver_clients_lock);
>  	mcli = mctp_i2c_new_client(client);
>  	if (IS_ERR(mcli)) {
> -		rc = PTR_ERR(mcli);
> -		mcli = NULL;
> -		goto out;
> -	} else {
> -		list_add(&mcli->list, &driver_clients);
> +		mutex_unlock(&driver_clients_lock);
> +		return PTR_ERR(mcli);
>  	}
> +	list_add(&mcli->list, &driver_clients);
> +	mutex_unlock(&driver_clients_lock);
>  
> -	/* Add a netdev for adapters that have a 'mctp-controller' property */
> +	/* Add a netdev for adapters that have a 'mctp-controller' property.
> +	 * Runs with the lock dropped, for the reason mctp_i2c_notify_add()
> +	 * gives.
> +	 */
>  	i2c_for_each_dev(mcli, mctp_i2c_client_try_attach);

[Severity: Medium]
With driver_clients_lock no longer held across the walk, can
mctp_i2c_notify_del() for a mux adapter X finish while this walk is still
running? device_del() sends BUS_NOTIFY_DEL_DEVICE before
bus_remove_device() takes X off the bus klist:

CPU0                                CPU1
                                    mctp_i2c_probe()
                                      list_add(&mcli->list, ...)
                                      mutex_unlock(&driver_clients_lock)
device_del(X)
  mctp_i2c_notify_del()
    /* nothing to remove for X yet */
                                      i2c_for_each_dev()
                                        mctp_i2c_client_try_attach(X)
                                          mctp_i2c_add_sysfs()
                                            get_device(&X->dev)
                                          mctp_i2c_add_netdev()
  bus_remove_device(X)

Nothing drops those references afterwards, so i2c_del_adapter(X) would
block on dev_released until the client is unbound.

Before this patch, probe held driver_clients_lock across the walk, which
limited this to a narrower window affecting only the netdev. This change
widens the window, and the tracking entry adds a second leaked reference.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015081341.1262695-1-kenny_hsieh%40jabil.com