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

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

[Xia Yan]

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@vger.kernel.org
Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>
---

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) ||
 	   (fl6->flowi6_oif && fl6->flowi6_oif != dst_dev(dst)->ifindex)) {
 		dst_release(dst);
 		dst = NULL;

base-commit: d8a4f4c59da52b7c31ae337243b8473a19a3f819
-- 
2.39.0.windows.2

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@vger.kernel.org
> Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

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

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

[Xia Yan]

Hi Willem,

Thanks for the review. I reproduced both on a 6.6-based kernel.

With a connected UDP socket and a cached route, an IPV6_TCLASS cmsg
bypasses a DSCP rule pointing to a prohibit route. Clearing the cache
makes the same send fail with EACCES. The IPv4 case returns EACCES
with or without a cached route.

Under ECMP, changing the per-message flow label keeps using the cached
nexthop. Clearing the cache selects the other nexthop for that label.
Packet captures confirmed the requested traffic class and flow label.

I'll follow up separately for net-next.

Best regards,
Xia

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1 (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 new check compares the per-message mark with the *current*
  `READ_ONCE(sk->sk_mark)`.
- [Low] The comment block in ip6_sk_dst_check()
  (net/ipv6/ip6_output.c:1105-1121) lists the validity conditions as '1.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's
  own root-cause analysis covers it: ip6_sk_dst_lookup_flow() always…
- [Low] tools/testing/selftests/net/cmsg_so_mark.sh never exercises the
  connected-socket cached-route path this patch fixes.

--

ipv6: fix cached route reuse for SO_MARK control messages

This patch makes ip6_sk_dst_check() reject the socket's cached dst when
the per-message flowi6_mark differs from sk->sk_mark. The goal is for a
connected IPv6 UDP socket that sends with an SO_MARK control message to
get a fresh route lookup with the requested mark.

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

[Severity: Low]
This is a pre-existing issue, but does cmsg_so_mark.sh ever reach the
cached-route path this patch changes?

In tools/testing/selftests/net/cmsg_sender.c, main() only creates the
socket and then calls sendmsg() to an explicit destination. It never calls
connect():

	fd = socket(ai->ai_family, opt.sock.type, opt.sock.proto);

So no dst is ever cached for the socket mark. That matches the test
passing both before and after the fix.

The script also has a dead branch. The loop only iterates over:

	for ovr in setsock cmsg both; do

so this line never takes effect:

	    [ $ovr == "diff" ] && m="-M $((MARK + 1)) -m"

Would it make sense to add a connected-socket case to the selftest along
with this fix?

> 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) ||
>  	   (fl6->flowi6_oif && fl6->flowi6_oif != dst_dev(dst)->ifindex)) {

[Severity: Medium]
Can a concurrent setsockopt(SO_MARK) still let a cmsg-marked send reuse a
route that was looked up under the old mark?

The new check compares the per-message mark with the current sk->sk_mark.
It does not compare it with the mark the cached dst was looked up under.

The uncorked udpv6_sendmsg() path runs without the socket lock.
__sock_set_mark() runs under lock_sock() and does:

	WRITE_ONCE(sk->sk_mark, val);
	sk_dst_reset(sk);

Suppose the cache holds dst_A, looked up with mark M1, and sk_mark == M1:

CPU0 (sendmsg with SO_MARK cmsg M2)
udpv6_sendmsg()
    fl6->flowi6_mark = ipc6.sockc.mark;     /* M2 */
    ip6_sk_dst_lookup_flow()
        sk_dst_check()                       /* takes ref on dst_A */

CPU1
setsockopt(SO_MARK, M2)
    __sock_set_mark()
        WRITE_ONCE(sk->sk_mark, val);        /* M2 */
        sk_dst_reset(sk);

CPU0
        ip6_sk_dst_check()
            M2 == READ_ONCE(sk->sk_mark)     /* dst_A accepted */

The packet then goes out with skb->mark M2 on the route chosen for M1.
With the prohibit-route reproducer from the commit message, that send
would succeed instead of returning EACCES.

There is a related window as well. Suppose a send without a cmsg read M1
before the setsockopt(). It fails the new check, looks up a fresh route
with M1, and stores it with ip6_sk_dst_store_flow() after the reset.

The cache then holds an M1 route while sk_mark == M2, and later sends pass
the new check. That store-after-reset race predates this patch. Still, it
means sk_mark is not a reliable record of the mark used for the cached
dst's lookup.

Would it close both windows to skip the cache when a cmsg mark is
supplied, or to store the lookup mark next to the cached dst?

[Severity: Low]
This isn't a bug, but the comment block above this if() in
ip6_sk_dst_check() still lists only two conditions:

	 * 1. ip6_rt_check(): If route was host route,
	 ...
	 * 2. oif also should be the same.

Could it be updated to mention the mark comparison? The preamble also
still says "we do not support routing by source, TOS, and MSG_DONTROUTE".

[Severity: High]
This isn't a bug introduced by this patch, but the root cause in the
commit message seems to reach beyond the mark.

ip6_sk_dst_lookup_flow() always consults the socket dst cache. The
connected flag only decides whether a new lookup is stored:

	struct dst_entry *dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);

	dst = ip6_sk_dst_check(sk, dst, fl6);
	if (dst)
		return dst;

fib6_rule_match() also routes on other per-message keys, and
ip6_sk_dst_check() still does not compare them:

	if ((r->dscp ^ ip6_dscp(fl6->flowlabel)) & r->dscp_mask)
		return 0;

	if ((r->flowlabel ^ flowi6_get_flowlabel(fl6)) & r->flowlabel_mask)
		return 0;

The destination port is matched too. With CONFIG_IPV6_SUBTREES=n, the
source address is not checked either.

For a connected socket, ip6_datagram_flow_key_init() looked up the cached
dst with ip6_make_flowinfo(np->tclass, np->flow_label) and inet_dport.
udpv6_sendmsg() can build fl6 from different values and still reuse that
dst:

  - an IPV6_TCLASS cmsg. ip6_datagram_send_ctl() accepts it with only a
    range check, and it is folded in by
    fl6->flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6->flowlabel);
  - sin6_flowinfo with IPV6_FLOWINFO_SEND
  - sendto() to the connected daddr with a different port

In these cases, do fib6 rules keyed on dsfield, flowlabel or dport ever
get consulted? That includes rules with prohibit, blackhole or
other-table actions.

ping_v6_sendmsg() looks to have the same problem with IPV6_TCLASS:

	dst = ip6_sk_dst_lookup_flow(sk, &fl6, daddr, false);

An SO_MARK cmsg requires CAP_NET_RAW or CAP_NET_ADMIN. The IPV6_TCLASS
cmsg requires no capability. setsockopt(IPV6_TCLASS) already calls
sk_dst_reset(), so tclass is treated as a routing key elsewhere.

For comparison, IPv4 udp_sendmsg() clears connected on any cmsg or
explicit address. It only consults the socket cache when connected:

	if (connected)
		rt = dst_rtable(sk_dst_check(sk, 0));

Could the IPv6 path skip the socket cache whenever per-message routing
inputs are present, as IPv4 does? That would cover all of these keys at
once instead of adding them to ip6_sk_dst_check() one at a time.

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