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

[netdev-bot+sashiko@kernel.org]

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