[PATCH net 2/2] net: thunderbolt: Mark the connection down when bringing it up fails

[Fan Ye via B4 Relay <devnull+fy15309206903.gmail.com@kernel.org>]

From: Fan Ye <fy15309206903@gmail.com>

Every failure path in tbnet_connected_work() undoes its own work and
returns, but none of them clears login_sent/login_received. The
connection therefore still looks established, and the next
tbnet_tear_down() takes its main branch and runs the whole teardown a
second time over work that was already undone:

  thunderbolt-net 0-1.0 thunderbolt0: failed to allocate Rx HopID
  thunderbolt 0000:78:00.0: RX ring 1 already stopped
  WARNING: CPU: 0 PID: 235 at drivers/thunderbolt/nhi.c:773 tb_ring_stop
   tbnet_tear_down -> tbnet_stop -> __dev_close_many
  thunderbolt 0000:78:00.0: TX ring 1 already stopped
  WARNING: CPU: 0 PID: 235 at drivers/thunderbolt/nhi.c:773 tb_ring_stop

(line 773 is the dev_WARN in tb_ring_stop() as of v6.17, which is what
this was captured on; it is line 760 in current mainline)

It stops rings that were never started, which is what the two warnings
above are, and on a kernel booted with panic_on_warn those are fatal.

It also releases net->remote_transmit_path. On the HopID mismatch path
that one was never successfully allocated by this connection - the
allocator handed out a different id precisely because the wanted one was
already taken by somebody else - so this hands back an id the connection
does not own, and it does so silently. The id is then free to be handed
out again while its owner is still using it.

Mark the connection as no longer established on those paths. Only
login_sent is cleared, which is enough for tbnet_tear_down() to leave the
already unwound state alone; login_received records that the peer has
logged in with us and carries the transmit path it gave us, and nothing
on this side can make the peer send that again.

Skipping that block skips two things that are not just a repeat of the
unwind. One is the logout request it would have sent to the peer. The
other is net->remote_transmit_path = 0 at the end of it; that field is
only read under the same login_sent && login_received guard and the
peer's next login request overwrites it, so leaving it stale is
harmless, but it is a clear that no longer happens. The rest of the
block is either already undone by the unwind that just ran or was never
done in the first place - the rings are not started and no buffers are
allocated when the HopID mismatch is hit, and the paths are not enabled
on any path that reaches err_stop_rings. The parts outside the block -
carrier off, queue stopped, login stopped, and the state reset at the
end - keep running as before.

Clearing login_sent also changes what the peer's next login request
does: tbnet_handle_packet() re-queues our login work when it sees
!login_sent, where before it would only have queued connected_work. That
is the direction I want - it gives the connection a fresh login instead
of retrying the bring-up on stale state - but it is a behaviour change
beyond keeping tbnet_tear_down() out of the way.

Measured on a link between two ASMedia ASM4242 hosts by cycling the
interface down and up 200 times from one of them over 80 minutes, and
counting what the kernel logs on both.
The mismatch is reached on its own during that, no fault injection, and
both runs were started from a cold boot with no module reloads in
between. Only the thunderbolt-net module differs between the two:

                                        without      with
    failed to allocate Rx HopID          11 / 12     9 / 13
    ring already stopped + WARNING       22 / 24     0 / 0
                                     (host A / host B)

The race still happens as often as before - it is not what this patch
addresses - but it no longer leaves a warning splat behind, and no longer
releases a HopID that belongs to someone else.

Fixes: e69b6c02b4c3 ("net: Add support for networking over Thunderbolt cable")
Cc: <stable@vger.kernel.org> # 5.13+
Signed-off-by: Fan Ye <fy15309206903@gmail.com>

---
These four came out of one investigation on a pair of ASMedia ASM4242
hosts wired to each other. Apply them in this order: the second one
touches lines the first one adds, so it needs that one underneath to
apply at all, and the last two want the first two under them for the
reason below.

  1 net: thunderbolt: Release the Rx HopID that was handed out on mismatch
  2 net: thunderbolt: Mark the connection down when bringing it up fails
  3 thunderbolt: Report DMA path teardown failures to the caller
  4 thunderbolt: Stop waiting on a path pending bit that never clears

This one is number 2 on that list.

1 and 2 fix two separate things that happen to be reached through the
same branch. Neither depends on the other for correctness - each leaves
the other's defect in place - but 2 edits the lines 1 adds, so it will
not apply on its own.

3 and 4 do want 1 and 2 underneath: the warning splat that 2 removes
fires throughout any prolonged run of link cycling, which is what 3 and 4
have to be measured across. 3 makes teardown failures visible to the
caller at all; 4 stops the teardown paying for one that cannot succeed.
Note what that pair does on this particular router - 4 leaves the first
failure to be reported and silences the rest, so 3's new signal fires
once per adapter here rather than on every teardown. 4 is the one I am
least sure of, for the reasons in its own notes.

Sits on top of "net: thunderbolt: Release the Rx HopID that was handed
out on mismatch". The two fix different things - that one releases the id
the allocator actually handed out, this one stops the teardown that
follows from running a second time - but this one edits the lines that
one adds, so it will not apply without it.

An earlier version of this also rearmed the login from the failure paths,
on the assumption that the connection would otherwise stay down. It does
not: every one of those failures was followed by the XDomain connection
being torn down and rebuilt, and the interface came back on its own. Of
the 60 occurrences across three runs, 59 had a tunnel set up again
afterwards that I could time - 42 of them 19 seconds later, 14 at 20, one
at 21 and two at 43 - with and without the patch alike. That is the
re-enumeration, not a login retry; a retry would have shown up as a
multiple of TBNET_LOGIN_DELAY. The rearm bought nothing and needed a
retry bound of its own, so it is gone.

What I am not claiming: that this makes the link survive longer. I have
run the 200 cycles four times, on four different combinations. Two of
them stopped coming up after 65 and 69 minutes and two ran to the end.
Both of the ones that finished had this patch, and neither of the ones
that died had this version of it - one had no part of it at all, the
other an earlier version. On the face of it that reads the way I would
like it to, which is exactly why I am not reading it that way: four runs,
four different combinations, one run each. What I can say is that the
race this patch is about is not what ends a run - it fired 22 times in
the run in the table above and the link came back from all 22.

One question I cannot answer from here. Skipping the block also skips the
logout request that would have gone to the peer. Everything else in it is
either already undone or was never done, but the logout is a real thing
the peer no longer hears about. I could not measure a difference - the
XDomain connection is torn down and rebuilt right afterwards in every
case I logged - but whether that is good enough, or whether the failure
paths should send one explicitly before returning, is a call I would
rather leave to you.
---
 drivers/net/thunderbolt/main.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
index e5199a87ea7a..b6aec215a8e9 100644
--- a/drivers/net/thunderbolt/main.c
+++ b/drivers/net/thunderbolt/main.c
@@ -626,6 +626,20 @@ static int tbnet_alloc_tx_buffers(struct tbnet *net)
 	return 0;
 }
 
+static void tbnet_connect_failed(struct tbnet *net)
+{
+	/*
+	 * Mark the connection as no longer established so that
+	 * tbnet_tear_down() does not run over the unwind we just did.
+	 * Only login_sent is cleared: login_received records that the peer
+	 * has logged in with us and carries the transmit path it gave us,
+	 * and nothing on this side can make the peer send that again.
+	 */
+	mutex_lock(&net->connection_lock);
+	net->login_sent = false;
+	mutex_unlock(&net->connection_lock);
+}
+
 static void tbnet_connected_work(struct work_struct *work)
 {
 	struct tbnet *net = container_of(work, typeof(*net), connected_work);
@@ -649,6 +663,7 @@ static void tbnet_connected_work(struct work_struct *work)
 		netdev_err(net->dev, "failed to allocate Rx HopID\n");
 		if (ret >= 0)
 			tb_xdomain_release_in_hopid(net->xd, ret);
+		tbnet_connect_failed(net);
 		return;
 	}
 
@@ -693,6 +708,7 @@ static void tbnet_connected_work(struct work_struct *work)
 	tb_ring_stop(net->rx_ring.ring);
 	tb_ring_stop(net->tx_ring.ring);
 	tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
+	tbnet_connect_failed(net);
 }
 
 static void tbnet_login_work(struct work_struct *work)

-- 
2.43.0