netdev - Re: [PATCH net v3 2/2] selftests: add selftest for chaining of tc ingress handling to egress

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4c28ed5e-d669-1f27-fa8f-b4f1e651c013@nvidia.com>
Date:   Wed, 12 Oct 2022 11:12:05 +0300
From:   Paul Blakey <paulb@...dia.com>
To:     Paolo Abeni <pabeni@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Vlad Buslov <vladbu@...dia.com>, Oz Shlomo <ozsh@...dia.com>,
        Roi Dayan <roid@...dia.com>, netdev@...r.kernel.org,
        Saeed Mahameed <saeedm@...dia.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net v3 2/2] selftests: add selftest for chaining of tc
 ingress handling to egress



On 04/10/2022 12:36, Paolo Abeni wrote:
> Hello,
> 
> On Sun, 2022-10-02 at 13:24 +0300, Paul Blakey wrote:
>> This test runs a simple ingress tc setup between two veth pairs,
>> then adds a egress->ingress rule to test the chaining of tc ingress
>> pipeline to tc egress piepline.
>>
>> Signed-off-by: Paul Blakey <paulb@...dia.com>
>> ---
>>   .../net/test_ingress_egress_chaining.sh       | 81 +++++++++++++++++++
>>   1 file changed, 81 insertions(+)
>>   create mode 100644 tools/testing/selftests/net/test_ingress_egress_chaining.sh
>>
>> diff --git a/tools/testing/selftests/net/test_ingress_egress_chaining.sh b/tools/testing/selftests/net/test_ingress_egress_chaining.sh
>> new file mode 100644
>> index 000000000000..4775f5657e68
>> --- /dev/null
>> +++ b/tools/testing/selftests/net/test_ingress_egress_chaining.sh
>> @@ -0,0 +1,81 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +# This test runs a simple ingress tc setup between two veth pairs,
>> +# and chains a single egress rule to test ingress chaining to egress.
>> +#
>> +# Kselftest framework requirement - SKIP code is 4.
>> +ksft_skip=4
>> +
>> +if [ "$(id -u)" -ne 0 ];then
>> +	echo "SKIP: Need root privileges"
>> +	exit $ksft_skip
>> +fi
>> +
>> +if [ ! -x "$(command -v iperf)" ]; then
>> +	echo "SKIP: Could not run test without iperf tool"
> 
> You just need to establish a TCP connection towards a given IP, right?
> 
> Than you can use the existing self-tests program:
> 
> # listener:
> ./udpgso_bench_rx -t &
> 
> # client:
> ./udpgso_bench_tx -t -l <transfer time> -4  -D <listener IP>
> 
> and avoid dependencies on external tools.
> 
>> +	exit $ksft_skip
>> +fi
>> +
>> +needed_mods="act_mirred cls_flower sch_ingress"
>> +for mod in $needed_mods; do
>> +	modinfo $mod &>/dev/null || { echo "SKIP: Need act_mirred module"; exit $ksft_skip; }
>> +done
>> +
>> +ns="ns$((RANDOM%899+100))"
>> +veth1="veth1$((RANDOM%899+100))"
>> +veth2="veth2$((RANDOM%899+100))"
>> +peer1="peer1$((RANDOM%899+100))"
>> +peer2="peer2$((RANDOM%899+100))"
>> +
>> +function fail() {
>> +	echo "FAIL: $@" >> /dev/stderr
>> +	exit 1
>> +}
>> +
>> +function cleanup() {
>> +	killall -q -9 iperf
>> +	ip link del $veth1 &> /dev/null
>> +	ip link del $veth2 &> /dev/null
>> +	ip netns del $ns &> /dev/null
>> +}
>> +trap cleanup EXIT
>> +
>> +function config() {
>> +	echo "Setup veth pairs [$veth1, $peer1], and veth pair [$veth2, $peer2]"
>> +	ip link add $veth1 type veth peer name $peer1
>> +	ip link add $veth2 type veth peer name $peer2
>> +	ifconfig $peer1 5.5.5.6/24 up
> 
> Please use the modern 'ip addr' syntax. More importantly, it's better
> if you move both peers in separate netns, to avoid 'random' self-test
> failure due to the specific local routing configuration.

I have one of the peers outside a namespace because I needed the egress 
tc rule to see both devices.

Besides that I will change as requested, Thanks.

> 
> Additionally you could pick addresses from tests blocks (192.0.2.0/24,
> 198.51.100.0/24, 203.0.113.0/24) or at least from private ranges.
> 
>> +	ip netns add $ns
>> +	ip link set dev $peer2 netns $ns
>> +	ip netns exec $ns ifconfig $peer2 5.5.5.5/24 up
>> +	ifconfig $veth2 0 up
>> +	ifconfig $veth1 0 up
> 
> Please use 'ip link' ...
> 
>> +
>> +	echo "Add tc filter ingress->egress forwarding $veth1 <-> $veth2"
>> +	tc qdisc add dev $veth2 ingress
>> +	tc qdisc add dev $veth1 ingress
>> +	tc filter add dev $veth2 ingress prio 1 proto all flower \
>> +		action mirred egress redirect dev $veth1
>> +	tc filter add dev $veth1 ingress prio 1 proto all flower \
>> +		action mirred egress redirect dev $veth2
>> +
>> +	echo "Add tc filter egress->ingress forwarding $peer1 -> $veth1, bypassing the veth pipe"
>> +	tc qdisc add dev $peer1 clsact
>> +	tc filter add dev $peer1 egress prio 20 proto ip flower \
>> +		action mirred ingress redirect dev $veth1
>> +}
>> +
>> +function test_run() {
>> +	echo "Run iperf"
>> +	iperf -s -D
> 
> Depending on the timing, the server can create the listener socket
> after that the client tried to connect, causing random failures.
> 
> You should introduce some explicit, small, delay to give the server the
> time to start-up, e.g.:
> 
> # start server
> sleep 0.2
> # start client
> 
> 
> Thanks!
> 
> Paolo
>