[PATCH net v2] tcp: fix device binding after failed IPv6 connect

[PATCH net v2] tcp: fix device binding after failed IPv6 connect

[Xia Yan]

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.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Suggested-by: Jiayuan Chen <jiayuan.chen@linux.dev>
Link: https://lore.kernel.org/netdev/062d02ca-e9e1-4f4e-ac57-0de27272a205@linux.dev/
Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>
---
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/20261031074654.73952-1-yanxia@cmss.chinamobile.com/

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(+)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index e006a30360eae..5bd4bd49d2824 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -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

Re: [PATCH net v2] tcp: fix device binding after failed IPv6 connect

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The TCP Fast Open deferred-connect path skips the new restore.

Pre-existing issues:
- [Medium] __ip6_datagram_connect() in net/ipv6/datagram.c (UDP, raw,
  ICMPv6 ping and L2TP-over-IPv6) has the same stale-binding defect, and…
- [Low] In tcp_v6_connect(), the store that installs the scope binding at
  net/ipv6/tcp_ipv6.c:202 (`sk->sk_bound_dev_if = usin->sin6_scope_id;`)…

--

tcp: fix device binding after failed IPv6 connect

This patch saves the original sk_bound_dev_if in tcp_v6_connect(). On
synchronous failure paths it restores that value, but only if the binding
was installed from sin6_scope_id by this call. On the late_failure path
the restore happens before tcp_set_state(TCP_CLOSE) and
inet_bhash2_reset_saddr(), so the bind2 cleanup sees the original l3mdev.

Link: https://lore.kernel.org/netdev/062d02ca-e9e1-4f4e-ac57-0de27272a205@linux.dev/

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index e006a30360eae..5bd4bd49d2824 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c

[ ... ]

> @@ -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;
>  			^^^^

[Severity: Low]
This isn't a bug introduced by this patch, but the store that installs the
scope binding here is a plain write. The new restores in late_failure and
failure use WRITE_ONCE().

Other code reads sk_bound_dev_if without this socket's lock. For example,
inet_bind_conflict() runs under the bhash lock:

    bound_dev_if2 = READ_ONCE(sk2->sk_bound_dev_if);

The matching store in __ip6_datagram_connect() already uses WRITE_ONCE():

    WRITE_ONCE(sk->sk_bound_dev_if, usin->sin6_scope_id);

Could this store also become WRITE_ONCE() in a respin, so that setting and
restoring the binding use the same annotation?

> +			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);

[Severity: Medium]
Does the TCP Fast Open deferred connect path still leave the scope binding
in place?

With TCP_FASTOPEN_CONNECT set and a usable cookie, tcp_v6_connect()
returns early with the binding still installed. The cookie can come from
the cache, TFO_CLIENT_NO_COOKIE or the fastopen_no_cookie route metric:

	if (tcp_fastopen_defer_connect(sk, &err))
		return err;

old_bound_dev_if and scope_id_set are stack locals, so they are lost once
this returns.

The SYN is sent later from tcp_sendmsg_fastopen(). That function has its
own copy of this failure path, and the copy does not restore
sk_bound_dev_if:

net/ipv4/tcp.c:tcp_sendmsg_fastopen() {
    ...
	if (inet_test_bit(DEFER_CONNECT, sk)) {
		err = tcp_connect(sk);
		/* Same failure procedure as in tcp_v4/6_connect */
		if (err) {
			tcp_set_state(sk, TCP_CLOSE);
			inet->inet_dport = 0;
			sk->sk_route_caps = 0;
		}
	}
    ...
}

tcp_connect() can fail here in several ways:

  - -EHOSTUNREACH from inet6_sk_rebuild_header(), which redoes the route
    lookup with flowi6_oif = sk_bound_dev_if (for example, if the
    interface went down after connect())
  - -EKEYREJECTED on a TCP-AO/MD5 key mismatch
  - -ENOBUFS or -ECONNREFUSED

__inet_stream_connect() then sees TCP_CLOSE and takes the sock_error
path, which disconnects the socket and sets SS_UNCONNECTED.
tcp_disconnect() does not touch sk_bound_dev_if.

A later connect() to ::1 on the same socket would then use the stale scope
device as flowi6_oif and fail with ENETUNREACH. This is the symptom the
patch fixes on the non-deferred path.

An unprivileged user also cannot clear the binding by hand, because
sock_bindtoindex_locked() returns -EPERM without CAP_NET_RAW.

The two "same failure procedure" copies now behave differently. Does the
original binding need to be stored somewhere that outlives
tcp_v6_connect(), so that tcp_sendmsg_fastopen() can restore it too?

> +	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);

[Severity: Medium]
This is a pre-existing issue, but __ip6_datagram_connect() in
net/ipv6/datagram.c seems to have the same stale binding problem. That
covers UDP, raw, ICMPv6 ping and L2TP over IPv6 sockets.

It installs the binding without saving the old value, and it may also set
np->mcast_oif:

			WRITE_ONCE(sk->sk_bound_dev_if, usin->sin6_scope_id);

If the route lookup then fails, the error branch restores the peer
address, flow label and port, but not the binding:

	err = ip6_datagram_dst_update(sk, true);
	if (err) {
		/* Restore the socket peer info, to keep it consistent with
		 * the old socket state
		 */
		sk->sk_v6_daddr = old_daddr;
		np->flow_label = old_fl6_flowlabel;
		inet->inet_dport = old_dport;
		goto out;
	}

ip6_datagram_flow_key_init() later uses sk_bound_dev_if as flowi6_oif.
A following connect() to ::1 skips the scope branch, so it would fail the
same way as the TCP reproducer. Only connect(AF_UNSPEC), through
__udp_disconnect(), clears the binding.

Would a similar fix make sense for the datagram connect path?

>  	inet->inet_dport = 0;
>  	sk->sk_route_caps = 0;
>  	return err;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102143352.135769-1-yanxia%40cmss.chinamobile.com

Re: [PATCH net v2] tcp: fix device binding after failed IPv6 connect

[Xia Yan]

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