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

[PATCH net-next] 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.

Add a writable "mctp_controller" attribute on mux child adapters below
a bus that has an mctp-i2c-controller device bound to it. 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.

The root adapter is excluded: its netdev follows the bound
mctp-i2c-controller device rather than this attribute, so a write there
returns -EINVAL. The devicetree property keeps working, and both
mechanisms may be used on the same mux tree.

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.

Only one mctp-i2c-controller client is allowed per mux tree. A second
one would add the same attribute to every child adapter again, and
either client's removal would delete the attribute the other still
needs.

Tested on an AST2700 BMC with a nested mux tree by writing 1 to
mctp_controller on a channel that has no "mctp-controller" property,
which brings up the interface, then writing 0, which removes it. The
attribute also survives an unbind and rebind of the
mctp-i2c-controller driver.

Signed-off-by: Kenny Hsieh <kenny_hsieh@jabil.com>
---
 .../ABI/testing/sysfs-bus-i2c-devices-mctp    |  26 ++
 MAINTAINERS                                   |   1 +
 drivers/net/mctp/mctp-i2c.c                   | 256 +++++++++++++++---
 3 files changed, 242 insertions(+), 41 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 000000000000..a04bc30aa7b5
--- /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 3011f995437f..1ba32f61a21a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15718,6 +15718,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 b02476d338bb..d4da32656744 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>
 
@@ -89,6 +91,10 @@ struct mctp_i2c_dev {
 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;
@@ -128,7 +134,7 @@ static struct i2c_adapter *mux_root_adapter(struct i2c_adapter *adap)
  */
 static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
 {
-	struct mctp_i2c_client *mcli = NULL;
+	struct mctp_i2c_client *mcli = NULL, *m = NULL;
 	struct i2c_adapter *root = NULL;
 	int rc;
 
@@ -154,6 +160,21 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
 		goto err;
 	}
 
+	/* One client per mux tree. A second one would add the same
+	 * mctp_controller attribute to every child adapter again, and either
+	 * client's removal would delete the attribute the other still needs.
+	 */
+	WARN_ON(!mutex_is_locked(&driver_clients_lock));
+	list_for_each_entry(m, &driver_clients, list) {
+		if (m->client->adapter == root) {
+			dev_err(&client->dev,
+				"An mctp-i2c-controller client is already attached to adapter %s\n",
+				root->name);
+			rc = -EBUSY;
+			goto err;
+		}
+	}
+
 	mcli = kzalloc_obj(*mcli);
 	if (!mcli) {
 		rc = -ENOMEM;
@@ -163,6 +184,7 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
 	INIT_LIST_HEAD(&mcli->devs);
 	INIT_LIST_HEAD(&mcli->list);
 	mcli->lladdr = client->addr & 0xff;
+	write_pnet(&mcli->net, current->nsproxy->net_ns);
 	mcli->client = client;
 	i2c_set_clientdata(client, mcli);
 
@@ -857,6 +879,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)
 {
@@ -883,7 +924,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);
 
@@ -920,18 +966,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 +1006,121 @@ 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;
+
+	/* The root adapter's netdev follows the mctp-i2c-controller client
+	 * itself, so it is not ours to add or remove here.
+	 */
+	if (adap == root)
+		return -EINVAL;
+
+	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;
+	}
+
+	if (enable) {
+		if (mctp_i2c_find_dev(mcli, adap))
+			rc = 0;	/* Already present, nothing to do */
+		else
+			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,
+};
+
+/* The root adapter is skipped: its netdev follows the bound client. The group
+ * is never removed, since kernfs drops it when the adapter is deleted, and
+ * updating rather than creating lets a rebind accept an existing group.
+ */
+static void mctp_i2c_add_sysfs(struct i2c_adapter *adap,
+			       struct i2c_adapter *root)
+{
+	int rc;
+
+	if (adap == root)
+		return;
+
+	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);
+}
+
 /* Called for each existing i2c device (adapter or client) when a
  * new mctp-i2c client is probed.
  */
@@ -981,37 +1134,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 +1208,32 @@ 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);
 }
 
 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)
-- 
2.50.1

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

[netdev-bot+sashiko]

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

Critical: 1 · High: 1 (1) · Medium: 4 (1) · Low: 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).

- [Critical] Use-after-free of module text and data through the leftover
  mctp_controller sysfs file.
- [High] mctp_i2c_new_client() caches the probe-time namespace with
  write_pnet(&mcli->net, current->nsproxy->net_ns) (line 187) and never…
- [Medium] The ABI document disagrees with the code.
- [Medium] mctp_controller_store() can create a netdev for an adapter
  that is already being deleted.
- [Medium] mctp_i2c_probe() now publishes mcli on driver_clients and
  walks the bus with driver_clients_lock dropped, and…
- [Medium] net: TOCTOU race allows concurrent sysfs writes to attempt
  duplicate netdev creation
- [Low] The commit message says that for the root adapter 'a write there
  returns -EINVAL'.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new
  mctp_controller store makes it reachable on demand.
- [Medium] This problem wasn't introduced by this patch.

--

mctp i2c: add sysfs control for mux child netdevs

This patch adds a writable mctp_controller sysfs attribute to the mux
child adapters below a root bus that has an mctp-i2c-controller bound.
Writing 1 or 0 to it creates or removes the MCTP netdev for that channel.
The patch also stores the probe-time network namespace in the client,
allows only one client per mux tree, and drops driver_clients_lock while
probe walks the bus.

> The root adapter is excluded: its netdev follows the bound
> mctp-i2c-controller device rather than this attribute, so a write there
> returns -EINVAL. The devicetree property keeps working, and both
> mechanisms may be used on the same mux tree.

[Severity: Low]
Is this accurate? mctp_i2c_add_sysfs() returns early for the root adapter:

	if (adap == root)
		return;

So mctp_controller never exists on the root bus, and a write there fails
with ENOENT, not EINVAL. That also seems to make the adap == root check in
mctp_controller_store() unreachable.

> Tested on an AST2700 BMC with a nested mux tree by writing 1 to
> mctp_controller on a channel that has no "mctp-controller" property,
> which brings up the interface, then writing 0, which removes it. The
> attribute also survives an unbind and rebind of the
> mctp-i2c-controller driver.

The file survives, but does the enabled state survive too?

mctp_i2c_remove() unregisters every midev, including the ones created
through mctp_controller. On rebind, mctp_i2c_client_try_attach() only
recreates netdevs for DT-marked adapters (or a non-OF root). Channels that
userspace enabled read 0 afterwards.

Should the commit message or the ABI document say that userspace has to
write 1 again after a rebind?

> 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

[ ... ]

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

[Severity: Medium]
Does the code match this description? The group is never removed (the
mctp_i2c_add_sysfs() comment says so), and mctp_i2c_remove() does not
touch it. After the controller is unbound, the attribute stays on every
child adapter that existed while it was bound.

Child adapters created after the unbind never get the attribute, because
mctp_i2c_notify_add() returns before it calls mctp_i2c_add_sysfs():

	if (!have_client)
		return;

So whether a channel has the attribute depends on the order things were
created, not on whether a controller is bound.

The no-op rule doesn't hold either when no controller is bound.
mctp_controller_show() reports 0, but mctp_controller_store() returns
-ENODEV for every value, including a write of 0.

The commit message gives this reason for the one-client-per-tree rule:
"either client's removal would delete the attribute the other still
needs". The comment in mctp_i2c_new_client() says the same. With no
removal path, does that reason still apply?

[ ... ]

> diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
> index b02476d338bbf..d4da32656744a 100644
> --- a/drivers/net/mctp/mctp-i2c.c
> +++ b/drivers/net/mctp/mctp-i2c.c

[ ... ]

> @@ -163,6 +184,7 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
>  	INIT_LIST_HEAD(&mcli->devs);
>  	INIT_LIST_HEAD(&mcli->list);
>  	mcli->lladdr = client->addr & 0xff;
> +	write_pnet(&mcli->net, current->nsproxy->net_ns);

[Severity: High]
Should this hold a reference on the namespace? write_pnet() is a plain
rcu_assign_pointer(). The driver never calls get_net() or put_net(), and
mctp_i2c_free_client() releases nothing.

Suppose the controller is probed from a non-init netns, for example by
writing new_device or the driver's bind file under ip netns exec, and that
netns is later deleted. The struct net is freed while mcli lives on. Any
MCTP netdevs in it are moved to init_net, but mcli->net still points at
the freed namespace.

Later, a write of 1 to mctp_controller or a DT-marked mux child being
added goes through:

mctp_i2c_add_netdev()
    dev_net_set(ndev, read_pnet(&mcli->net));
    mctp_register_netdev()
        register_netdevice()

Can this become a use-after-free of struct net in dev_get_valid_name(),
dev_index_reserve() or list_netdevice()?

Before this patch, mctp_i2c_add_netdev() used current->nsproxy->net_ns
at call time, which was always live.

[ ... ]

> @@ -968,6 +1006,121 @@ 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)
> +{

[ ... ]

> +	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;
> +	}
> +
> +	if (enable) {
> +		if (mctp_i2c_find_dev(mcli, adap))
> +			rc = 0;	/* Already present, nothing to do */
> +		else
> +			rc = mctp_i2c_add_netdev(mcli, adap);

[Severity: Medium]
Can this create a netdev for an adapter that is already being deleted?

device_del() sends BUS_NOTIFY_DEL_DEVICE, which runs
mctp_i2c_notify_del(), well before it calls kobject_del(). The
mctp_controller file was added straight onto the adapter kobject, so it
stays usable until kobject_del() drains kernfs.

Here is the case where a store is waiting on driver_clients_lock while
mctp_i2c_notify_del() holds it:

CPU1                                  CPU2
device_del(&adap->dev)
  bus_notify(DEL_DEVICE)
    mctp_i2c_notify_del()
      mutex_lock(&driver_clients_lock)
                                      mctp_controller_store()
                                        mutex_lock(&driver_clients_lock)
      mctp_i2c_remove_netdev()
      mutex_unlock(&driver_clients_lock)
                                        mctp_i2c_find_dev() -> NULL
                                        mctp_i2c_add_netdev()
                                          mctp_i2c_midev_init()
                                            get_device(&adap->dev)
  kobject_del(&dev->kobj)

No second DEL notification will come, so the new netdev is never torn
down. The adapter reference taken in mctp_i2c_midev_init() is never
dropped either.

Would i2c_del_adapter() then block here until the controller client is
unbound?

	device_unregister(&adap->dev);
	wait_for_completion(&adap->dev_released);

If kobject_del() has already deactivated the adapter directory,
netdev_register_kobject() fails instead. That leads into the
mctp_i2c_add_netdev() error path described below.

[Severity: High]
This isn't a bug introduced by this patch, but the new store lets
userspace reach the mctp_i2c_add_netdev() error path whenever it likes.
Can mctp_i2c_add_netdev() free midev twice when mctp_register_netdev()
fails?

mctp_i2c_ndo_uninit() is installed as ndo_uninit and calls
mctp_i2c_midev_free(). register_netdevice() calls ndo_uninit on its
err_uninit path. That path is reached when netdev_do_alloc_pcpu_stats(),
dev_index_reserve(), a NETDEV_POST_INIT notifier or
netdev_register_kobject() fails. A NETDEV_REGISTER notifier error reaches
ndo_uninit through unregister_netdevice_queue().

midev is still set when mctp_i2c_add_netdev() reaches its error label:

err:
	if (midev)
		mctp_i2c_midev_free(midev);
	if (ndev)
		free_netdev(ndev);

The second mctp_i2c_midev_free() runs list_del() on an entry that is
already poisoned. It also calls put_device() a second time on both
&midev->adapter->dev and &mcli->client->dev.

Could the extra puts release the i2c client or the adapter early and lead
to a later use-after-free?

> +	} else {
> +		mctp_i2c_remove_netdev(mcli, adap);

[Severity: Medium]
This is a pre-existing issue, but writing 0 here lets userspace remove the
selected channel's netdev at any moment. When the midev that mcli->sel
points at is freed, mctp_i2c_midev_free() does:

	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);
	}

That selects the most recently added netdev, whichever mux channel is
physically selected. mctp_i2c_slave_cb() files every received byte under
mcli->sel. Replies from the removed channel's endpoint, or the rest of a
partly received frame, would then be credited to an unrelated channel's
netdev.

Does that fit the new ABI text saying "in-flight requests fail"?

> +		rc = 0;
> +	}
> +out:
> +	mutex_unlock(&driver_clients_lock);
> +
> +	return rc < 0 ? rc : count;
> +}
> +
> +static DEVICE_ATTR_RW(mctp_controller);

[ ... ]

> +/* The root adapter is skipped: its netdev follows the bound client. The group
> + * is never removed, since kernfs drops it when the adapter is deleted, and
> + * updating rather than creating lets a rebind accept an existing group.
> + */
> +static void mctp_i2c_add_sysfs(struct i2c_adapter *adap,
> +			       struct i2c_adapter *root)
> +{
> +	int rc;
> +
> +	if (adap == root)
> +		return;
> +
> +	rc = sysfs_update_group(&adap->dev.kobj, &mctp_i2c_adapter_group);

[Severity: Critical]
What happens to these files when the module is unloaded?
MCTP_TRANSPORT_I2C is tristate. The adapter kobjects belong to the i2c
core and the mux drivers, so they outlive mctp-i2c.

Nothing in the driver calls sysfs_remove_group(). mctp_i2c_remove()
doesn't, and mctp_i2c_mod_exit() only does:

	rc = bus_unregister_notifier(&i2c_bus_type, &mctp_i2c_notifier);
	...
	i2c_del_driver(&mctp_i2c_driver);

sysfs_add_file_mode_ns() stores &dev_attr_mctp_controller.attr as the
kernfs node's private data and takes no module reference. dev_sysfs_ops
later calls dev_attr->show and dev_attr->store through that pointer.

After rmmod mctp_i2c, every child adapter would still have
/sys/bus/i2c/devices/i2c-N/mctp_controller. DEVICE_ATTR_RW gives it mode
0644, so wouldn't a read by any user load the show pointer from freed
module data and jump into freed module text?

[ ... ]

> @@ -1030,36 +1208,32 @@ static void mctp_i2c_notify_del(struct device *dev)

[ ... ]

>  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]
mcli is now on driver_clients while the bus walk runs without
driver_clients_lock. Can mctp_i2c_client_try_attach() and
mctp_i2c_notify_add() then call sysfs_update_group() on the same adapter
at the same time?

device_add() puts a new adapter on the bus klist in bus_add_device()
before it sends BUS_NOTIFY_ADD_DEVICE. i2c_register_adapter() calls
device_add() without core_lock, and i2c_for_each_dev() holds only
core_lock. So both paths can see a mux child that is registered during
probe.

In update mode, create_files() calls kernfs_remove_by_name() and then
sysfs_add_file_mode_ns(), and the pair is not atomic. Take the
interleaving A-remove, B-remove, A-add, B-add. B fails with -EEXIST, and
its error path calls remove_files(parent, grp), which deletes the file A
just created.

The adapter is then left without a mctp_controller attribute, and the only
trace is a "Failed adding mctp_controller attribute, -17" warning. Should
these group updates be serialized?

[ ... ]

> @@ -981,37 +1134,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)

[Severity: Medium]
The client is looked up again once the lock is retaken, but the netdev
is not. By this point mctp_i2c_add_sysfs() has already made
mctp_controller visible on adap, so a store can run in between:

CPU1                                  CPU2
mctp_i2c_notify_add()
  mctp_i2c_add_sysfs(adap, root)
                                      mctp_controller_store()
                                        mutex_lock(&driver_clients_lock)
                                        mctp_i2c_find_dev() -> NULL
                                        mctp_i2c_add_netdev()
  mutex_lock(&driver_clients_lock)
  mctp_i2c_find_client(root)
  mctp_i2c_add_netdev(mcli, adap)

The second mctp_i2c_add_netdev() allocates another ndev with the same
mctpi2c<nr> name and links a second midev for adap into mcli->devs.
mctp_register_netdev() then fails with -EEXIST, because that name is
already taken in mcli->net, and the error path unwinds it. The netdev
created by the store keeps working. The log still reports "register
netdev ... failed -17" and "Failed adding mctp-i2c net device" for a
bus that is fine.

mctp_i2c_client_try_attach() has the same gap between
mctp_i2c_add_sysfs() and mctp_i2c_add_netdev(), and on a rebind it is
much wider. The group is never removed, so every child adapter already
has mctp_controller, and mcli sits on driver_clients for the whole
i2c_for_each_dev() walk. A write of 1 to a DT-marked channel during
the walk creates the netdev before try_attach reaches that adapter.
The duplicate attempt then fails the same way. The return value is
discarded there, so only the dev_err() is left.

Would it be simpler to do the mctp_i2c_find_dev() check inside
mctp_i2c_add_netdev(), which already requires driver_clients_lock, and
treat an existing netdev as success? mctp_i2c_notify_add(),
mctp_i2c_client_try_attach() and mctp_controller_store() would then
all get it.

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

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

[Kenny Hsieh]

The findings look legitimate, I checked them against the source. Matt's
points change how several are best answered, so v2 reworks the attribute
lifetime rather than patching each symptom, and the v2 changelog will
say what each one turned into.

On the two pre-existing ones: the double free goes into this series,
since this patch is what makes it reachable on demand. The mcli->sel
reselection on removal is reachable today without this patch, so I am
leaving it out of this series.

pw-bot: cr

-----Original Message-----
From: netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> 
Sent: Tuesday, October 6, 2026 4:02 PM
To: Kenny Hsieh <kenny_hsieh@jabil.com>
Cc: netdev@vger.kernel.org; jk@codeconstruct.com.au; matt@codeconstruct.com.au; andi.shyti@kernel.org; peda@lysator.liu.se; andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@kernel.org; kuba@kernel.org; pabeni@redhat.com; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] mctp i2c: add sysfs control for mux child netdevs

[You don't often get email from netdev-bot+sashiko@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

EXTERNAL EMAIL: Exercise caution when handling links and attachments.





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

Critical: 1 · High: 1 (1) · Medium: 4 (1) · Low: 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).

- [Critical] Use-after-free of module text and data through the leftover
  mctp_controller sysfs file.
- [High] mctp_i2c_new_client() caches the probe-time namespace with
  write_pnet(&mcli->net, current->nsproxy->net_ns) (line 187) and never…
- [Medium] The ABI document disagrees with the code.
- [Medium] mctp_controller_store() can create a netdev for an adapter
  that is already being deleted.
- [Medium] mctp_i2c_probe() now publishes mcli on driver_clients and
  walks the bus with driver_clients_lock dropped, and…
- [Medium] net: TOCTOU race allows concurrent sysfs writes to attempt
  duplicate netdev creation
- [Low] The commit message says that for the root adapter 'a write there
  returns -EINVAL'.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new
  mctp_controller store makes it reachable on demand.
- [Medium] This problem wasn't introduced by this patch.

--

mctp i2c: add sysfs control for mux child netdevs

This patch adds a writable mctp_controller sysfs attribute to the mux child adapters below a root bus that has an mctp-i2c-controller bound.
Writing 1 or 0 to it creates or removes the MCTP netdev for that channel.
The patch also stores the probe-time network namespace in the client, allows only one client per mux tree, and drops driver_clients_lock while probe walks the bus.

> The root adapter is excluded: its netdev follows the bound 
> mctp-i2c-controller device rather than this attribute, so a write 
> there returns -EINVAL. The devicetree property keeps working, and both 
> mechanisms may be used on the same mux tree.

[Severity: Low]
Is this accurate? mctp_i2c_add_sysfs() returns early for the root adapter:

        if (adap == root)
                return;

So mctp_controller never exists on the root bus, and a write there fails with ENOENT, not EINVAL. That also seems to make the adap == root check in
mctp_controller_store() unreachable.

> Tested on an AST2700 BMC with a nested mux tree by writing 1 to 
> mctp_controller on a channel that has no "mctp-controller" property, 
> which brings up the interface, then writing 0, which removes it. The 
> attribute also survives an unbind and rebind of the 
> mctp-i2c-controller driver.

The file survives, but does the enabled state survive too?

mctp_i2c_remove() unregisters every midev, including the ones created through mctp_controller. On rebind, mctp_i2c_client_try_attach() only recreates netdevs for DT-marked adapters (or a non-OF root). Channels that userspace enabled read 0 afterwards.

Should the commit message or the ABI document say that userspace has to write 1 again after a rebind?

> 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

[ ... ]

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

[Severity: Medium]
Does the code match this description? The group is never removed (the
mctp_i2c_add_sysfs() comment says so), and mctp_i2c_remove() does not touch it. After the controller is unbound, the attribute stays on every child adapter that existed while it was bound.

Child adapters created after the unbind never get the attribute, because
mctp_i2c_notify_add() returns before it calls mctp_i2c_add_sysfs():

        if (!have_client)
                return;

So whether a channel has the attribute depends on the order things were created, not on whether a controller is bound.

The no-op rule doesn't hold either when no controller is bound.
mctp_controller_show() reports 0, but mctp_controller_store() returns -ENODEV for every value, including a write of 0.

The commit message gives this reason for the one-client-per-tree rule:
"either client's removal would delete the attribute the other still needs". The comment in mctp_i2c_new_client() says the same. With no removal path, does that reason still apply?

[ ... ]

> diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c 
> index b02476d338bbf..d4da32656744a 100644
> --- a/drivers/net/mctp/mctp-i2c.c
> +++ b/drivers/net/mctp/mctp-i2c.c

[ ... ]

> @@ -163,6 +184,7 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
>       INIT_LIST_HEAD(&mcli->devs);
>       INIT_LIST_HEAD(&mcli->list);
>       mcli->lladdr = client->addr & 0xff;
> +     write_pnet(&mcli->net, current->nsproxy->net_ns);

[Severity: High]
Should this hold a reference on the namespace? write_pnet() is a plain rcu_assign_pointer(). The driver never calls get_net() or put_net(), and
mctp_i2c_free_client() releases nothing.

Suppose the controller is probed from a non-init netns, for example by writing new_device or the driver's bind file under ip netns exec, and that netns is later deleted. The struct net is freed while mcli lives on. Any MCTP netdevs in it are moved to init_net, but mcli->net still points at the freed namespace.

Later, a write of 1 to mctp_controller or a DT-marked mux child being added goes through:

mctp_i2c_add_netdev()
    dev_net_set(ndev, read_pnet(&mcli->net));
    mctp_register_netdev()
        register_netdevice()

Can this become a use-after-free of struct net in dev_get_valid_name(),
dev_index_reserve() or list_netdevice()?

Before this patch, mctp_i2c_add_netdev() used current->nsproxy->net_ns at call time, which was always live.

[ ... ]

> @@ -968,6 +1006,121 @@ 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) {

[ ... ]

> +     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;
> +     }
> +
> +     if (enable) {
> +             if (mctp_i2c_find_dev(mcli, adap))
> +                     rc = 0; /* Already present, nothing to do */
> +             else
> +                     rc = mctp_i2c_add_netdev(mcli, adap);

[Severity: Medium]
Can this create a netdev for an adapter that is already being deleted?

device_del() sends BUS_NOTIFY_DEL_DEVICE, which runs mctp_i2c_notify_del(), well before it calls kobject_del(). The mctp_controller file was added straight onto the adapter kobject, so it stays usable until kobject_del() drains kernfs.

Here is the case where a store is waiting on driver_clients_lock while
mctp_i2c_notify_del() holds it:

CPU1                                  CPU2
device_del(&adap->dev)
  bus_notify(DEL_DEVICE)
    mctp_i2c_notify_del()
      mutex_lock(&driver_clients_lock)
                                      mctp_controller_store()
                                        mutex_lock(&driver_clients_lock)
      mctp_i2c_remove_netdev()
      mutex_unlock(&driver_clients_lock)
                                        mctp_i2c_find_dev() -> NULL
                                        mctp_i2c_add_netdev()
                                          mctp_i2c_midev_init()
                                            get_device(&adap->dev)
  kobject_del(&dev->kobj)

No second DEL notification will come, so the new netdev is never torn down. The adapter reference taken in mctp_i2c_midev_init() is never dropped either.

Would i2c_del_adapter() then block here until the controller client is unbound?

        device_unregister(&adap->dev);
        wait_for_completion(&adap->dev_released);

If kobject_del() has already deactivated the adapter directory,
netdev_register_kobject() fails instead. That leads into the
mctp_i2c_add_netdev() error path described below.

[Severity: High]
This isn't a bug introduced by this patch, but the new store lets userspace reach the mctp_i2c_add_netdev() error path whenever it likes.
Can mctp_i2c_add_netdev() free midev twice when mctp_register_netdev() fails?

mctp_i2c_ndo_uninit() is installed as ndo_uninit and calls mctp_i2c_midev_free(). register_netdevice() calls ndo_uninit on its err_uninit path. That path is reached when netdev_do_alloc_pcpu_stats(), dev_index_reserve(), a NETDEV_POST_INIT notifier or
netdev_register_kobject() fails. A NETDEV_REGISTER notifier error reaches ndo_uninit through unregister_netdevice_queue().

midev is still set when mctp_i2c_add_netdev() reaches its error label:

err:
        if (midev)
                mctp_i2c_midev_free(midev);
        if (ndev)
                free_netdev(ndev);

The second mctp_i2c_midev_free() runs list_del() on an entry that is already poisoned. It also calls put_device() a second time on both &midev->adapter->dev and &mcli->client->dev.

Could the extra puts release the i2c client or the adapter early and lead to a later use-after-free?

> +     } else {
> +             mctp_i2c_remove_netdev(mcli, adap);

[Severity: Medium]
This is a pre-existing issue, but writing 0 here lets userspace remove the selected channel's netdev at any moment. When the midev that mcli->sel points at is freed, mctp_i2c_midev_free() does:

        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);
        }

That selects the most recently added netdev, whichever mux channel is physically selected. mctp_i2c_slave_cb() files every received byte under
mcli->sel. Replies from the removed channel's endpoint, or the rest of a
partly received frame, would then be credited to an unrelated channel's netdev.

Does that fit the new ABI text saying "in-flight requests fail"?

> +             rc = 0;
> +     }
> +out:
> +     mutex_unlock(&driver_clients_lock);
> +
> +     return rc < 0 ? rc : count;
> +}
> +
> +static DEVICE_ATTR_RW(mctp_controller);

[ ... ]

> +/* The root adapter is skipped: its netdev follows the bound client. 
> +The group
> + * is never removed, since kernfs drops it when the adapter is 
> +deleted, and
> + * updating rather than creating lets a rebind accept an existing group.
> + */
> +static void mctp_i2c_add_sysfs(struct i2c_adapter *adap,
> +                            struct i2c_adapter *root) {
> +     int rc;
> +
> +     if (adap == root)
> +             return;
> +
> +     rc = sysfs_update_group(&adap->dev.kobj, 
> + &mctp_i2c_adapter_group);

[Severity: Critical]
What happens to these files when the module is unloaded?
MCTP_TRANSPORT_I2C is tristate. The adapter kobjects belong to the i2c core and the mux drivers, so they outlive mctp-i2c.

Nothing in the driver calls sysfs_remove_group(). mctp_i2c_remove() doesn't, and mctp_i2c_mod_exit() only does:

        rc = bus_unregister_notifier(&i2c_bus_type, &mctp_i2c_notifier);
        ...
        i2c_del_driver(&mctp_i2c_driver);

sysfs_add_file_mode_ns() stores &dev_attr_mctp_controller.attr as the kernfs node's private data and takes no module reference. dev_sysfs_ops later calls dev_attr->show and dev_attr->store through that pointer.

After rmmod mctp_i2c, every child adapter would still have /sys/bus/i2c/devices/i2c-N/mctp_controller. DEVICE_ATTR_RW gives it mode 0644, so wouldn't a read by any user load the show pointer from freed module data and jump into freed module text?

[ ... ]

> @@ -1030,36 +1208,32 @@ static void mctp_i2c_notify_del(struct device 
> *dev)

[ ... ]

>  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]
mcli is now on driver_clients while the bus walk runs without
driver_clients_lock. Can mctp_i2c_client_try_attach() and
mctp_i2c_notify_add() then call sysfs_update_group() on the same adapter
at the same time?

device_add() puts a new adapter on the bus klist in bus_add_device()
before it sends BUS_NOTIFY_ADD_DEVICE. i2c_register_adapter() calls
device_add() without core_lock, and i2c_for_each_dev() holds only
core_lock. So both paths can see a mux child that is registered during
probe.

In update mode, create_files() calls kernfs_remove_by_name() and then
sysfs_add_file_mode_ns(), and the pair is not atomic. Take the
interleaving A-remove, B-remove, A-add, B-add. B fails with -EEXIST, and
its error path calls remove_files(parent, grp), which deletes the file A
just created.

The adapter is then left without a mctp_controller attribute, and the only
trace is a "Failed adding mctp_controller attribute, -17" warning. Should
these group updates be serialized?

[ ... ]

> @@ -981,37 +1134,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)

[Severity: Medium]
The client is looked up again once the lock is retaken, but the netdev
is not. By this point mctp_i2c_add_sysfs() has already made
mctp_controller visible on adap, so a store can run in between:

CPU1                                  CPU2
mctp_i2c_notify_add()
  mctp_i2c_add_sysfs(adap, root)
                                      mctp_controller_store()
                                        mutex_lock(&driver_clients_lock)
                                        mctp_i2c_find_dev() -> NULL
                                        mctp_i2c_add_netdev()
  mutex_lock(&driver_clients_lock)
  mctp_i2c_find_client(root)
  mctp_i2c_add_netdev(mcli, adap)

The second mctp_i2c_add_netdev() allocates another ndev with the same
mctpi2c<nr> name and links a second midev for adap into mcli->devs.
mctp_register_netdev() then fails with -EEXIST, because that name is
already taken in mcli->net, and the error path unwinds it. The netdev
created by the store keeps working. The log still reports "register
netdev ... failed -17" and "Failed adding mctp-i2c net device" for a
bus that is fine.

mctp_i2c_client_try_attach() has the same gap between
mctp_i2c_add_sysfs() and mctp_i2c_add_netdev(), and on a rebind it is
much wider. The group is never removed, so every child adapter already
has mctp_controller, and mcli sits on driver_clients for the whole
i2c_for_each_dev() walk. A write of 1 to a DT-marked channel during
the walk creates the netdev before try_attach reaches that adapter.
The duplicate attempt then fails the same way. The return value is
discarded there, so only the dev_err() is left.

Would it be simpler to do the mctp_i2c_find_dev() check inside
mctp_i2c_add_netdev(), which already requires driver_clients_lock, and
treat an existing netdev as success? mctp_i2c_notify_add(),
mctp_i2c_client_try_attach() and mctp_controller_store() would then
all get it.

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

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

[Matt Johnston]

Hi Kenny,

Thanks for this patch. The sashiko concerns look plausible. I've added a few
other thoughts. 
(Disclosure, I suggested this sysfs approach to Kenny earlier).

On Fri, 2026-10-02 at 15:31 +0800, Kenny Hsieh wrote:
> 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.
> 
> Add a writable "mctp_controller" attribute on mux child adapters below
> a bus that has an mctp-i2c-controller device bound to it. 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.
> 
> The root adapter is excluded: its netdev follows the bound
> mctp-i2c-controller device rather than this attribute, so a write there
> returns -EINVAL. The devicetree property keeps working, and both
> mechanisms may be used on the same mux tree.

It might make sense to create the mctp_controller sysfs attribute for root
adapters too, so the root bus mctpi2c netdev can be removed at runtime. It
would probably remove some special-casing for the root node too.

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

Perhaps creating/removing a netdev should only be permitted when the current
process matches the mctp-i2c-controller network namespace. There could be
unforseen results creating a netdev in a different namespace? The restriction
could be relaxed later if needed and is deemed safe.

> Only one mctp-i2c-controller client is allowed per mux tree. A second
> one would add the same attribute to every child adapter again, and
> either client's removal would delete the attribute the other still
> needs.

This is a bit redundant given mctp-i2c-controller client will already only
bind to root adapters?

> Tested on an AST2700 BMC with a nested mux tree by writing 1 to
> mctp_controller on a channel that has no "mctp-controller" property,
> which brings up the interface, then writing 0, which removes it. The
> attribute also survives an unbind and rebind of the
> mctp-i2c-controller driver.
> 
> Signed-off-by: Kenny Hsieh <kenny_hsieh@jabil.com>
> ---
>  .../ABI/testing/sysfs-bus-i2c-devices-mctp    |  26 ++
>  MAINTAINERS                                   |   1 +
>  drivers/net/mctp/mctp-i2c.c                   | 256 +++++++++++++++---
>  3 files changed, 242 insertions(+), 41 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 000000000000..a04bc30aa7b5
> --- /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 3011f995437f..1ba32f61a21a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15718,6 +15718,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 b02476d338bb..d4da32656744 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>
>  
> @@ -89,6 +91,10 @@ struct mctp_i2c_dev {
>  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;
> @@ -128,7 +134,7 @@ static struct i2c_adapter *mux_root_adapter(struct i2c_adapter *adap)
>   */
>  static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
>  {
> -	struct mctp_i2c_client *mcli = NULL;
> +	struct mctp_i2c_client *mcli = NULL, *m = NULL;
>  	struct i2c_adapter *root = NULL;
>  	int rc;
>  
> @@ -154,6 +160,21 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
>  		goto err;
>  	}
>  
> +	/* One client per mux tree. A second one would add the same
> +	 * mctp_controller attribute to every child adapter again, and either
> +	 * client's removal would delete the attribute the other still needs.
> +	 */
> +	WARN_ON(!mutex_is_locked(&driver_clients_lock));
> +	list_for_each_entry(m, &driver_clients, list) {
> +		if (m->client->adapter == root) {
> +			dev_err(&client->dev,
> +				"An mctp-i2c-controller client is already attached to adapter %s\n",
> +				root->name);
> +			rc = -EBUSY;
> +			goto err;
> +		}
> +	}
> +
>  	mcli = kzalloc_obj(*mcli);
>  	if (!mcli) {
>  		rc = -ENOMEM;
> @@ -163,6 +184,7 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
>  	INIT_LIST_HEAD(&mcli->devs);
>  	INIT_LIST_HEAD(&mcli->list);
>  	mcli->lladdr = client->addr & 0xff;
> +	write_pnet(&mcli->net, current->nsproxy->net_ns);
>  	mcli->client = client;
>  	i2c_set_clientdata(client, mcli);
>  
> @@ -857,6 +879,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)
>  {
> @@ -883,7 +924,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);
>  
> @@ -920,18 +966,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 +1006,121 @@ 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;
> +
> +	/* The root adapter's netdev follows the mctp-i2c-controller client
> +	 * itself, so it is not ours to add or remove here.
> +	 */
> +	if (adap == root)
> +		return -EINVAL;
> +
> +	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;
> +	}
> +
> +	if (enable) {
> +		if (mctp_i2c_find_dev(mcli, adap))
> +			rc = 0;	/* Already present, nothing to do */
> +		else
> +			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,
> +};
> +
> +/* The root adapter is skipped: its netdev follows the bound client. The group
> + * is never removed, since kernfs drops it when the adapter is deleted, and
> + * updating rather than creating lets a rebind accept an existing group.

Is there a use case for the sysfs attributes to be persistent even if the the
mctp-i2c-controller client is removed and re-added? To me it would be less
surprising if they started with their initial settings when it's re-added.

> + */
> +static void mctp_i2c_add_sysfs(struct i2c_adapter *adap,
> +			       struct i2c_adapter *root)
> +{
> +	int rc;
> +
> +	if (adap == root)
> +		return;
> +
> +	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);
> +}
> +
>  /* Called for each existing i2c device (adapter or client) when a
>   * new mctp-i2c client is probed.
>   */
> @@ -981,37 +1134,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 +1208,32 @@ 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);
>  }
>  
>  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)

Cheers,
Matt

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

[Kenny Hsieh]

Hi Matt,

On Tue, 2026-10-06 at 18:31 +0800, Matt Johnston wrote:
> Perhaps creating/removing a netdev should only be permitted when the
> current process matches the mctp-i2c-controller network namespace.
> There could be unforseen results creating a netdev in a different
> namespace? The restriction could be relaxed later if needed and is
> deemed safe.

I agree with the intent, but I could not find a way to obtain the
comparison target.

The store handler receives the mux child adapter's struct device. There
is no generic mapping from a struct device to a netns: dev_net() takes a
struct net_device, and an i2c adapter is not one. Reading it from a
netdev the client already owns does not work either, since the first
create has none.

That leaves storing it, which is what v1 does, and v1 stores it without
taking a reference on the namespace. So unless the i2c core grows an
accessor for "the netns this adapter belongs to", the comparison has
nothing to compare against.

What I plan for v2 instead:

  - keep the stored namespace, but take a reference:
    write_pnet(&mcli->net, get_net(current->nsproxy->net_ns)) with a
    matching put_net() in mctp_i2c_free_client(). fs/smb/client does
    this.

  - add the permission check that every netdev sysfs store handler uses,
    ns_capable(net->user_ns, CAP_NET_ADMIN), per netdev_store() in
    net/core/net-sysfs.c. An unprivileged process in another namespace
    is then refused.

That gives the restriction you asked for without a stored-vs-current
comparison.

Thanks,
Kenny

-----Original Message-----
From: Matt Johnston <matt@codeconstruct.com.au> 
Sent: Tuesday, October 6, 2026 6:32 PM
To: Kenny Hsieh <kenny_hsieh@jabil.com>; netdev@vger.kernel.org; Jeremy Kerr <jk@codeconstruct.com.au>; Andi Shyti <andi.shyti@kernel.org>; Peter Rosin <peda@lysator.liu.se>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>; davem@davemloft.net; Eric Dumazet <edumazet@kernel.org>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] mctp i2c: add sysfs control for mux child netdevs

EXTERNAL EMAIL: Exercise caution when handling links and attachments.





Hi Kenny,

Thanks for this patch. The sashiko concerns look plausible. I've added a few other thoughts.
(Disclosure, I suggested this sysfs approach to Kenny earlier).

On Fri, 2026-10-02 at 15:31 +0800, Kenny Hsieh wrote:
> 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.
>
> Add a writable "mctp_controller" attribute on mux child adapters below 
> a bus that has an mctp-i2c-controller device bound to it. 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.
>
> The root adapter is excluded: its netdev follows the bound 
> mctp-i2c-controller device rather than this attribute, so a write 
> there returns -EINVAL. The devicetree property keeps working, and both 
> mechanisms may be used on the same mux tree.

It might make sense to create the mctp_controller sysfs attribute for root adapters too, so the root bus mctpi2c netdev can be removed at runtime. It would probably remove some special-casing for the root node too.

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

Perhaps creating/removing a netdev should only be permitted when the current process matches the mctp-i2c-controller network namespace. There could be unforseen results creating a netdev in a different namespace? The restriction could be relaxed later if needed and is deemed safe.

> Only one mctp-i2c-controller client is allowed per mux tree. A second 
> one would add the same attribute to every child adapter again, and 
> either client's removal would delete the attribute the other still 
> needs.

This is a bit redundant given mctp-i2c-controller client will already only bind to root adapters?

> Tested on an AST2700 BMC with a nested mux tree by writing 1 to 
> mctp_controller on a channel that has no "mctp-controller" property, 
> which brings up the interface, then writing 0, which removes it. The 
> attribute also survives an unbind and rebind of the 
> mctp-i2c-controller driver.
>
> Signed-off-by: Kenny Hsieh <kenny_hsieh@jabil.com>
> ---
>  .../ABI/testing/sysfs-bus-i2c-devices-mctp    |  26 ++
>  MAINTAINERS                                   |   1 +
>  drivers/net/mctp/mctp-i2c.c                   | 256 +++++++++++++++---
>  3 files changed, 242 insertions(+), 41 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 000000000000..a04bc30aa7b5
> --- /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 
> 3011f995437f..1ba32f61a21a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15718,6 +15718,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 b02476d338bb..d4da32656744 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>
>
> @@ -89,6 +91,10 @@ struct mctp_i2c_dev {  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;
> @@ -128,7 +134,7 @@ static struct i2c_adapter *mux_root_adapter(struct i2c_adapter *adap)
>   */
>  static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client 
> *client)  {
> -     struct mctp_i2c_client *mcli = NULL;
> +     struct mctp_i2c_client *mcli = NULL, *m = NULL;
>       struct i2c_adapter *root = NULL;
>       int rc;
>
> @@ -154,6 +160,21 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
>               goto err;
>       }
>
> +     /* One client per mux tree. A second one would add the same
> +      * mctp_controller attribute to every child adapter again, and either
> +      * client's removal would delete the attribute the other still needs.
> +      */
> +     WARN_ON(!mutex_is_locked(&driver_clients_lock));
> +     list_for_each_entry(m, &driver_clients, list) {
> +             if (m->client->adapter == root) {
> +                     dev_err(&client->dev,
> +                             "An mctp-i2c-controller client is already attached to adapter %s\n",
> +                             root->name);
> +                     rc = -EBUSY;
> +                     goto err;
> +             }
> +     }
> +
>       mcli = kzalloc_obj(*mcli);
>       if (!mcli) {
>               rc = -ENOMEM;
> @@ -163,6 +184,7 @@ static struct mctp_i2c_client *mctp_i2c_new_client(struct i2c_client *client)
>       INIT_LIST_HEAD(&mcli->devs);
>       INIT_LIST_HEAD(&mcli->list);
>       mcli->lladdr = client->addr & 0xff;
> +     write_pnet(&mcli->net, current->nsproxy->net_ns);
>       mcli->client = client;
>       i2c_set_clientdata(client, mcli);
>
> @@ -857,6 +879,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)  { @@ -883,7 
> +924,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);
>
> @@ -920,18 +966,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 +1006,121 @@ 
> 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;
> +
> +     /* The root adapter's netdev follows the mctp-i2c-controller client
> +      * itself, so it is not ours to add or remove here.
> +      */
> +     if (adap == root)
> +             return -EINVAL;
> +
> +     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;
> +     }
> +
> +     if (enable) {
> +             if (mctp_i2c_find_dev(mcli, adap))
> +                     rc = 0; /* Already present, nothing to do */
> +             else
> +                     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, };
> +
> +/* The root adapter is skipped: its netdev follows the bound client. 
> +The group
> + * is never removed, since kernfs drops it when the adapter is 
> +deleted, and
> + * updating rather than creating lets a rebind accept an existing group.

Is there a use case for the sysfs attributes to be persistent even if the the mctp-i2c-controller client is removed and re-added? To me it would be less surprising if they started with their initial settings when it's re-added.

> + */
> +static void mctp_i2c_add_sysfs(struct i2c_adapter *adap,
> +                            struct i2c_adapter *root) {
> +     int rc;
> +
> +     if (adap == root)
> +             return;
> +
> +     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); }
> +
>  /* Called for each existing i2c device (adapter or client) when a
>   * new mctp-i2c client is probed.
>   */
> @@ -981,37 +1134,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 +1208,32 @@ 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);
>  }
>
>  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)

Cheers,
Matt