netdev - Re: [PATCH net-next 01/38] selftests/net: add lib.sh

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: Fri, 24 Nov 2023 15:35:51 +0100
From: Petr Machata <petrm@...dia.com>
To: Hangbin Liu <liuhangbin@...il.com>
CC: <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>, "Jakub
 Kicinski" <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, "Paolo
 Abeni" <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>, David Ahern
	<dsahern@...nel.org>, <linux-kselftest@...r.kernel.org>, Po-Hsu Lin
	<po-hsu.lin@...onical.com>, Guillaume Nault <gnault@...hat.com>,
	Björn Töpel <bjorn@...osinc.com>, Ryan Roberts
	<ryan.roberts@....com>, Andrew Morton <akpm@...ux-foundation.org>, "Mark
 Brown" <broonie@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>
Subject: Re: [PATCH net-next 01/38] selftests/net: add lib.sh


Hangbin Liu <liuhangbin@...il.com> writes:

> +cleanup_ns()
> +{
> +	local ns=""
> +	local errexit=0
> +
> +	# disable errexit temporary
> +	if [[ $- =~ "e" ]]; then
> +		errexit=1
> +		set +e
> +	fi
> +
> +	for ns in "$@"; do
> +		ip netns delete "${ns}" &> /dev/null
> +		busywait 2 "ip netns list | grep -vq $1" &> /dev/null

The grep would get confused by substrings of other names.
This should be grep -vq "^$ns$".

> +		if ip netns list | grep -q $1; then

Busywait returns != 0 when the wait condition is not reached within a
given time. So it should be possible to roll the duplicated if-grep into
the busywait line like so:

		if ! busywait 2 "ip netns etc."; then

> +			echo "Failed to remove namespace $1"
> +			return $ksft_skip

This does not restore the errexit.

I think it might be clearest to have this function as a helper, say
__cleanup_ns, and then have a wrapper that does the errexit management:

cleanup_ns()
{
	local errexit
	local rc

	# disable errexit temporarily
	if [[ $- =~ "e" ]]; then
		errexit=1
		set +e
	fi

	__cleanup_ns "$@"
	rc=$?

	[ $errexit -eq 1 ] && set -e
	return $rc
}

If this comes up more often, we can have a helper like
with_disabled_errexit or whatever, that does this management and
dispatches to "$@", so cleanup_ns() would become:

cleanup_ns()
{
	with_disabled_errexit __cleanup_ns "$@"
}

> +		fi
> +	done
> +
> +	[ $errexit -eq 1 ] && set -e
> +	return 0
> +}
> +
> +# By default, remove all netns before EXIT.
> +cleanup_all_ns()
> +{
> +	cleanup_ns $NS_LIST
> +}
> +trap cleanup_all_ns EXIT

Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh,
because basically all users of forwarding/lib.sh use the EXIT trap.

I wonder if we need something like these push_cleanup / on_exit helpers:

	https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15

But I don't want to force this on your already large patchset :)
So just ignore the bit about including from forwarding/lib.sh.

> +# setup netns with given names as prefix. e.g
> +# setup_ns local remote
> +setup_ns()
> +{
> +	local ns=""
> +	# the ns list we created in this call
> +	local ns_list=""
> +	while [ -n "$1" ]; do

I would find it more readable if this used the same iteration approach
as the 'for ns in "$@"' above. The $1/shift approach used here is
somewhat confusing.

> +		# Some test may setup/remove same netns multi times
> +		if unset $1 2> /dev/null; then
> +			ns="${1,,}-$(mktemp -u XXXXXX)"
> +			eval readonly $1=$ns
> +		else
> +			eval ns='$'$1
> +			cleanup_ns $ns
> +
> +		fi
> +
> +		ip netns add $ns
> +		if ! ip netns list | grep -q $ns; then

As above, the grep could get confused. But in fact wouldn't just
checking the exit code of ip netns add be enough?

> +			echo "Failed to create namespace $1"
> +			cleanup_ns $ns_list
> +			return $ksft_skip
> +		fi
> +		ip -n $ns link set lo up
> +		ns_list="$ns_list $ns"
> +
> +		shift
> +	done
> +	NS_LIST="$NS_LIST $ns_list"
> +}