Re: [PATCH net v3 3/3] selftests: net: add tests for saddr, anycast and loopback subnets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] selftests: net: race between UDP send and addr del in test_remove_addr_tcp_udp()
- [Medium] selftests: net: missing IPv6 FIB rule in setup_loopback_subnets()
- [Medium] selftests: net: ip_saddr IPV6_PKTINFO bypasses routing saddr check in UDPv6 tests
- [Medium] selftests: net: ip_saddr hdrincl tests never exercise FLOWI_FLAG_ANYSRC check
- [Medium] selftests: net: subshell RET loss and custom loops in wait_addr_missing()
- [Low] selftests: net: missing sockaddr cast in send_or_receive_hdrincl()
--

commit ac326b57c470225e1a2b925bc997e96e565c7e5a
Author: Íñigo Huguet <ihuguet@riseup.net>

selftests: net: add tests for saddr, anycast and loopback subnets

This commit adds networking selftests covering outgoing packet source address
validation for IPv4 and IPv6 across TCP, UDP, and raw sockets with HDRINCL,
along with tests for IPv6 anycast and loopback subnets.

> diff --git a/tools/testing/selftests/net/ip_saddr.c b/tools/testing/selftests/net/ip_saddr.c
> new file mode 100644
> index 0000000000000..ed9b0a360b281
> --- /dev/null
> +++ b/tools/testing/selftests/net/ip_saddr.c
> @@ -0,0 +1,557 @@
[ ... ]
> +static int create_sock_ipv6(int proto, int port, bool transparent)
> +{
> +	struct sockaddr_in6 address;
> +	int sk, opt;
[ ... ]
> +	opt = 1;
> +	if (setsockopt(sk, IPPROTO_IPV6, IPV6_RECVPKTINFO, &opt, sizeof(opt))) {
> +		perror("Failed to set IPV6_PKTINFO");
> +		exit(EXIT_FAILURE);
> +	}
> +	opt = 1;
> +	if (transparent &&
> +	    setsockopt(sk, IPPROTO_IPV6, IPV6_TRANSPARENT, &opt, sizeof(opt))) {
> +		perror("Failed to set IPV6_TRANSPARENT");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	memset(&address, 0, sizeof(address));
> +	address.sin6_family = AF_INET6;
> +	address.sin6_addr = in6addr_any;
> +	address.sin6_port = htons(port);
> +
> +	if (bind(sk, (struct sockaddr *)&address, sizeof(address))) {
> +		perror("Failed to bind");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	return sk;
> +}
[ ... ]
> +static void wait_and_send_udp(int sk, int sync_fd)
> +{
> +	uint8_t data_buff[256], ctrl_buff[256];
> +	ssize_t nbytes;
[ ... ]
> +	nbytes = recvmsg(sk, &msg, 0);
> +	if (nbytes < 0) {
> +		perror("Failed to receive");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	// The "test_foreign_addr_*" tests needs us to reply to the same address
> +	// but different port, and also from a different local address. The new
> +	// saddr and dport are received in the previous message with the format
> +	// "new_source_address new_dest_port"
> +	data_buff[nbytes] = '\0';
> +	maybe_chg_saddr_dport((char *)data_buff, &msg);
> +
> +	sync_wait(sync_fd);
> +
> +	// Change the data, but keep the same ancillary data so that we use the
> +	// same addresses as we received.
> +	iov.iov_base = MESSAGE;
> +	iov.iov_len = strlen(MESSAGE);
> +
> +	if (sendmsg(sk, &msg, 0) < 0) {
> +		/* Contrary to the TCP case, as UDP doesn't keep a "connection",
> +		 * the test cases where the source address is not configured in
> +		 * the host return an error here. It is expected, ignore it.
> +		 */
> +	}
> +}

[Severity: Medium]
Does passing the IPV6_PKTINFO control message to sendmsg() in
wait_and_send_udp() bypass the new routing source address check in
ip6_dst_lookup_tail() during the negative IPv6 UDP tests?

In create_sock_ipv6(), the UDP socket is bound to in6addr_any with
IPV6_RECVPKTINFO enabled, and wait_and_send_udp() passes the received
IPV6_PKTINFO control message back into sendmsg(sk, &msg, 0).

In the kernel, udpv6_sendmsg() calls ip6_datagram_send_ctl() when
msg->msg_controllen is non-zero. For a non-transparent socket
(!ipv6_can_nonlocal_bind()), ip6_datagram_send_ctl() validates
src_info->ipi6_addr via ipv6_chk_addr_and_flags() and
ipv6_chk_acast_addr_src(), returning -EINVAL immediately when the address has
been removed, expired, or replaced with FOREIGN_ADDR6.

Because udpv6_sendmsg() aborts during cmsg parsing before calling
ip6_sk_dst_lookup_flow() -> ip6_dst_lookup_tail() -> ip6_is_local_saddr(),
test_remove_addr_ipv6_udp_normal, test_expire_addr_ipv6_udp_normal, and
test_foreign_addr_ipv6_udp_normal will pass even on a kernel without the
ip6_dst_lookup_tail() change. Should the UDP socket instead bind or connect to
the source address so that fl6->saddr is populated from np->saddr and checked
in ip6_dst_lookup_tail()?

> +
> +/* -----------------------------------------------
> + * Functions for the HDRINCL tests:
> + * - Create a RAW socket with the HDRINCL sockopt
> + * - If acting as server, wait for incoming data and print it
> + * - If acting as client, craft a packet with the saddr and daddr from the args
> + *   and send it.
> + * ------------------------------------------------
> + */
> +static int create_sock_ipv4_hdrincl(void)
> +{
> +	struct sockaddr_in address;
> +	int sk, opt;
[ ... ]
> +	memset(&address, 0, sizeof(address));
> +	address.sin_family = AF_INET;
> +	address.sin_addr.s_addr = htonl(INADDR_ANY);
> +
> +	if (bind(sk, (struct sockaddr *)&address, sizeof(address))) {
> +		perror("Failed to bind");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	return sk;
> +}
> +
> +static int create_sock_ipv6_hdrincl(void)
> +{
> +	struct sockaddr_in6 address;
> +	int sk, opt;
[ ... ]
> +	memset(&address, 0, sizeof(address));
> +	address.sin6_family = AF_INET6;
> +	address.sin6_addr = in6addr_any;
> +
> +	if (bind(sk, (struct sockaddr *)&address, sizeof(address))) {
> +		perror("Failed to bind");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	return sk;
> +}

[Severity: Medium]
Do the HDRINCL raw socket tests actually exercise the FLOWI_FLAG_ANYSRC check
during route lookup?

Both create_sock_ipv4_hdrincl() and create_sock_ipv6_hdrincl() bind the raw
socket to INADDR_ANY and in6addr_any, and send_or_receive_hdrincl() calls
sendto() without a PKTINFO control message, placing local_addr only inside the
user-crafted packet buffer. In addition, test_remove_addr_hdrincl() and
test_expire_addr_hdrincl() in ip_saddr.sh delete or expire SENDER_ADDR before
starting the client ip_saddr process.

In the kernel, raw_sendmsg() and rawv6_sendmsg() perform the route lookup
(ip_route_output_flow() and ip6_dst_lookup_flow()) before raw_send_hdrinc() or
rawv6_send_hdrinc() copies the IP header from userspace. During that route
lookup, fl4.saddr and fl6.saddr are taken from inet->inet_saddr and np->saddr
(which are 0.0.0.0 and ::), so the route lookup selects the valid permanent
CLI_ADDR4 or CLI_ADDR6 configured on eth0 rather than testing an invalidated
or foreign source address against FLOWI_FLAG_ANYSRC.

Should the raw socket bind to SENDER_ADDR before the address is removed or
expired (or pass local_addr via IP_PKTINFO / IPV6_PKTINFO) so that fl4.saddr
and fl6.saddr hold the invalidated or foreign address when entering route
lookup?

[ ... ]
> +static void send_or_receive_hdrincl(int sk,
> +				    bool is_ipv6,
> +				    bool send_msg,
> +				    union addr46 *local_addr,
> +				    union addr46 *peer_addr)
> +{
> +	uint8_t buff[256];
> +	ssize_t msglen;
> +
> +	if (send_msg && is_ipv6) {
> +		struct sockaddr_in6 dest;
> +
> +		memset(&dest, 0, sizeof(dest));
> +		dest.sin6_family = AF_INET6;
> +		dest.sin6_addr = peer_addr->addr6;
> +		msglen = create_packet_ipv6(buff,
> +					    local_addr->addr6,
> +					    peer_addr->addr6);
> +		if (sendto(sk, buff, msglen, 0, &dest, sizeof(dest)) < 0) {
> +			perror("Failed to send");
> +			exit(EXIT_FAILURE);
> +		}
> +	} else if (send_msg && !is_ipv6) {
> +		struct sockaddr_in dest;
> +
> +		memset(&dest, 0, sizeof(dest));
> +		dest.sin_family = AF_INET;
> +		dest.sin_addr = peer_addr->addr4;
> +		msglen = create_packet_ipv4(buff,
> +					    local_addr->addr4,
> +					    peer_addr->addr4);
> +		if (sendto(sk, buff, msglen, 0, &dest, sizeof(dest)) < 0) {

[Severity: Low]
Will passing &dest (of type struct sockaddr_in6 * and struct sockaddr_in *)
directly to sendto() without casting to (struct sockaddr *)&dest fail to build
with -Werror=incompatible-pointer-types on non-glibc C libraries such as musl
that do not use a transparent union for struct sockaddr?

[ ... ]
> diff --git a/tools/testing/selftests/net/ip_saddr.sh b/tools/testing/selftests/net/ip_saddr.sh
> new file mode 100755
> index 0000000000000..9345b743afd0b
> --- /dev/null
> +++ b/tools/testing/selftests/net/ip_saddr.sh
> @@ -0,0 +1,575 @@
[ ... ]
> +wait_raw_sock_listen_any_addr() {
> +	local ipvx="$1"
> +	local ns="$2"
> +	local ptrn proto found i
> +
> +	if [ "$ipvx" = "ipv4" ]; then
> +		ptrn='^00000000:'
> +		proto=raw
> +	elif [ "$ipvx" = "ipv6" ]; then
> +		ptrn='^00000000000000000000000000000000:'
> +		proto=raw6
> +	else
> +		RET=$ksft_fail
> +		return 1
> +	fi
> +	for i in $(seq 10); do
> +		found="$(ip netns exec "$ns" cat "/proc/net/$proto" | \
> +			awk -vptrn="$ptrn" '$2 ~ ptrn {print "yes"}')"
> +		[ "$found" = "yes" ] && return 0
> +		sleep 0.1
> +	done
> +	RET=$ksft_fail
> +	return 1
> +}
> +
> +wait_addr_missing() {
> +	local ns="$1"
> +	local iface="$2"
> +	local addr="$3"
> +	local timeout="${4:-1}"
> +
> +	for i in $(seq $((timeout * 10))); do
> +		ip -n "$ns" -br a show dev "$iface" | grep -q "$addr" || return 0
> +		sleep 0.1
> +	done
> +	RET=$ksft_fail
> +	return 1
> +}

[Severity: Medium]
When wait_addr_missing() is called inside the command substitution and child
bash -c process in test_remove_addr_tcp_udp() and test_expire_addr_tcp_udp(),
isn't the RET=$ksft_fail assignment lost if the wait times out?

Because ksft_fail is not exported in lib.sh and wait_addr_missing() runs in a
subshell without its return status being checked, a timeout in
wait_addr_missing() will not update RET in the parent shell. In the
transparent socket tests (such as test_expire_addr_ipv4_tcp_transparent), the
subshell will still write to SRV_SYNC_FILE and receive the server's message
while the address is still present, resulting in a false pass.

Also, could wait_raw_sock_listen_any_addr() and wait_addr_missing() reuse
slowwait or busywait from tools/testing/selftests/net/lib.sh instead of open
coding the polling loops?

[ ... ]
> +# Open a TCP/UDP connection to the server, remove the address from the server
> +# and wait for a message from it. As the address is no longer present, the
> +# expectation is not to receive the message, except for transparent sockets.
> +test_remove_addr_tcp_udp() {
> +	local test_name="$1"
> +	local ipvx="$2"
> +	local proto="$3"
> +	local sock_type="$4"
> +	local srv_msg
> +
> +	setup_test_tcp_udp "$ipvx" "$proto" "$sock_type" "$SRV_SYNC_FILE"
> +	srv_msg="$(ip netns exec $ns_cli bash -c "
> +                        exec 3<>'/dev/$proto/${SENDER_ADDR%/*}/$PORT'
> +                        [ '$proto' = 'udp' ] && echo 'x' >&3
> +                        ip -n $ns_srv addr del '$SENDER_ADDR' dev eth0
> +                        wait_addr_missing '$ns_srv' eth0 '$SENDER_ADDR'
> +                        echo 'x' > '$SRV_SYNC_FILE'
> +                        timeout 2s cat <&3
> +                        exec 3<&-
> +                ")"
> +	set_test_result "$sock_type" "yes" "$srv_msg"
> +	cleanup_test
> +	log_test "$test_name"
> +}

[Severity: Medium]
Can "ip -n $ns_srv addr del '$SENDER_ADDR' dev eth0" in
test_remove_addr_tcp_udp() race with the delivery of the initial UDP datagram
to the server?

Unlike TCP, where opening /dev/tcp/... blocks until ARP or NDISC resolution
and the three-way handshake complete with wait_and_send_tcp() in ns_srv,
opening /dev/udp/... only performs a local connect() syscall, and
"echo 'x' >&3" returns as soon as the datagram is queued (including while
waiting for ARP or neighbor discovery resolution).

Furthermore, parse_tcp_udp_args() in ip_saddr.c opens SRV_SYNC_FILE with
O_RDWR at startup before calling recvmsg(), so writing to SRV_SYNC_FILE does
not wait for the server to finish recvmsg() either. If SENDER_ADDR is deleted
on ns_srv before ARP/ND resolution or UDP reception completes, the client's
initial datagram is dropped, wait_and_send_udp() remains blocked in recvmsg(),
and test_remove_addr_ipv4_udp_transparent and
test_remove_addr_ipv6_udp_transparent time out and fail (while the normal
variants pass without exercising sendmsg()).

Should wait_and_send_udp() signal the client after recvmsg() succeeds before
the client deletes SENDER_ADDR?

[ ... ]
> diff --git a/tools/testing/selftests/net/loopback_subnet.sh b/tools/testing/selftests/net/loopback_subnet.sh
> new file mode 100755
> index 0000000000000..c846e65f01f3b
> --- /dev/null
> +++ b/tools/testing/selftests/net/loopback_subnet.sh
> @@ -0,0 +1,135 @@
[ ... ]
> +setup_loopback_subnets() {
> +        local rt_table="${1:-local}"
> +
> +        ip -n $ns_srv route add local "$LO_SUBNET4" dev lo table "$rt_table"
> +        ip -n $ns_srv route add local "$LO_SUBNET6" dev lo table "$rt_table"
> +        if [[ $rt_table != local && $rt_table != 255 ]]; then
> +                ip -n $ns_srv rule add from all lookup "$rt_table"
> +        fi
> +}
> +
> +cleanup_loopback_subnets() {
> +        local rt_table="${1:-local}"
> +
> +        ip -n $ns_srv route del local "$LO_SUBNET4" dev lo table "$rt_table"
> +        ip -n $ns_srv route del local "$LO_SUBNET6" dev lo table "$rt_table"
> +        if [[ $rt_table != local && $rt_table != 255 ]]; then
> +                ip -n $ns_srv rule del from all lookup "$rt_table"
> +        fi
> +}

[Severity: Medium]
Aren't setup_loopback_subnets() and cleanup_loopback_subnets() missing "-6"
rule commands for IPv6?

In setup_loopback_subnets(), "ip -n $ns_srv route add local $LO_SUBNET6 ..."
automatically selects AF_INET6 because LO_SUBNET6 is an IPv6 prefix, but
"ip -n $ns_srv rule add from all lookup $rt_table" has no address family
indicator and defaults to IPv4 (AF_INET) only.

Without "ip -6 -n $ns_srv rule add from all lookup $rt_table" (and the
matching "ip -6 ... rule del" in cleanup_loopback_subnets()), IPv6 FIB
lookups in ns_srv never consult RT_TABLE (200), causing
test_loopback_subnet_from_localhost_ipv6_table and
test_loopback_subnet_from_remote_ipv6_table to fail due to a missing FIB rule
rather than testing loopback subnet behavior in custom tables.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028191103.623156-1-ihuguet@riseup.net?part=3