Re: [PATCH 2/2] net: thunderbolt: Fix lockdep issue during tunnel tear down

[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 plausible.
---
    net: thunderbolt: Fix lockdep issue during tunnel tear down

This commit attempts to fix a circular locking dependency warning in
tbnet_tear_down() by dropping the connection lock earlier and returning if the
connection isn't fully established.

> diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> index 631af237998d..d8fcf18fc55c 100644
> --- a/drivers/net/thunderbolt/main.c
> +++ b/drivers/net/thunderbolt/main.c
[ ... ]
> @@ -372,48 +372,47 @@ static void tbnet_free_buffers(struct tbnet_ring *ring)
>  
>  static void tbnet_tear_down(struct tbnet *net, bool send_logout)
>  {
> +	int ret, retries = TBNET_LOGOUT_RETRIES;
> +
>  	netif_carrier_off(net->dev);
>  	netif_stop_queue(net->dev);
>  
>  	stop_login(net);
>  
>  	mutex_lock(&net->connection_lock);
> +	if (!net->login_sent || !net->login_received) {
> +		mutex_unlock(&net->connection_lock);
> +		return;
> +	}

If the connection is in a partially established state (e.g. login_sent is
true, but login_received is false), this early return skips clearing the
connection states.

Could this stall the connection if a new TBIP_LOGIN arrives later?
tbnet_handle_packet() would see net->login_sent as true and might fail to
queue the login_work to reply.

> +	net->login_sent = false;
> +	net->login_received = false;
> +	mutex_unlock(&net->connection_lock);

By releasing net->connection_lock here, the rest of the teardown process runs
without mutual exclusion. Since the network interface is still running, is it
possible for tbnet_handle_packet() to receive a new TBIP_LOGIN request right
after this lock is dropped?

If tbnet_handle_packet() acquires the lock, updates net->remote_transmit_path
and brings up a new connection concurrently:

CPU1
tbnet_tear_down()
   mutex_unlock(&net->connection_lock);

CPU2
tbnet_handle_packet()
   mutex_lock(&net->connection_lock);
   net->remote_transmit_path = pkg->transmit_path;
   mutex_unlock(&net->connection_lock);

CPU1
   tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);

Would CPU1 then erroneously release the newly established connection's hopid
and free active buffers?

>  
> -	if (net->login_sent && net->login_received) {
> -		int ret, retries = TBNET_LOGOUT_RETRIES;
[ ... ]
> -		tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
> -		net->remote_transmit_path = 0;
> -	}
> +	tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
> +	net->remote_transmit_path = 0;
>  
> -	net->login_retries = 0;

With the unconditional clearing of net->login_retries removed, does the
login_retries counter keep accumulating if tbnet_tear_down() returns early
above?

If it exceeds TBNET_LOGIN_RETRIES, could it permanently prevent new
connections until the module is reloaded?

> -	net->login_sent = false;
> -	net->login_received = false;
>  
>  	netdev_dbg(net->dev, "network traffic stopped\n");
> -
> -	mutex_unlock(&net->connection_lock);
>  }