Re: [PATCH net] ipv6: fix cached route reuse for SO_MARK control messages

[Willem de Bruijn]

Xia Yan wrote:
> A connected IPv6 UDP socket can reuse its cached route when a SO_MARK
> control message supplies a different mark. The packet then takes the
> route selected for the socket mark instead of the per-message mark.
> 
> udpv6_sendmsg() sets flowi6_mark from the control message and clears
> connected. However, ip6_sk_dst_lookup_flow() still tries the cached dst;
> connected only controls whether a newly looked-up route is stored.
> ip6_sk_dst_check() checks the addresses and output interface, but not
> the mark.
> 
> Commit 42dcfd850e51 ("udp6: allow SO_MARK ctrl msg to affect routing")
> fixed the mark used for fresh route lookups; connected sockets still
> reuse the route cached before the control message.
> 
> Reject the cached dst when flowi6_mark differs from sk_mark so that
> the packet goes through a new route lookup with the requested mark.
> 
> Found during code review of IPv6 UDP route caching. Reproduced in UML
> with a connected socket and a fwmark rule pointing to a prohibit route:
> marked sends succeeded before the fix and return EACCES after it.
> A two-interface test also confirms that marked sends use the selected
> interface and subsequent sends without a control message still use the
> socket mark. cmsg_so_mark.sh passes before and after.
> 
> Fixes: c6af0c227a22 ("ip: support SO_MARK cmsg")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Xia Yan <yanxia@xxxxxxxxxxxxxxxxxxxx>

Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx>

> ---
> 
> Tested on net 5838f10563d58afca6711edb5e4eda968ca181d9. The relevant
> send and route-cache code is unchanged at the base commit.
> 
> Full x86_64 W=1 builds completed before and after with no new
> diagnostics, using configurations based on allmodconfig and
> allyesconfig. WERROR was disabled for both; allyesconfig also had
> KASAN disabled. Both used LD_ORPHAN_WARN_LEVEL="warn" and
> DRM_WERROR=n.
> 
>  net/ipv6/ip6_output.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a6..843bb95e635b9 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1125,6 +1125,7 @@ static struct dst_entry *ip6_sk_dst_check(struct sock *sk,
>  	    ip6_rt_check(&rt->rt6i_src, &fl6->saddr,
>  			 np->saddr_cache ? &np->saddr : NULL) ||
>  #endif
> +	    fl6->flowi6_mark != READ_ONCE(sk->sk_mark) ||

Asides, not expectation that you fix the following:

The same is probably also true for flowlabel and dscp.

Not net material, but ideally ipv4 and ipv6 would do the exact same
thing.

>  	   (fl6->flowi6_oif && fl6->flowi6_oif != dst_dev(dst)->ifindex)) {
>  		dst_release(dst);
>  		dst = NULL;
> 
> base-commit: d8a4f4c59da52b7c31ae337243b8473a19a3f819
> -- 
> 2.39.0.windows.2
> 
> 
>