netdev - Re: [PATCH net 10/17] selftests: forwarding: ethtool

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] [thread-next>] [day] [month] [year] [list]

Date: Wed, 2 Aug 2023 16:30:30 +0300
From: Ido Schimmel <idosch@...sch.org>
To: Petr Machata <petrm@...dia.com>, vladimir.oltean@....com
Cc: Ido Schimmel <idosch@...dia.com>, netdev@...r.kernel.org,
	davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	edumazet@...gle.com, razor@...ckwall.org,
	mirsad.todorovac@....unizg.hr
Subject: Re: [PATCH net 10/17] selftests: forwarding: ethtool_mm: Skip when
 using veth pairs

On Wed, Aug 02, 2023 at 02:27:49PM +0200, Petr Machata wrote:
> 
> Vladimir Oltean <vladimir.oltean@....com> writes:
> 
> > Hi Ido,
> >
> > On Wed, Aug 02, 2023 at 10:51:11AM +0300, Ido Schimmel wrote:
> >> MAC Merge cannot be tested with veth pairs, resulting in failures:
> >> 
> >>  # ./ethtool_mm.sh
> >>  [...]
> >>  TEST: Manual configuration with verification: swp1 to swp2          [FAIL]
> >>          Verification did not succeed
> >> 
> >> Fix by skipping the test when used with veth pairs.
> >> 
> >> Fixes: e6991384ace5 ("selftests: forwarding: add a test for MAC Merge layer")
> >> Reported-by: Mirsad Todorovac <mirsad.todorovac@....unizg.hr>
> >> Closes: https://lore.kernel.org/netdev/adc5e40d-d040-a65e-eb26-edf47dac5b02@alu.unizg.hr/
> >> Signed-off-by: Ido Schimmel <idosch@...dia.com>
> >> Reviewed-by: Petr Machata <petrm@...dia.com>
> >> Tested-by: Mirsad Todorovac <mirsad.todorovac@....unizg.hr>
> >> ---
> >
> > That will skip the selftest just for veth pairs. This will skip it for
> > any device that doesn't support the MAC Merge layer:
> >
> > diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> > index c580ad623848..5432848a3c59 100755
> > --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> > +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> > @@ -224,6 +224,8 @@ h1_create()
> >  		hw 1
> >
> >  	ethtool --set-mm $h1 pmac-enabled on tx-enabled off verify-enabled off
> > +
> > +	h1_created=yes
> >  }
> >
> >  h2_create()
> > @@ -236,10 +238,16 @@ h2_create()
> >  		queues 1@0 1@1 1@2 1@3 \
> >  		fp P E E E \
> >  		hw 1
> > +
> > +	h2_created=yes
> >  }
> >
> >  h1_destroy()
> >  {
> > +	if ! [[ $h1_created = yes ]]; then
> > +		return
> > +	fi
> > +
> >  	ethtool --set-mm $h1 pmac-enabled off tx-enabled off verify-enabled off
> >
> >  	tc qdisc del dev $h1 root
> > @@ -249,6 +257,10 @@ h1_destroy()
> >
> >  h2_destroy()
> >  {
> > +	if ! [[ $h2_created = yes ]]; then
> > +		return
> > +	fi
> > +
> >  	tc qdisc del dev $h2 root
> >
> >  	ethtool --set-mm $h2 pmac-enabled off tx-enabled off verify-enabled off
> > @@ -266,6 +278,14 @@ setup_prepare()
> >  	h1=${NETIFS[p1]}
> >  	h2=${NETIFS[p2]}
> >
> > +	for netif in ${NETIFS[@]}; do
> > +		ethtool --show-mm $netif 2>&1 &> /dev/null
> > +		if [[ $? -ne 0 ]]; then
> > +			echo "SKIP: $netif does not support MAC Merge"
> > +			exit $ksft_skip
> > +		fi
> > +	done
> > +
> 
> Ido, if you decide to go this route, just hoist the loop to the global
> scope before registering the trap, then you don't need tho hX_created
> business.

I think the idea was to run this check after verifying that ethtool
supports MAC Merge in setup_prepare(). How about moving all these checks
before doing any configuration and registering a trap handler?

diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
index 4331e2161e8d..39e736f30322 100755
--- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
+++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
@@ -258,11 +258,6 @@ h2_destroy()
 
 setup_prepare()
 {
-       check_ethtool_mm_support
-       check_tc_fp_support
-       require_command lldptool
-       bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually"
-
        h1=${NETIFS[p1]}
        h2=${NETIFS[p2]}
 
@@ -278,7 +273,18 @@ cleanup()
        h1_destroy
 }
 
-skip_on_veth
+check_ethtool_mm_support
+check_tc_fp_support
+require_command lldptool
+bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually"
+
+for netif in ${NETIFS[@]}; do
+       ethtool --show-mm $netif 2>&1 &> /dev/null
+       if [[ $? -ne 0 ]]; then
+               echo "SKIP: $netif does not support MAC Merge"
+               exit $ksft_skip
+       fi
+done
 
 trap cleanup EXIT

> 
> >  	h1_create
> >  	h2_create
> >  }
> 
>