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

[Matt Johnston <matt@codeconstruct.com.au>]

Hi Kenny,

Thanks for looking into this.

On Thu, 2026-10-22 at 11:17 +0800, Kenny Hsieh wrote:
> mctp_i2c_add_netdev() initialises midev before registering the netdev,
> and frees it on its own error path when mctp_register_netdev() fails.
> The netdev's ndo_uninit, mctp_i2c_ndo_uninit(), frees it too.
> 
> register_netdevice() calls ndo_uninit on its later failures: the
> err_uninit path and a NETDEV_REGISTER notifier error. On those, midev is
> freed twice. The second mctp_i2c_midev_free() runs list_del() on an entry
> that is already poisoned, and drops a second reference on both the
> adapter device and the client device, which can free either early.

I wonder if it would be better to delete mctp_i2c_ndo_uninit() and move the
the mctp_i2c_midev_free() to mctp_i2c_unregister(), before free_netdev().
Then the free sequence would be symmetrical with the init sequence in
mctp_i2c_add_netdev.

Prior to mctp_i2c_midev_free() calling mctp_unregister_netdev() it would need
to deselect mcli->sel if matching (the reason mctp_i2c_ndo_uninit() existed):

	/* Perform cleanup here to ensure that mcli->sel isn't holding
	 * a reference that would prevent unregister_netdevice()
	 * from completing.

Cheers,
Matt


> Earlier failures do not reach ndo_uninit. mctp_add_dev() failing,
> ethtool_check_ops() failing and a name that is already taken all return
> before it, so the error path's own free is still needed for those.
> 
> Make mctp_i2c_midev_free() safe to call twice, by clearing midev->client
> once it has run and returning early when it is already clear. Both
> callers keep working unchanged.
> 
> Tested on an AST2700 BMC by taking the netdev name before creating it,
> so the register fails early with -EEXIST. midev is freed once, and a
> later create on the same adapter succeeds.
> 
> The double free was pointed out by the Sashiko AI review of a pending
> mctp-i2c change, as a problem that predates it.
> 
> 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>
> ---
>  drivers/net/mctp/mctp-i2c.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
> index b02476d33..bca379af3 100644
> --- a/drivers/net/mctp/mctp-i2c.c
> +++ b/drivers/net/mctp/mctp-i2c.c
> @@ -788,6 +788,13 @@ static void mctp_i2c_midev_free(struct mctp_i2c_dev *midev)
>  	struct mctp_i2c_client *mcli = midev->client;
>  	unsigned long flags;
>  
> +	/* Runs from ndo_uninit and from the mctp_i2c_add_netdev() error
> +	 * path, and register_netdevice() calls ndo_uninit on some failures
> +	 * but not others. A NULL client means midev is already freed.
> +	 */
> +	if (!mcli)
> +		return;
> +
>  	if (midev->tx_thread) {
>  		kthread_stop(midev->tx_thread);
>  		midev->tx_thread = NULL;
> @@ -810,6 +817,7 @@ static void mctp_i2c_midev_free(struct mctp_i2c_dev *midev)
>  	skb_queue_purge(&midev->tx_queue);
>  	put_device(&midev->adapter->dev);
>  	put_device(&mcli->client->dev);
> +	midev->client = NULL;
>  }
>  
>  /* Stops, unregisters, and frees midev */