[PATCH net v2] mctp i2c: fix double free of midev on netdev register failure

[PATCH net v2] mctp i2c: fix double free of midev on netdev register failure

[Kenny Hsieh]

mctp_i2c_add_netdev() frees midev if mctp_register_netdev() fails, but
register_netdevice() may already have freed it through ndo_uninit,
mctp_i2c_ndo_uninit(). midev is then freed twice: list_del() runs on a
poisoned entry, and the adapter and client lose a second reference.

Remove ndo_uninit and free midev only where it was set up: in the
add_netdev() error path, and in mctp_i2c_unregister() before
free_netdev().

ndo_uninit was there to drop mcli->sel's reference, which would block
unregister_netdevice() while sel points at the netdev being removed.
mctp_i2c_unregister() now does that before unregistering, keeping the
existing reselection.

Freeing after unregister exposes two more things the old order hid.
Flows released while the netdev unregisters queue the unlock marker,
which is embedded in midev and must not reach kfree_skb() through the
purge. And the bus lock, an rt_mutex taken by the tx thread, was
released by whoever called kthread_stop(); the tx thread now drops it
itself before exiting.

The double free was pointed out by the Sashiko AI review of a pending
mctp-i2c change, as a problem that predates it.

Suggested-by: Matt Johnston <matt@codeconstruct.com.au>
Fixes: f5b8abf9fc3d ("mctp i2c: MCTP I2C binding driver")
Closes: https://lore.kernel.org/netdev/179239912623.3135.11352652156626817288@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Kenny Hsieh <kenny_hsieh@jabil.com>
---
v2:
 - remove ndo_uninit instead of making the free idempotent, as suggested
   by Matt. The unlink and sel handling is factored into
   mctp_i2c_midev_detach(), which mctp_i2c_unregister() calls before
   unregistering; mctp_i2c_midev_free() still calls it for the
   add_netdev() error path, where it does nothing the second time.
 - take the unlock marker off the tx queue before purging it, and have
   the tx thread drop the bus lock itself before exiting. Both were found
   by stress testing the new order.

Tested on an AST2700 BMC with 20 iterations of removing and re-adding a
netdev, and unbinding and rebinding its controller, while requests run
on it. dmesg stays clean.

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

 drivers/net/mctp/mctp-i2c.c | 64 ++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
index b02476d33..4f4c715b4 100644
--- a/drivers/net/mctp/mctp-i2c.c
+++ b/drivers/net/mctp/mctp-i2c.c
@@ -109,7 +109,6 @@ struct mctp_i2c_hdr {
 static int mctp_i2c_recv(struct mctp_i2c_dev *midev);
 static int mctp_i2c_slave_cb(struct i2c_client *client,
 			     enum i2c_slave_event event, u8 *val);
-static void mctp_i2c_ndo_uninit(struct net_device *dev);
 static int mctp_i2c_ndo_open(struct net_device *dev);
 
 static struct i2c_adapter *mux_root_adapter(struct i2c_adapter *adap)
@@ -628,8 +627,11 @@ static int mctp_i2c_tx_thread(void *data)
 	unsigned long flags;
 
 	for (;;) {
-		if (kthread_should_stop())
+		if (kthread_should_stop()) {
+			/* Only the rt_mutex owner may release the bus lock */
+			mctp_i2c_unlock_reset(midev);
 			break;
+		}
 
 		spin_lock_irqsave(&midev->tx_queue.lock, flags);
 		skb = __skb_dequeue(&midev->tx_queue);
@@ -711,7 +713,6 @@ static void mctp_i2c_release_flow(struct mctp_dev *mdev,
 
 static const struct net_device_ops mctp_i2c_ops = {
 	.ndo_start_xmit = mctp_i2c_start_xmit,
-	.ndo_uninit = mctp_i2c_ndo_uninit,
 	.ndo_open = mctp_i2c_ndo_open,
 };
 
@@ -782,6 +783,31 @@ static struct mctp_i2c_dev *mctp_i2c_midev_init(struct net_device *dev,
 	return midev;
 }
 
+/* Removes midev from the parent i2c client, moving sel off midev.
+ * mctp_i2c_unregister() needs this before unregister, as sel's netdev
+ * reference would block it; mctp_i2c_midev_free() needs it for the failed
+ * register path. A second call on the same midev does nothing.
+ */
+static void mctp_i2c_midev_detach(struct mctp_i2c_dev *midev)
+{
+	struct mctp_i2c_client *mcli = midev->client;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mcli->sel_lock, flags);
+	if (list_empty(&midev->list)) {
+		spin_unlock_irqrestore(&mcli->sel_lock, flags);
+		return;
+	}
+	list_del_init(&midev->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);
+	}
+	spin_unlock_irqrestore(&mcli->sel_lock, flags);
+}
+
 /* Counterpart of mctp_i2c_midev_init */
 static void mctp_i2c_midev_free(struct mctp_i2c_dev *midev)
 {
@@ -797,15 +823,13 @@ static void mctp_i2c_midev_free(struct mctp_i2c_dev *midev)
 	mctp_i2c_unlock_reset(midev);
 
 	/* Remove the netdev from the parent i2c client. */
-	spin_lock_irqsave(&mcli->sel_lock, flags);
-	list_del(&midev->list);
-	if (mcli->sel == midev) {
-		struct mctp_i2c_dev *first;
+	mctp_i2c_midev_detach(midev);
 
-		first = list_first_entry_or_null(&mcli->devs, struct mctp_i2c_dev, list);
-		__mctp_i2c_device_select(mcli, first);
-	}
-	spin_unlock_irqrestore(&mcli->sel_lock, flags);
+	/* The unlock marker is embedded in midev, so keep it from kfree_skb() */
+	spin_lock_irqsave(&midev->tx_queue.lock, flags);
+	if (midev->unlock_marker.next)
+		__skb_unlink(&midev->unlock_marker, &midev->tx_queue);
+	spin_unlock_irqrestore(&midev->tx_queue.lock, flags);
 
 	skb_queue_purge(&midev->tx_queue);
 	put_device(&midev->adapter->dev);
@@ -827,23 +851,17 @@ static void mctp_i2c_unregister(struct mctp_i2c_dev *midev)
 	spin_unlock_irqrestore(&midev->lock, flags);
 	wait_for_completion(&midev->rx_done);
 
+	/* Release sel's reference on the netdev, otherwise
+	 * mctp_unregister_netdev() blocks waiting for it.
+	 */
+	mctp_i2c_midev_detach(midev);
+
 	mctp_unregister_netdev(midev->ndev);
-	/* midev has been freed now by mctp_i2c_ndo_uninit callback */
+	mctp_i2c_midev_free(midev);
 
 	free_netdev(midev->ndev);
 }
 
-static void mctp_i2c_ndo_uninit(struct net_device *dev)
-{
-	struct mctp_i2c_dev *midev = netdev_priv(dev);
-
-	/* Perform cleanup here to ensure that mcli->sel isn't holding
-	 * a reference that would prevent unregister_netdevice()
-	 * from completing.
-	 */
-	mctp_i2c_midev_free(midev);
-}
-
 static int mctp_i2c_ndo_open(struct net_device *dev)
 {
 	struct mctp_i2c_dev *midev = netdev_priv(dev);
-- 
2.50.1

Re: [PATCH net v2] mctp i2c: fix double free of midev on netdev register failure

[netdev-bot+sinfo]

Hi!

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

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

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

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

Re: [PATCH net v2] mctp i2c: fix double free of midev on netdev register failure

[Kenny Hsieh]

The double free is from code inspection. 
Sashiko flagged it while reviewing another mctp-i2c patch, and I checked it against the register_netdevice() error paths. 
The late failures need an internal allocation or notifier error, so I haven't hit it on hardware.

Re: [PATCH net v2] mctp i2c: fix double free of midev on netdev register failure

[netdev-bot+sashiko]

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

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

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] If register_netdevice() fails at the NETDEV_REGISTER notifier
  stage, mctp_i2c_add_netdev() now hangs forever.
- [Medium] Moving skb_queue_purge(&midev->tx_queue) to after
  mctp_unregister_netdev() creates a circular wait with…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  mctp_register_netdevice() (net/mctp/device.c) calls mctp_add_dev()…

--

mctp i2c: fix double free of midev on netdev register failure

This patch removes the ndo_uninit callback from mctp-i2c. midev is now
freed only in the mctp_i2c_add_netdev() error path, and in
mctp_i2c_unregister() after the netdev has been unregistered. A new helper,
mctp_i2c_midev_detach(), drops mcli->sel's netdev reference before
unregistering, and the tx thread now releases the bus lock itself.

Closes: https://lore.kernel.org/netdev/179239912623.3135.11352652156626817288@kernel.org/

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

[ ... ]

> @@ -711,7 +713,6 @@ static void mctp_i2c_release_flow(struct mctp_dev *mdev,
>  
>  static const struct net_device_ops mctp_i2c_ops = {
>  	.ndo_start_xmit = mctp_i2c_start_xmit,
> -	.ndo_uninit = mctp_i2c_ndo_uninit,
>  	.ndo_open = mctp_i2c_ndo_open,
>  };
>  

[Severity: High]
Can removing .ndo_uninit turn the double free into a permanent hang when
register_netdevice() fails at the NETDEV_REGISTER notifier stage?

When a client has no sel yet, mctp_i2c_midev_init() selects the new midev.
That takes a netdev reference before registration:

    if (!mcli->sel)
        __mctp_i2c_device_select(mcli, midev);   /* dev_hold(midev->ndev) */

If a NETDEV_REGISTER notifier then fails, register_netdevice() unwinds the
device itself:

    ret = call_netdevice_notifiers(NETDEV_REGISTER, dev);
    ...
    if (ret) {
        dev->needs_free_netdev = false;
        unregister_netdevice_queue(dev, NULL);
        goto out;
    }

After that, the rtnl_unlock() in mctp_register_netdev() waits for all
references to go away:

mctp_i2c_add_netdev()
  mctp_register_netdev()
    rtnl_lock()
    mctp_register_netdevice()
      register_netdevice()
        unregister_netdevice_queue()   <- ndo_uninit used to run here
    rtnl_unlock()
      netdev_run_todo()
        netdev_wait_allrefs_any()      <- waits on sel's dev_hold()

Without ndo_uninit, sel's reference is only dropped by
mctp_i2c_midev_free()->mctp_i2c_midev_detach() in the err: path of
mctp_i2c_add_netdev(). That path runs only after mctp_register_netdev()
returns. Does mctp_register_netdev() ever return in this case?

The stuck thread also holds driver_clients_lock. Later calls to
mctp_i2c_probe(), the remove path, and mctp_i2c_notify_add() would all
block behind it.

Possible triggers are an allocation failure in a protocol notifier (for
example inetdev_init() in the IPv4 notifier) or fault injection while the
first netdev on a client is being registered.

This is the same register failure path the commit message says it fixes.
The new comment above mctp_i2c_midev_detach() says mctp_i2c_midev_free()
covers the failed register path, but that doesn't seem to hold for this
case.

Would keeping an ndo_uninit that calls only the idempotent
mctp_i2c_midev_detach() avoid this?

[ ... ]

> @@ -782,6 +783,31 @@ static struct mctp_i2c_dev *mctp_i2c_midev_init(struct net_device *dev,
>  	return midev;
>  }
>  
> +/* Removes midev from the parent i2c client, moving sel off midev.
> + * mctp_i2c_unregister() needs this before unregister, as sel's netdev
> + * reference would block it; mctp_i2c_midev_free() needs it for the failed
> + * register path. A second call on the same midev does nothing.
> + */

[Severity: Medium]
This isn't a bug introduced by this patch, but there is a related problem
on the failed register path in the mctp core. mctp_register_netdevice()
in net/mctp/device.c sets up the mctp_dev before calling
register_netdevice():

    mdev = mctp_add_dev(dev);
    ...
    return register_netdevice(dev);

mctp_add_dev() takes a netdev reference:

    refcount_set(&mdev->refs, 1);
    dev_hold(dev);
    mdev->dev = dev;

register_netdevice() can fail before the NETDEV_REGISTER notifier, for
example:

  - dev_get_valid_name() returns -EEXIST for the fixed "mctpi2cN" name
  - a NETDEV_POST_INIT notifier fails
  - netdev_register_kobject() fails

In those cases no NETDEV_UNREGISTER is sent, so mctp_unregister() never
runs. Are the mctp_dev and its dev_hold() reference leaked here?

mctp_i2c_add_netdev() then calls free_netdev() on the
NETREG_UNINITIALIZED device. With CONFIG_NET_DEV_REFCNT_TRACKER, this
would warn in ref_tracker_dir_exit() about the leaked reference.

[ ... ]

> @@ -827,23 +851,17 @@ static void mctp_i2c_unregister(struct mctp_i2c_dev *midev)
>  	spin_unlock_irqrestore(&midev->lock, flags);
>  	wait_for_completion(&midev->rx_done);
>  
> +	/* Release sel's reference on the netdev, otherwise
> +	 * mctp_unregister_netdev() blocks waiting for it.
> +	 */
> +	mctp_i2c_midev_detach(midev);
> +
>  	mctp_unregister_netdev(midev->ndev);
> -	/* midev has been freed now by mctp_i2c_ndo_uninit callback */
> +	mctp_i2c_midev_free(midev);
>  
>  	free_netdev(midev->ndev);
>  }

[Severity: Medium]
With skb_queue_purge() now running after mctp_unregister_netdev(), can
this wait forever when CONFIG_MCTP_FLOWS is enabled?

mctp_i2c_unregister() stops the tx thread at its start. Until dev_close()
runs inside unregister_netdev(), mctp_i2c_start_xmit() can still queue
skbs to midev->tx_queue, and nothing drains them. Each queued skb's
SKB_EXT_MCTP extension holds a reference on its mctp_sk_key.

mctp_flow_prepare_output() reattaches a device whenever key->dev is NULL,
without checking key->valid:

    if (!key->dev)
        mctp_dev_set_key(dev, key);

Suppose __mctp_key_remove() removes the key between fragments in
mctp_do_fragment_route(). That can happen through an early reply from the
peer with the matching tag, or through expiry. The next fragment then
binds the mdev, and so the netdev, to a key that is no longer on any
list. That reference is dropped only by the final mctp_key_unref(), which
needs the queued skbs to be freed first.

The resulting dependency would be:

mctp_i2c_unregister()
  mctp_unregister_netdev()
    unregister_netdev()
      netdev_run_todo()
        netdev_wait_allrefs_any()        <- waits on the key's mdev ref
  mctp_i2c_midev_free()
    skb_queue_purge(&midev->tx_queue)    <- would drop the key ref

Before this patch the purge ran from ndo_uninit, after dev_close() and
before the refcount wait.

Should tx_queue be purged before the refcount wait in
mctp_unregister_netdev()?

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