tcp: fix device binding after failed IPv6 connect

tcp_v6_connect() sets sk_bound_dev_if from sin6_scope_id when connecting to a link-local address. If the route lookup fails, the failure path leaves that device binding in place. A retry to ::1 on the same socket can then fail with ENETUNREACH, although a fresh socket can connect.

Save the original binding and restore it on synchronous errors returned by tcp_v6_connect(). Restore it before tcp_set_state(TCP_CLOSE) and inet_bhash2_reset_saddr(), since the latter uses sk_bound_dev_if to select the l3mdev for the bind2 bucket.

Only undo a binding installed from sin6_scope_id by this call, and leave a different binding installed by a later callback intact. Once the late_failure path has handled the binding, do not restore it again after the state callback and source-address cleanup.

This patch does not change disconnect handling after an asynchronous connection failure.

Found during LLM-assisted review of the tcp_v6_connect() error paths; the same assistance was used for the fix, tests and commit message. Reproduced in UML by connecting to fe80::1 through a down dummy interface, then retrying the same socket against a listener on ::1.

Tested in UML with 39 targeted userspace checks covering reconnects, existing bindings, VRF bind-bucket cleanup and BPF sock_ops binding changes. All 39 checks pass with v2, including the retry reproducer.

---
Changes in v2:
- Restore the scope binding before the TCP_CLOSE state change and inet_bhash2_reset_saddr(), as Jiayuan pointed out.
- Only restore a scope binding installed by this call, and preserve a different binding installed by a callback.
- Avoid a second restore after the late_failure cleanup.
- Follow reverse xmas tree declaration order.
- Clarify that this change handles synchronous connect failures.

v1: https://lore.kernel.org/netdev/[email protected]/

Testing on net cb284914e749:
- UML: baseline 32/39, v1 35/39, v2 39/39. The tests reproduce the stale binding on baseline, the bind2 warning on v1, and the BPF binding regression on v1. v2 passes all checks without kernel warnings. These are custom userspace tests; the kernel networking selftest suite was not run.
- x86_64, GCC 12.3.1, W=1: baseline and v2 completed vmlinux, bzImage and module builds using configurations derived from allmodconfig and allyesconfig. WERROR, KASAN and KCSAN were disabled; allyes also had KUNIT_ALL_TESTS disabled, with the resulting dependency changes. The changed object compiled without diagnostics in both configs.
- Unmodified allmodconfig and allyesconfig failed on both baseline and v2 with the same existing WERROR diagnostics in lockdep_proc.c, callthunks.c, coda/upcall.c and stackinit_kunit.c. The derived allyes builds have identical diagnostics. The v2 allmod build was incremental, so its log does not establish full-tree warning equality.

 net/ipv6/tcp_ipv6.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

index e006a30360eae..5bd4bd49d2824 100644
@@ -134,6 +134,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
 			  int addr_len)
 {
 	struct sockaddr_in6 *usin = (struct sockaddr_in6 *) uaddr;
+	int old_bound_dev_if = READ_ONCE(sk->sk_bound_dev_if);
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct inet_timewait_death_row *tcp_death_row;
 	struct ipv6_pinfo *np = tcp_inet6_sk(sk);

@@ -142,6 +143,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct net *net = sock_net(sk);
 	struct ipv6_txoptions *opt;
+	bool scope_id_set = false;
 	struct dst_entry *dst;
 	struct flowi6 *fl6;
 	int addr_type;

@@ -198,6 +200,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
 				return -EINVAL;
 
 			sk->sk_bound_dev_if = usin->sin6_scope_id;
+			scope_id_set = true;
 		}
 
 		/* Connect to link-local address requires an interface */

@@ -353,9 +356,16 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
 	return 0;
 
 late_failure:
+	if (scope_id_set &&
+	    READ_ONCE(sk->sk_bound_dev_if) == usin->sin6_scope_id)
+		WRITE_ONCE(sk->sk_bound_dev_if, old_bound_dev_if);
+	scope_id_set = false;
 	tcp_set_state(sk, TCP_CLOSE);
 	inet_bhash2_reset_saddr(sk);
 failure:
+	if (scope_id_set &&
+	    READ_ONCE(sk->sk_bound_dev_if) == usin->sin6_scope_id)
+		WRITE_ONCE(sk->sk_bound_dev_if, old_bound_dev_if);
 	inet->inet_dport = 0;
 	sk->sk_route_caps = 0;
 	return err;

base-commit: cb284914e749388eef402938bdb3115fe1f6d337

Hi,

Thanks for the review. I agree that the deferred Fast Open send failure is not covered. In that case, tcp_v6_connect() returns 0 and connect() has already returned before tcp_sendmsg_fastopen() encounters the error.

v2 restores the binding on synchronous errors returned by tcp_v6_connect(), as stated in the commit message. The later send failure is a pre-existing issue. Covering it would mean keeping the original binding beyond tcp_v6_connect() and handling any subsequent binding changes. I'd prefer to handle that separately from this fix.

The datagram binding rollback has now been merged into net/main:
https://git.kernel.org/netdev/net/c/bc53aab4881adc134f495092034e9e7c0791d84c

Agreed on WRITE_ONCE() for the scope assignment. I'll include that if another revision is needed.

Thanks,
Xia Yan

Hi Jakub,

Done in v3. It restores the old binding unconditionally on native IPv6 errors, before TCP_CLOSE and the bind2 cleanup. It avoids restoring again after the close callback.

The UML tests reproduce the stale binding and VRF warning on the baseline. With v3, the reconnect and both VRF cases pass without kernel warnings. I also checked the CONNECT/CLOSE sock_ops callbacks and IPv4-mapped failures.

Deferred TCP_FASTOPEN_CONNECT failures after connect() returns success are unchanged. I'll reproduce those separately and prepare a follow-up.

I'll send v3 as a separate thread.

Thanks,
Xia