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