netdev - Re: [PATCH iproute2 4/4] testsuite: remove gre kmods if the test loads them

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:   Sun, 16 Dec 2018 13:52:29 +0000
From:   Luca Boccassi <bluca@...ian.org>
To:     Petr Vorel <pvorel@...e.cz>
Cc:     netdev@...r.kernel.org, stephen@...workplumber.org,
        Petr Vorel <petr.vorel@...il.com>
Subject: Re: [PATCH iproute2 4/4] testsuite: remove gre kmods if the test
 loads them

On Sat, 2018-12-15 at 18:37 +0100, Petr Vorel wrote:
> Hi Luca,
> 
> > The tunnel test leaves behind link devices created by the GRE
> > kernel
> > modules:
> > $ ip -br link
> > ...
> > gre0@...E    DOWN 0.0.0.0 <NOARP>
> > gretap0@...E DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
> > erspan0@...E DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
> > ip6tnl0@...E DOWN :: <NOARP>
> > ip6gre0@...E DOWN 00:00:00:00:
> > $ lsmod | grep gre
> > ip6_gre      40960  0
> > ip6_tunnel   40960  1 ip6_gre
> > ip_gre       32768  0
> > ip_tunnel    24576  1 ip_gre
> > gre          16384  2 ip6_gre,ip_gre
> > Check beforehand if the gre kernel module is loaded, and if not
> > unload
> > them all at the end of the test. This should avoid causing problems
> > if
> > a user is already using GRE for other purposes.
> > Signed-off-by: Luca Boccassi <bluca@...ian.org>
> 
> Reviewed-by: Petr Vorel <pvorel@...e.cz>
> 
> > ---
> >  testsuite/tests/ip/tunnel/add_tunnel.t | 24
> > ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > diff --git a/testsuite/tests/ip/tunnel/add_tunnel.t
> > b/testsuite/tests/ip/tunnel/add_tunnel.t
> > index 3f5a9d3c..76f8b011 100755
> > --- a/testsuite/tests/ip/tunnel/add_tunnel.t
> > +++ b/testsuite/tests/ip/tunnel/add_tunnel.t
> > @@ -4,6 +4,15 @@
> >  TUNNEL_NAME="tunnel_test_ip"
> > +# note that checkbashism reports command -v, but dash supports it
> > and it's posix compliant
> 
> NOTE: also 'busybox sh' and mksh (used also in older Android) support
> it.
> BTW: yes, it's not optional in POSIX 2008/2013, it's optional in 2004
> [1].
> But I'd put this info only into commit message, most people probably
> does not
> care.

I left it as a comment as I know some developers are running
checkbashism on the scripts in this repo, so it might save them some
time in the future.

> > +if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null
> > 2>&1
> > +then
> > +    KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"
> > +    COUNT_KMODS_BEFORE=$(lsmod | grep -c -e "^ip6_gre" -e
> > "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre")
> > +else
> > +    KMODS=""
> > +fi
> 
> ...
> > +if [ -n "$KMODS" ]
> > +then
> > +    # unload kernel modules to remove dummy interfaces only if
> > they were not in use beforehand
> > +    COUNT_KMODS_AFTER=$(lsmod | grep -c -e "^ip6_gre" -e
> > "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre")
> > +    if [ "$COUNT_KMODS_BEFORE" -eq 0 ] && [ "$COUNT_KMODS_AFTER"
> > -gt 0 ]
> > +    then
> > +        for mod in $KMODS
> > +        do
> > +            sudo rmmod "$mod"
> > +        done
> > +    else
> > +        ts_log "[gre kernel module was loaded before test, not
> > removing]"
> > +    fi
> > +fi
> 
> You repeating yourself with module names.
> How about something like this:
> 
> KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"
> 
> # unload kernel modules to remove dummy interfaces only if they were
> not in use beforehand
> KMODS_REMOVE=
> if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null
> 2>&1; then
> 	for i in $KMODS; do
> 		lsmod |grep -q "^$i " || KMODS_REMOVE="$KMODS_REMOVE
> $i";
> 	done
> fi
> 
> ...
> 
> for mod in $KMODS_REMOVE; do
> 	sudo rmmod "$mod"
> done
> 
> 
> I.e. keeping modules to remove (you can also loaded modules, if you
> want to warn
> about it, but I'd ignore it).
> 
> BTW I'd use 'then' and 'do' on the same line (i.e.: if [ ... ]; then)
> + define variable before usage.

Ok, thanks for the suggestions and the reviews, I've applied them in v2
and tested again, all looks fine.

-- 
Kind regards,
Luca Boccassi
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)