Re: [PATCH net v2 1/2] ipv6: fix cached route reuse for SO_MARK control messages

[netdev-bot+sashiko]

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

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

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).

- [High] The fix is incomplete.

--

ipv6: fix cached route reuse for SO_MARK control messages

This patch adds a mark_set flag to struct ipcm6_cookie. The flag is set
when an SO_MARK control message is supplied. When it is set,
udpv6_sendmsg() and ping_v6_sendmsg() call ip6_dst_lookup_flow()
directly instead of ip6_sk_dst_lookup_flow(), so a route cached for the
socket mark is not reused for a different per-message mark.

> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index ff7a7ffbf9833..f3f3d0254e419 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -823,6 +823,8 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
>  			err = __sock_cmsg_send(sk, cmsg, &ipc6->sockc);
>  			if (err)
>  				return err;
> +			if (cmsg->cmsg_type == SO_MARK)
> +				ipc6->mark_set = true;
>  			continue;
>  		}
>  
> diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
> index 6e90d0bf9f3da..e1676de75ae99 100644
> --- a/net/ipv6/ping.c
> +++ b/net/ipv6/ping.c
> @@ -148,7 +148,10 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  
>  	fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
>  
> -	dst = ip6_sk_dst_lookup_flow(sk, &fl6, daddr, false);
> +	if (ipc6.mark_set)
> +		dst = ip6_dst_lookup_flow(sock_net(sk), sk, &fl6, daddr);
> +	else
> +		dst = ip6_sk_dst_lookup_flow(sk, &fl6, daddr, false);
>  	if (IS_ERR(dst))
>  		return PTR_ERR(dst);
>  	rt = dst_rt6_info(dst);
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 59f9872c35521..35f7b0a47f7dd 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1693,7 +1693,10 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  
>  	fl6->flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6->flowlabel);
>  
> -	dst = ip6_sk_dst_lookup_flow(sk, fl6, final_p, connected);
> +	if (ipc6.mark_set)
> +		dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);
> +	else
> +		dst = ip6_sk_dst_lookup_flow(sk, fl6, final_p, connected);

[Severity: High]

Does this fix all reuse of a stale cached route, or only the SO_MARK
case? The commit message gives the root cause as ip6_sk_dst_lookup_flow()
consulting the socket dst cache even when connected is false. Only
mark_set bypasses the cache here, though.

ip6_sk_dst_check() validates only daddr, saddr (and only with
CONFIG_IPV6_SUBTREES) and oif:

net/ipv6/ip6_output.c:ip6_sk_dst_check() {
    ...
	if (ip6_rt_check(&rt->rt6i_dst, &fl6->daddr,
			 np->daddr_cache ? &sk->sk_v6_daddr : NULL) ||
#ifdef CONFIG_IPV6_SUBTREES
	    ip6_rt_check(&rt->rt6i_src, &fl6->saddr,
			 np->saddr_cache ? &np->saddr : NULL) ||
#endif
	   (fl6->flowi6_oif && fl6->flowi6_oif != dst_dev(dst)->ifindex)) {
    ...
}

The comment above that check says source and TOS routing are not
supported. That no longer seems accurate.

fib6_rule_match() also matches DSCP, flowlabel and sport/dport.
Several other per-message keys appear to still get the route cached at
connect() time:

  - an IPV6_TCLASS cmsg sets ipc6->tclass, which is folded into
    fl6->flowlabel just before the lookup
  - an IPV6_FLOWINFO cmsg, or sin6_flowinfo with SNDFLOW set
  - IPV6_PKTINFO with a different source address, when
    CONFIG_IPV6_SUBTREES is off
  - sendto() to the connected address with a different sin6_port

For the sendto() case, udpv6_sendmsg() points daddr at sk_v6_daddr, so
the daddr_cache check passes even though fl6_dport differs:

	if (sk->sk_state == TCP_ESTABLISHED &&
	    ipv6_addr_equal(daddr, &sk->sk_v6_daddr))
		daddr = &sk->sk_v6_daddr;

Here is one example: connect() a UDP socket, then call sendmsg() with no
msg_name and an IPV6_TCLASS cmsg of 0x10.

mark_set stays false, so
ip6_sk_dst_lookup_flow()->sk_dst_check() returns the cached dst. The
packet is sent on that route instead of hitting a dsfield prohibit rule
that would return -EACCES. ping_v6_sendmsg() behaves the same way with
an IPV6_TCLASS cmsg.

None of these paths need CAP_NET_RAW or CAP_NET_ADMIN, which the
SO_MARK cmsg does need.

The socket-level IPV6_TCLASS option already calls sk_dst_reset() in
do_ipv6_setsockopt(), so tclass is already treated as a cache key
there.

For comparison, the IPv4 udp_sendmsg() clears connected for any cmsg.
It calls sk_dst_check() only when connected is still set:

		if (err > 0) {
			err = ip_cmsg_send(sk, msg, &ipc,
					   sk->sk_family == AF_INET6);
			connected = 0;
		}
	...
	if (connected)
		rt = dst_rtable(sk_dst_check(sk, 0));

Would it be better for udpv6_sendmsg() to skip the cache whenever
connected is false, with a matching condition in ping_v6_sendmsg(),
instead of adding a flag just for SO_MARK?

>  	if (IS_ERR(dst)) {
>  		err = PTR_ERR(dst);
>  		dst = NULL;

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