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

[netdev-bot+sashiko@kernel.org]

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