Re: [net-next,v5,11/16] netkit: Add netkit notifier to check for unregistering devices

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least pausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
netkit: Add netkit notifier to check for unregistering devices

This commit adds a netdevice notifier to watch for NETDEV_UNREGISTER
events and unregister netkit devices that have leased queues from a
physical device being unregistered.

> diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c
> index 9d9d74a7e589..d4426c30e0fd 100644
> --- a/drivers/net/netkit.c
> +++ b/drivers/net/netkit.c
> @@ -1037,6 +1037,49 @@ static int netkit_change_link(struct net_device *dev, struct nlattr *tb[],
>  	return 0;
>  }
>
> +static void netkit_check_lease_unregister(struct net_device *dev)
> +{
> +	LIST_HEAD(list_kill);
> +	u32 q_idx;
> +
> +	if (READ_ONCE(dev->reg_state) != NETREG_UNREGISTERING ||
> +	    !dev->dev.parent)
> +		return;
> +
> +	for (q_idx = 0; q_idx < dev->real_num_rx_queues; q_idx++) {
> +		struct net_device *tmp = dev;
> +		u32 tmp_q_idx = q_idx;
> +
> +		netdev_lock_ops(dev);
> +		if (netif_rx_queue_lease_get_owner(&tmp, &tmp_q_idx)) {
> +			if (tmp->netdev_ops != &netkit_netdev_ops)
> +				continue;
                                ^^^^^^^^

Can this continue leak the lock acquired by netdev_lock_ops(dev) above? The
netdev_unlock_ops(dev) call is outside this if block, so continuing here
would skip the unlock and potentially deadlock on the next loop iteration
when netdev_lock_ops(dev) is called again.

> +			/* A single phys device can have multiple queues leased
> +			 * to one netkit device. We can only queue that netkit
> +			 * device once to the list_kill. Queues of that phys
> +			 * device can be leased with different individual netkit
> +			 * devices, hence we batch via list_kill.
> +			 */
> +			if (unregister_netdevice_queued(tmp))
> +				continue;
                                ^^^^^^^^

Same issue here - this continue also skips netdev_unlock_ops(dev).

> +			netkit_del_link(tmp, &list_kill);
> +		}
> +		netdev_unlock_ops(dev);
> +	}

[ ... ]