netdev - Re: [PATCH net 2/3] selftests: net: lib: avoid error removing empty netns name

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]

Message-ID: <0f85551f-dd67-4d88-abc2-cdf80293a604@kernel.org>
Date: Wed, 5 Jun 2024 16:12:52 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Petr Machata <petrm@...dia.com>
Cc: mptcp@...ts.linux.dev, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>,
 Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org, Geliang Tang <geliang@...nel.org>
Subject: Re: [PATCH net 2/3] selftests: net: lib: avoid error removing empty
 netns name

Hi Petr,

Thank you for the review!

On 05/06/2024 12:38, Petr Machata wrote:
> 
> "Matthieu Baerts (NGI0)" <matttbe@...nel.org> writes:
> 
>> If there is an error to create the first netns with 'setup_ns()',
>> 'cleanup_ns()' will be called with an empty string as first parameter.
>>
>> The consequences is that 'cleanup_ns()' will try to delete an invalid
>> netns, and wait 20 seconds if the netns list is empty.
>>
>> Instead of just checking if the name is not empty, convert the string
>> separated by spaces to an array. Manipulating the array is cleaner, and
>> calling 'cleanup_ns()' with an empty array will be a no-op.
>>
>> Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
>> Cc: stable@...r.kernel.org
>> Acked-by: Geliang Tang <geliang@...nel.org>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@...nel.org>
>> ---
>>  tools/testing/selftests/net/lib.sh | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
>> index a422e10d3d3a..e2f51102d7e1 100644
>> --- a/tools/testing/selftests/net/lib.sh
>> +++ b/tools/testing/selftests/net/lib.sh
>> @@ -15,7 +15,7 @@ ksft_xfail=2
>>  ksft_skip=4
>>  
>>  # namespace list created by setup_ns
>> -NS_LIST=""
>> +NS_LIST=()
>>  
>>  ##############################################################################
>>  # Helpers
>> @@ -137,6 +137,7 @@ cleanup_ns()
>>  	fi
>>  
>>  	for ns in "$@"; do
>> +		[ -z "${ns}" ] && continue
> 
> I think this is now irrelevant though? Now cleanup_ns() will be called
> with no arguments for an empty NS list, so the loop does not even kick in.

If you don't mind, I think it is "safer" to keep it: some selftests are
using 'cleanup_ns()' directly, not via 'cleanup_all_ns()', e.g.
netns-name.sh, cmsg-*.sh, fib-*.sh, etc. which can call it with the
variables not set if 'setup_ns' failed during the init phase.

For the moment, all these selftests are calling 'cleanup_ns()' with
parameters added without double quotes: so it is fine. Until someone
changes that to please shellcheck, like we did on our side with MPTCP
selftests. So this line will be useful soon when we will publish the
rest of our patches to use 'lib.sh' [1] :)

Link:
https://lore.kernel.org/mptcp/5f4615c3-0621-43c5-ad25-55747a4350ce@kernel.org/T/
[1]

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.