Re: [PATCH net v2 2/2] selftests: net: exercise SO_MARK on connected sockets

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new "IPv$i $p connected mark 0" cases in
  tools/testing/selftests/net/cmsg_so_mark.sh can never fail, so they…

--

selftests: net: exercise SO_MARK on connected sockets

This adds a -k option to cmsg_sender that calls connect() before sending
and leaves msg_name unset. It also adds 18 connected-socket cases to
cmsg_so_mark.sh for IPv4 and IPv6 UDP, ping and raw sends. The
per-message mark in these cases is the socket mark, the prohibited mark,
or zero.

[ ... ]

> diff --git a/tools/testing/selftests/net/cmsg_so_mark.sh b/tools/testing/selftests/net/cmsg_so_mark.sh
> index 772ad0cc26304..7a5e317bc4c94 100755
> --- a/tools/testing/selftests/net/cmsg_so_mark.sh
> +++ b/tools/testing/selftests/net/cmsg_so_mark.sh
> @@ -68,6 +68,22 @@ for ovr in setsock cmsg both; do
>      done
>  done
>
> +# Check per-message marks after connect() has populated the route cache.
> +for i in 4 6; do
> +    [ "$i" = 4 ] && TGT=$TGT4 || TGT=$TGT6
> +
> +    for p in u i r; do
> +	for mark in "$((MARK + 1))" "$MARK" 0; do
> +	    expected=0
> +	    [ "$mark" -eq "$MARK" ] && expected=1

[Severity: Low]
Can the "connected mark 0" cases ever fail?

Here cmsg_sender runs with -k -M 1001 -m 0. connect() caches a dst that
was looked up with sk_mark 1001.

opt.mark.ena is set, so ca_write_cmsg_u32() still sends an SO_MARK cmsg
with value 0.

The only policy rule in the script matches MARK:

ip -netns $NS rule add fwmark $MARK lookup 300
ip -6 -netns $NS rule add fwmark $MARK lookup 300
ip -netns $NS route add prohibit any table 300
ip -6 -netns $NS route add prohibit any table 300

That means mark 0 and mark 1001 both go through the main table to the
dummy0 route.

On IPv6, ip6_datagram_send_ctl() sets ipc6->mark_set for any SO_MARK
cmsg. With mark_set, udpv6_sendmsg() does a fresh lookup:

    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 the zero-mark handling were missing or broken, say mark_set was only
set for non-zero marks, the send would reuse the cached main-table dst.
It would succeed either way. ping_v6_sendmsg() behaves the same.

The IPv4 and raw mark 0 cases don't use a socket dst cache on this path,
so they can't tell the two cases apart either.

The paired kernel fix ("ipv6: fix cached route reuse for SO_MARK control
messages") mentions "including an explicit zero". These cases expect 0
in both situations, so they only check connectivity.

Could a rule be added that prohibits mark 0 with a full mask, for example
fwmark 0/0xffffffff lookup 300? A plain fwmark 0 gives a mark_mask of 0
and matches everything.

With that rule, connect() with sk_mark 1001 would still succeed. The
IPv6 UDP and ping mark 0 sends would then return 1 when the cached dst
is bypassed and 0 when it is reused, so expected=1 would catch a
regression.

> +
> +	    ip netns exec "$NS" ./cmsg_sender -"$i" -p "$p" -k \
> +		-M "$((MARK + 1))" -m "$mark" -s "$TGT" 1234
> +	    check_result "$?" "$expected" "IPv$i $p connected mark $mark"
> +	done
> +    done
> +done

[ ... ]

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