linux-kernel - Re: [PATCH net-next v2] net: netconsole: selftests: Create a new netconsole selftest

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: <af896c55-723e-4c2b-b153-132f863e2f68@kernel.org>
Date: Wed, 14 Aug 2024 13:31:50 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Petr Machata <petrm@...dia.com>, Breno Leitao <leitao@...ian.org>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
 David Wei <dw@...idwei.uk>, Willem de Bruijn <willemb@...gle.com>,
 open list <linux-kernel@...r.kernel.org>,
 "open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>,
 Geliang Tang <geliang@...nel.org>, Hangbin Liu <liuhangbin@...il.com>
Subject: Re: [PATCH net-next v2] net: netconsole: selftests: Create a new
 netconsole selftest

Hi Petr, Breno,

On 14/08/2024 12:24, Petr Machata wrote:
> 
> Breno Leitao <leitao@...ian.org> writes:
> 
>> Adds a selftest that creates two virtual interfaces, assigns one to a
>> new namespace, and assigns IP addresses to both.
>>
>> It listens on the destination interface using socat and configures a
>> dynamic target on netconsole, pointing to the destination IP address.
>>
>> The test then checks if the message was received properly on the
>> destination interface.

(...)

>> diff --git a/tools/testing/selftests/drivers/net/netcons_basic.sh b/tools/testing/selftests/drivers/net/netcons_basic.sh
>> new file mode 100755
>> index 000000000000..e0e58fc7e89f
>> --- /dev/null
>> +++ b/tools/testing/selftests/drivers/net/netcons_basic.sh
>> @@ -0,0 +1,223 @@

(...)

> +NAMESPACE="netconsns_dst"

(...)

>> +function set_network() {
>> +	# This is coming from lib.sh. And it does unbound variable access
>> +	set +u
>> +	setup_ns "${NAMESPACE}"
>> +	set -u
> 
> It would make sense to fix lib.sh. I think this is what is needed?
> 
> modified   tools/testing/selftests/net/lib.sh
> @@ -178,7 +178,7 @@ setup_ns()
>  		fi
>  
>  		# Some test may setup/remove same netns multi times
> -		if [ -z "${!ns_name}" ]; then
> +		if ! declare -p "$ns_name" &> /dev/null; then
>  			eval "${ns_name}=${ns_name,,}-$(mktemp -u XXXXXX)"
>  		else
>  			cleanup_ns "${!ns_name}"
> 
> CC'd Geliang Tang <geliang@...nel.org>, Hangbin Liu <liuhangbin@...il.com>,
> Matthieu Baerts (NGI0) <matttbe@...nel.org> who were in the vicinity
> in the past.
Thank you for having CCed me.

I don't know if lib.sh needs to be modified: setup_ns() is supposed to
be called with the name of an existing variable. Can you not define this
variable before?

I mean: the modification from Petr looks good to me to support 'set -u',
but it sounds safer to define the variable before in the script, just in
case it is defined by in the environment, before starting the test, and
not taking the expected path.

Note that in all the other selftests, setup_ns() is called with the name
of the variable, not a variable like you did, e.g.

  NAMESPACE=
  setup_ns NAMESPACE

instead of:

  NAMESPACE="netconsns_dst"
  setup_ns "${NAMESPACE}"
  NAMESPACE=${NS_LIST[0]}

Maybe better to do like the others?

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