Re: [PATCH net-next v4 3/3] selftests: net: test SO_PRIORITY ancillary data with cmsg_sender

[Ido Schimmel <idosch@idosch.org>]

On Mon, Nov 18, 2024 at 03:51:47PM +0100, Anna Emese Nyiri wrote:
> Extend cmsg_sender.c with a new option '-Q' to send SO_PRIORITY
> ancillary data.
> 
> cmsg_so_priority.sh script added to validate SO_PRIORITY behavior 
> by creating VLAN device with egress QoS mapping and testing packet
> priorities using flower filters. Verify that packets with different
> priorities are correctly matched and counted by filters for multiple
> protocols and IP versions.
> 
> Suggested-by: Ido Schimmel <idosch@idosch.org>
> Signed-off-by: Anna Emese Nyiri <annaemesenyiri@gmail.com>
> ---
>  tools/testing/selftests/net/cmsg_sender.c     |  11 +-
>  .../testing/selftests/net/cmsg_so_priority.sh | 147 ++++++++++++++++++
>  2 files changed, 157 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/net/cmsg_so_priority.sh

Please add cmsg_so_priority.sh to tools/testing/selftests/net/Makefile
so that the test will be exercised as part of the netdev CI.

> 
> diff --git a/tools/testing/selftests/net/cmsg_sender.c b/tools/testing/selftests/net/cmsg_sender.c
> index 876c2db02a63..5267eacc35df 100644
> --- a/tools/testing/selftests/net/cmsg_sender.c
> +++ b/tools/testing/selftests/net/cmsg_sender.c
> @@ -52,6 +52,7 @@ struct options {
>  		unsigned int tclass;
>  		unsigned int hlimit;
>  		unsigned int priority;
> +		unsigned int priority_cmsg;

Why do you need this? Looks like it's unused

>  	} sockopt;
>  	struct {
>  		unsigned int family;
> @@ -59,6 +60,7 @@ struct options {
>  		unsigned int proto;
>  	} sock;
>  	struct option_cmsg_u32 mark;
> +	struct option_cmsg_u32 priority_cmsg;

To be consistent with other cmsg variables I would just name it
'priority' instead of 'priority_cmsg'

>  	struct {
>  		bool ena;
>  		unsigned int delay;
> @@ -97,6 +99,7 @@ static void __attribute__((noreturn)) cs_usage(const char *bin)
>  	       "\n"
>  	       "\t\t-m val  Set SO_MARK with given value\n"
>  	       "\t\t-M val  Set SO_MARK via setsockopt\n"

While at it, please add documentation for "-P" (SO_PRIORITY via
setsockopt). I noticed it is missing.

> +		   "\t\t-Q val  Set SO_PRIORITY via cmsg\n"

The alignment here is off.

>  	       "\t\t-d val  Set SO_TXTIME with given delay (usec)\n"
>  	       "\t\t-t      Enable time stamp reporting\n"
>  	       "\t\t-f val  Set don't fragment via cmsg\n"
> @@ -115,7 +118,7 @@ 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:")) != -1) {
> +	while ((o = getopt(argc, argv, "46sS:p:P:m:M:n:d:tf:F:c:C:l:L:H:Q:")) != -1) {
>  		switch (o) {
>  		case 's':
>  			opt.silent_send = true;
> @@ -148,6 +151,10 @@ static void cs_parse_args(int argc, char *argv[])
>  			opt.mark.ena = true;
>  			opt.mark.val = atoi(optarg);
>  			break;
> +		case 'Q':
> +			opt.priority_cmsg.ena = true;
> +			opt.priority_cmsg.val = atoi(optarg);
> +			break;
>  		case 'M':
>  			opt.sockopt.mark = atoi(optarg);
>  			break;
> @@ -252,6 +259,8 @@ cs_write_cmsg(int fd, struct msghdr *msg, char *cbuf, size_t cbuf_sz)
>  
>  	ca_write_cmsg_u32(cbuf, cbuf_sz, &cmsg_len,
>  			  SOL_SOCKET, SO_MARK, &opt.mark);
> +	ca_write_cmsg_u32(cbuf, cbuf_sz, &cmsg_len,
> +			SOL_SOCKET, SO_PRIORITY, &opt.priority_cmsg);
>  	ca_write_cmsg_u32(cbuf, cbuf_sz, &cmsg_len,
>  			  SOL_IPV6, IPV6_DONTFRAG, &opt.v6.dontfrag);
>  	ca_write_cmsg_u32(cbuf, cbuf_sz, &cmsg_len,
> diff --git a/tools/testing/selftests/net/cmsg_so_priority.sh b/tools/testing/selftests/net/cmsg_so_priority.sh
> new file mode 100755
> index 000000000000..e5919c5ed1a4
> --- /dev/null
> +++ b/tools/testing/selftests/net/cmsg_so_priority.sh
> @@ -0,0 +1,147 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +IP4=192.0.2.1/24
> +TGT4=192.0.2.2/24
> +TGT4_NO_MASK=192.0.2.2
> +TGT4_RAW=192.0.2.3/24
> +TGT4_RAW_NO_MASK=192.0.2.3
> +IP6=2001:db8::1/64
> +TGT6=2001:db8::2/64
> +TGT6_NO_MASK=2001:db8::2
> +TGT6_RAW=2001:db8::3/64
> +TGT6_RAW_NO_MASK=2001:db8::3
> +PORT=1234
> +DELAY=4000
> +
> +
> +create_filter() {
> +

Unnecessary blank line

> +    local ns=$1
> +    local dev=$2

These two are always the same so no need to parameterize them

> +    local handle=$3
> +    local vlan_prio=$4
> +    local ip_type=$5
> +    local proto=$6
> +    local dst_ip=$7
> +
> +    local cmd="tc -n $ns filter add dev $dev egress pref 1 handle $handle \
> +    proto 802.1q flower vlan_prio $vlan_prio vlan_ethtype $ip_type"
> +
> +    if [[ "$proto" == "u" ]]; then
> +        ip_proto="udp"
> +    elif [[ "$ip_type" == "ipv4" && "$proto" == "i" ]]; then
> +        ip_proto="icmp"
> +    elif [[ "$ip_type" == "ipv6" && "$proto" == "i" ]]; then
> +        ip_proto="icmpv6"
> +    fi
> +
> +    if [[ "$proto" != "r" ]]; then
> +        cmd="$cmd ip_proto $ip_proto"
> +    fi
> +
> +    cmd="$cmd dst_ip $dst_ip action pass"
> +
> +    eval $cmd
> +}
> +
> +TOTAL_TESTS=0
> +FAILED_TESTS=0
> +
> +check_result() {
> +    ((TOTAL_TESTS++))
> +    if [ "$1" -ne 0 ]; then
> +        ((FAILED_TESTS++))
> +    fi
> +}
> +
> +cleanup() {
> +    ip link del dummy1 2>/dev/null
> +    ip -n ns1 link del dummy1.10 2>/dev/null
> +    ip netns del ns1 2>/dev/null
> +}
> +
> +trap cleanup EXIT
> +
> +
> +
> +ip netns add ns1

Please use cmsg_so_mark.sh as a reference and check how it's creating
the namespace. It's done via the setup_ns() helper in lib.sh:

setup_ns NS

It will generate a random name for the namespace, allowing multiple
tests to be run in parallel without conflicts.

> +
> +ip -n ns1 link set dev lo up
> +ip -n ns1 link add name dummy1 up type dummy
> +
> +ip -n ns1 link add link dummy1 name dummy1.10 up type vlan id 10 \
> +        egress-qos-map 0:0 1:1 2:2 3:3 4:4 5:5 6:6 7:7
> +
> +ip -n ns1 address add $IP4 dev dummy1.10
> +ip -n ns1 address add $IP6 dev dummy1.10
> +
> +ip netns exec ns1 bash -c "
> +sysctl -w net.ipv4.ping_group_range='0 2147483647'
> +exit"

Can be:

ip netns exec ns1 sysctl -wq net.ipv4.ping_group_range='0 2147483647'

Note the '-q' option.

> +
> +
> +ip -n ns1 neigh add $TGT4_NO_MASK lladdr 00:11:22:33:44:55 nud permanent dev \
> +        dummy1.10
> +ip -n ns1 neigh add $TGT6_NO_MASK lladdr 00:11:22:33:44:55 nud permanent dev dummy1.10
> +ip -n ns1 neigh add $TGT4_RAW_NO_MASK lladdr 00:11:22:33:44:66 nud permanent dev dummy1.10
> +ip -n ns1 neigh add $TGT6_RAW_NO_MASK lladdr 00:11:22:33:44:66 nud permanent dev dummy1.10
> +
> +tc -n ns1 qdisc add dev dummy1 clsact
> +
> +FILTER_COUNTER=10
> +
> +for i in 4 6; do
> +    for proto in u i r; do
> +        echo "Test IPV$i, prot: $proto"
> +        for priority in {0..7}; do
> +            if [[ $i == 4 && $proto == "r" ]]; then
> +                TGT=$TGT4_RAW_NO_MASK
> +            elif [[ $i == 6 && $proto == "r" ]]; then
> +                TGT=$TGT6_RAW_NO_MASK
> +            elif [ $i == 4 ]; then
> +                TGT=$TGT4_NO_MASK
> +            else
> +                TGT=$TGT6_NO_MASK
> +            fi
> +
> +            handle="${FILTER_COUNTER}${priority}"
> +
> +            create_filter ns1 dummy1 $handle $priority ipv$i $proto $TGT
> +
> +            pkts=$(tc -n ns1 -j -s filter show dev dummy1 egress \
> +                | jq ".[] | select(.options.handle == ${handle}) | \
> +                .options.actions[0].stats.packets")
> +
> +            if [[ $pkts == 0 ]]; then
> +                check_result 0
> +            else
> +                echo "prio $priority: expected 0, got $pkts"
> +                check_result 1
> +            fi
> +
> +            ip netns exec ns1 ./cmsg_sender -$i -Q $priority -d "${DELAY}" -p $proto $TGT $PORT
> +            ip netns exec ns1 ./cmsg_sender -$i -P $priority -d "${DELAY}" -p $proto $TGT $PORT
> +
> +
> +            pkts=$(tc -n ns1 -j -s filter show dev dummy1 egress \
> +                | jq ".[] | select(.options.handle == ${handle}) | \
> +                .options.actions[0].stats.packets")
> +            if [[ $pkts == 2 ]]; then
> +                check_result 0
> +            else
> +                echo "prio $priority: expected 2, got $pkts"
> +                check_result 1
> +            fi
> +        done
> +        FILTER_COUNTER=$((FILTER_COUNTER + 10))
> +    done
> +done
> +
> +if [ $FAILED_TESTS -ne 0 ]; then
> +    echo "FAIL - $FAILED_TESTS/$TOTAL_TESTS tests failed"
> +    exit 1
> +else
> +    echo "OK - All $TOTAL_TESTS tests passed"
> +    exit 0
> +fi
> -- 
> 2.43.0
>