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:   Sat, 15 Dec 2018 18:37:07 +0100
From:   Petr Vorel <pvorel@...e.cz>
To:     Luca Boccassi <bluca@...ian.org>
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

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.

> +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.

[1] https://stackoverflow.com/questions/34572700/is-command-v-option-required-in-a-posix-shell-is-posh-compliant-with-posix

Kind regards,
Petr