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

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

[Xia Yan]

A connected IPv6 UDP or ping socket can reuse its cached route when an
SO_MARK cmsg requests another mark. A packet can then bypass a prohibit
rule instead of failing with EACCES.

ip6_sk_dst_lookup_flow() reads the cache even when connected is false.
UDP clears connected after parsing IP or socket control messages. Ping
always passes false.

Only read the cache when connected is true. Ordinary connected UDP
sends keep using the cache. Ping sends do a fresh lookup.

Found during code review of IPv6 UDP route caching. On UML, the same
connected-socket selftest fails four IPv6 cases before the fix and
passes all 54 with it.

Fixes: c6af0c227a22 ("ip: support SO_MARK cmsg")
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Link: https://lore.kernel.org/netdev/willemdebruijn.kernel.1e0038bcc3ef8@gmail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>
---
v3:
  - Read the cache only when connected is true, as Willem suggested.
  - Drop the SO_MARK-specific cookie flag and UDP/ping branches.
  - Restore the discovery method in the commit message.

v2: https://lore.kernel.org/netdev/20261104092240.15649-1-yanxia@cmss.chinamobile.com/T/

Tested on net cb284914e749 with GCC 12.3.1:
  - UML route checks: 50/50.
  - x86_64 W=1: vmlinux, bzImage and modules built with allmodconfig-
    and allyesconfig-derived configs. WERROR, KASAN and KCSAN disabled;
    allyes also has KUNIT_ALL_TESTS disabled. No added diagnostics.

 net/ipv6/ip6_output.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 25fe0ba..cb648c8 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1305,7 +1305,7 @@ EXPORT_SYMBOL_GPL(ip6_dst_lookup_flow);
  *	@sk: socket which provides the dst cache and route info
  *	@fl6: flow to lookup
  *	@final_dst: final destination address for ipsec lookup
- *	@connected: whether @sk is connected or not
+ *	@connected: whether the flow can use and update the socket dst cache
  *
  *	This function performs a route lookup on the given flow with the
  *	possibility of using the cached route in the socket if it is valid.
@@ -1322,11 +1322,14 @@ struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
 					 const struct in6_addr *final_dst,
 					 bool connected)
 {
-	struct dst_entry *dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);
+	struct dst_entry *dst;
 
-	dst = ip6_sk_dst_check(sk, dst, fl6);
-	if (dst)
-		return dst;
+	if (connected) {
+		dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);
+		dst = ip6_sk_dst_check(sk, dst, fl6);
+		if (dst)
+			return dst;
+	}
 
 	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_dst);
 	if (connected && !IS_ERR(dst))

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

[Xia Yan]

cmsg_so_mark.sh sends to explicit destinations, so it does not test the
route cached by connect().

Add -k to cmsg_sender to connect first and send without msg_name.
Check connected IPv4 and IPv6 UDP, ping and raw sockets with equal,
prohibited and zero per-message marks.

Add an exact mark 0 prohibit rule before the connected cases. Connect
with an allowed socket mark, then require rejection of a mark 0 cmsg.

The same test fails four IPv6 cases before the kernel fix and passes
all 54 with it.
Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>
---
v3:
  - Add an exact mark 0 prohibit rule for the connected cases.
  - Require rejection of a mark 0 cmsg after an allowed connect().

v2: https://lore.kernel.org/netdev/20261104092252.15712-1-yanxia@cmss.chinamobile.com/T/

 tools/testing/selftests/net/cmsg_sender.c   | 21 +++++++++++++++++---
 tools/testing/selftests/net/cmsg_so_mark.sh | 22 +++++++++++++++++++++
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/cmsg_sender.c b/tools/testing/selftests/net/cmsg_sender.c
index 67a72b1..035ab54 100644
--- a/tools/testing/selftests/net/cmsg_sender.c
+++ b/tools/testing/selftests/net/cmsg_sender.c
@@ -34,6 +34,7 @@ enum {
 	ERN_CMSG_RD,
 	ERN_CMSG_RCV,
 	ERN_SEND_MORE,
+	ERN_CONNECT,
 };
 
 struct option_cmsg_u32 {
@@ -48,6 +49,7 @@ struct options {
 	unsigned int size;
 	unsigned int num_pkt;
 	bool msg_more;
+	bool connected;
 	struct {
 		unsigned int mark;
 		unsigned int dontfrag;
@@ -93,6 +95,7 @@ static void __attribute__((noreturn)) cs_usage(const char *bin)
 	printf("Usage: %s [opts] <dst host> <dst port / service>\n", bin);
 	printf("Options:\n"
 	       "\t\t-s      Silent send() failures\n"
+	       "\t\t-k      Connect before sending\n"
 	       "\t\t-S      send() size\n"
 	       "\t\t-4/-6   Force IPv4 / IPv6 only\n"
 	       "\t\t-p prot Socket protocol\n"
@@ -121,8 +124,12 @@ static void cs_parse_args(int argc, char *argv[])
 {
 	int o;
 
-	while ((o = getopt(argc, argv, "46sS:p:P:m:M:n:d:tf:F:c:C:l:L:H:Q:")) != -1) {
+	while ((o = getopt(argc, argv,
+			   "46ksS:p:P:m:M:n:d:tf:F:c:C:l:L:H:Q:")) != -1) {
 		switch (o) {
+		case 'k':
+			opt.connected = true;
+			break;
 		case 's':
 			opt.silent_send = true;
 			break;
@@ -521,6 +528,12 @@ int main(int argc, char *argv[])
 
 	ca_set_sockopts(fd);
 
+	if (opt.connected && connect(fd, ai->ai_addr, ai->ai_addrlen)) {
+		fprintf(stderr, "Can't connect socket: %s\n", strerror(errno));
+		err = ERN_CONNECT;
+		goto err_out;
+	}
+
 	if (clock_gettime(CLOCK_REALTIME, &time_start_real))
 		error(ERN_GETTIME, errno, "gettime REALTIME");
 	if (clock_gettime(CLOCK_MONOTONIC, &time_start_mono))
@@ -530,8 +543,10 @@ int main(int argc, char *argv[])
 	iov[0].iov_len = opt.size;
 
 	memset(&msg, 0, sizeof(msg));
-	msg.msg_name = ai->ai_addr;
-	msg.msg_namelen = ai->ai_addrlen;
+	if (!opt.connected) {
+		msg.msg_name = ai->ai_addr;
+		msg.msg_namelen = ai->ai_addrlen;
+	}
 	msg.msg_iov = iov;
 	msg.msg_iovlen = 1;
 
diff --git a/tools/testing/selftests/net/cmsg_so_mark.sh b/tools/testing/selftests/net/cmsg_so_mark.sh
index 772ad0c..341abfc 100644
--- a/tools/testing/selftests/net/cmsg_so_mark.sh
+++ b/tools/testing/selftests/net/cmsg_so_mark.sh
@@ -68,6 +68,28 @@ for ovr in setsock cmsg both; do
     done
 done
 
+# Reject an explicit zero mark while leaving the socket mark allowed.
+ip -netns "$NS" rule add fwmark 0/0xffffffff lookup 300 || exit 1
+ip -6 -netns "$NS" rule add fwmark 0/0xffffffff lookup 300 || exit 1
+
+# 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
+	    if [ "$mark" -eq "$MARK" ] || [ "$mark" -eq 0 ]; then
+		expected=1
+	    fi
+
+	    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
+
 # Summary
 if [ $BAD -ne 0 ]; then
     echo "FAIL - $BAD/$TOTAL cases failed"

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

[Willem de Bruijn]

Xia Yan wrote:
> A connected IPv6 UDP or ping socket can reuse its cached route when an
> SO_MARK cmsg requests another mark. A packet can then bypass a prohibit
> rule instead of failing with EACCES.
> 
> ip6_sk_dst_lookup_flow() reads the cache even when connected is false.
> UDP clears connected after parsing IP or socket control messages. Ping
> always passes false.
> 
> Only read the cache when connected is true. Ordinary connected UDP
> sends keep using the cache. Ping sends do a fresh lookup.
> 
> Found during code review of IPv6 UDP route caching. On UML, the same
> connected-socket selftest fails four IPv6 cases before the fix and
> passes all 54 with it.
> 
> Fixes: c6af0c227a22 ("ip: support SO_MARK cmsg")
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Link: https://lore.kernel.org/netdev/willemdebruijn.kernel.1e0038bcc3ef8@gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>

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

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

[Willem de Bruijn]

Xia Yan wrote:
> cmsg_so_mark.sh sends to explicit destinations, so it does not test the
> route cached by connect().
> 
> Add -k to cmsg_sender to connect first and send without msg_name.
> Check connected IPv4 and IPv6 UDP, ping and raw sockets with equal,
> prohibited and zero per-message marks.
> 
> Add an exact mark 0 prohibit rule before the connected cases. Connect
> with an allowed socket mark, then require rejection of a mark 0 cmsg.
> 
> The same test fails four IPv6 cases before the kernel fix and passes
> all 54 with it.

nit: empty line before Signed-off-by block

> Signed-off-by: Xia Yan <yanxia@cmss.chinamobile.com>

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