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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Xia Yan wrote:
> A connected IPv6 UDP or ping socket can reuse a route selected for the
> socket mark when an SO_MARK control message supplies a different mark. A
> packet can then bypass a prohibit rule for its per-message mark instead
> of failing with EACCES.
> 
> ip6_sk_dst_lookup_flow() consults the socket dst cache even when
> connected is false. That argument only controls whether a new route is
> cached. The cache check does not compare the mark used for the lookup.
> 
> Record whether SO_MARK was supplied by the control messages, including
> an explicit zero. For UDP and ping sends with that flag, call
> ip6_dst_lookup_flow() directly. Keep the cache handling for sends
> without SO_MARK unchanged.

The extra cookie flag is unfortunate. Only to handle mark 0 as different
from not set?

Another approach that may avoid this and simultaneously also solves the
other inputs to policy routing:

    IPv4 udp_sendmsg() avoids all of these with one rule: if
    ip_cmsg_send() ran, connected = 0, and then sk_dst_check() is skipped
    entirely. udpv6_sendmsg() already sets connected = false in that same
    case. The only gap is that ip6_sk_dst_lookup_flow() ignores the flag
    for the cache read.

    Simpler and more complete: consult the cache only when connected, e.g.

        struct dst_entry *dst = NULL;

        if (connected) {
            dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);
            dst = ip6_sk_dst_check(sk, dst, fl6);
            if (dst)
                return dst;
        }

Would this be a viable alternative solution?

> The concurrent SO_MARK case was identified in the review of v1.
> 
> The issue was reproduced on UML with connected sockets and mark-based
> routing. A separate test kernel paused the sender after it acquired a
> cached dst reference; another thread then changed the socket mark to the
> per-message mark. The old code still sent the packet. The fix bypasses
> that cache path and returns EACCES. The 50 UDP/ping route checks and all
> 54 cmsg_so_mark.sh cases pass with the fix.

this can be skipped

> Fixes: c6af0c227a22 ("ip: support SO_MARK cmsg")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>
> ---
> v2:
>   - Replace the current sk_mark comparison with a flag recording whether
>     SO_MARK was supplied, and bypass the cache for those UDP/ping sends.
>   - Add connected-socket regression coverage in patch 2.
> 
> v1: https://lore.kernel.org/netdev/20261030141435.779302-1-yanxia@cmss.chinamobile.com/T/
> Review: https://lore.kernel.org/netdev/179371875338.488856.4546363285391899557@kernel.org/
> 
> Tested on net cb284914e749388eef402938bdb3115fe1f6d337 with GCC 12.3.1:
>   - UML normal: baseline 41/50, v1 and v2 50/50; the same extended
>     cmsg_so_mark.sh has 2/54 failures on baseline and passes on v1/v2.
>   - Test-only pause: baseline/v1 retain the old cached dst and send after
>     the socket mark changes; v2 skips the cache helper and gets EACCES.
>   - x86_64 W=1: full vmlinux, bzImage and modules builds pass on baseline
>     and v2 using configs derived from allmodconfig and allyesconfig.
>     WERROR, KASAN and KCSAN are disabled; KUNIT_ALL_TESTS is also disabled
>     for allyesconfig. The unmodified configs fail on the same existing
>     GCC 12.3.1 Werror diagnostics on both sides. Derived-build diagnostics
>     are identical on both sides, including their occurrence counts.
> 
> 
>  include/net/ipv6.h  | 1 +
>  net/ipv6/datagram.c | 2 ++
>  net/ipv6/ping.c     | 5 ++++-
>  net/ipv6/udp.c      | 5 ++++-
>  4 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -353,6 +353,7 @@
>  	__s16 tclass;
>  	__u16 gso_size;
>  	__s8  dontfrag;
> +	bool mark_set;
>  	struct ipv6_txoptions *opt;
>  };
>  
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -819,6 +819,8 @@
>  			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/udp.c b/net/ipv6/udp.c
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1693,7 +1693,10 @@
>  
>  	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);
>  	if (IS_ERR(dst)) {
>  		err = PTR_ERR(dst);
>  		dst = NULL;
> diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
> --- a/net/ipv6/ping.c
> +++ b/net/ipv6/ping.c
> @@ -148,7 +148,10 @@
>  
>  	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);
> 
>