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

Mika Westerberg
2026-04-29T04:47:05Z

On Tue, Apr 28, 2026 at 06:12:26PM -0700, Jakub Kicinski wrote:
> 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.

I don't think that matters because tbnet_tear_down() is actually tearing
down the connection so after this we do not expect the connection to be
established until start_login() is called again (this happens when the
interface is brought up again or on resume).

> > + 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?

Oh, this one definitely can happen.

> >
> > - 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?

Indeed, that's an accidental removal.

Okay let me try to figure out better fix for this lockdep issue.