[net-next,v2] net: netconsole: selftests: Create a new netconsole selftest

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.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
Changelog:

v2:
 * Change the location of the path (Jakub)
 * Move from veth to netdevsim
 * Other small changes in dependency checks and cleanup

v1:
 * https://lore.kernel.org/all/ZqyUHN770pjSofTC@gmail.com/

 MAINTAINERS                                   |   1 +
 tools/testing/selftests/drivers/net/Makefile  |   1 +
 .../selftests/drivers/net/netcons_basic.sh    | 223 ++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/netcons_basic.sh

On Tue, 13 Aug 2024 11:38:16 -0700 Breno Leitao wrote:
> 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.

We're getting a:

SKIP: directory /sys/kernel/config/netconsole does not exist. Check if NETCONSOLE_DYNAMIC is enabled

https://netdev-3.bots.linux.dev/vmksft-net-drv-dbg/results/726381/4-netcons-basic-sh/stdout

Gotta extend tools/testing/selftests/drivers/net/config

Hello Jakub,

On Tue, Aug 13, 2024 at 03:37:16PM -0700, Jakub Kicinski wrote:
> On Tue, 13 Aug 2024 11:38:16 -0700 Breno Leitao wrote:
> > 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.
> 
> We're getting a:
> 
> SKIP: directory /sys/kernel/config/netconsole does not exist. Check if NETCONSOLE_DYNAMIC is enabled
> 
> https://netdev-3.bots.linux.dev/vmksft-net-drv-dbg/results/726381/4-netcons-basic-sh/stdout
> 
> Gotta extend tools/testing/selftests/drivers/net/config

Thanks, I've added the following changes in
tools/testing/selftests/drivers/net/config:

	CONFIG_NETCONSOLE=m
	CONFIG_NETCONSOLE_DYNAMIC=y

Then I was able to get the test executing running the following
commands:

	# vng --build  --config tools/testing/selftests/net/config
	# vng -v --run . --user root --cpus 4 -- \
		make -C tools/testing/selftests TARGETS=drivers/net  \
		TEST_PROGS="netcons_basic.sh" TEST_GEN_PROGS="" run_tests
	.....
	# timeout set to 45
	# selftests: drivers/net: netcons_basic.sh
	[   11.172987] netdevsim netdevsim407 eni407np1: renamed from eth1
	[   12.441965] printk: legacy console [netcon_ext0] enabled
	[   12.443049] netpoll: netconsole: local port 6665
	[   12.444104] netpoll: netconsole: local IPv4 address 192.168.1.1
	[   12.445281] netpoll: netconsole: interface 'eni407np1'
	[   12.446299] netpoll: netconsole: remote port 6666
	[   12.447246] netpoll: netconsole: remote IPv4 address 192.168.1.2
	[   12.448419] netpoll: netconsole: remote ethernet address aa:c8:83:a5:05:b3
	[   12.450646] netconsole: network logging started
	[   13.547405] netconsole selftest: netcons_GjLI0
	ok 1 selftests: drivers/net: netcons_basic.sh

I will wait a bit more, and then integrate this change into v2.

Thanks for the review
--breno

Breno Leitao <leitao@debian.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.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> Changelog:
>
> v2:
>  * Change the location of the path (Jakub)
>  * Move from veth to netdevsim
>  * Other small changes in dependency checks and cleanup
>
> v1:
>  * https://lore.kernel.org/all/ZqyUHN770pjSofTC@gmail.com/
>
>  MAINTAINERS                                   |   1 +
>  tools/testing/selftests/drivers/net/Makefile  |   1 +
>  .../selftests/drivers/net/netcons_basic.sh    | 223 ++++++++++++++++++
>  3 files changed, 225 insertions(+)
>  create mode 100755 tools/testing/selftests/drivers/net/netcons_basic.sh
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a9dace908305..ded45f1dff7e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15770,6 +15770,7 @@ M:	Breno Leitao <leitao@debian.org>
>  S:	Maintained
>  F:	Documentation/networking/netconsole.rst
>  F:	drivers/net/netconsole.c
> +F:	tools/testing/selftests/drivers/net/netcons_basic.sh
>  
>  NETDEVSIM
>  M:	Jakub Kicinski <kuba@kernel.org>
> diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
> index e54f382bcb02..928530b26abc 100644
> --- a/tools/testing/selftests/drivers/net/Makefile
> +++ b/tools/testing/selftests/drivers/net/Makefile
> @@ -3,6 +3,7 @@
>  TEST_INCLUDES := $(wildcard lib/py/*.py)
>  
>  TEST_PROGS := \
> +	netcons_basic.sh \
>  	ping.py \
>  	queues.py \
>  	stats.py \
> 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 @@
> +#!/usr/bin/env bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# This test creates two netdevsim virtual interfaces, assigns one of them (the
> +# "destination interface") to a new namespace, and assigns IP addresses to both
> +# interfaces.
> +#
> +# It listens on the destination interface using socat and configures a dynamic
> +# target on netconsole, pointing to the destination IP address.
> +#
> +# Finally, it checks whether the message was received properly on the
> +# destination interface.  Note that this test may pollute the kernel log buffer
> +# (dmesg) and relies on dynamic configuration and namespaces being configured.
> +#
> +# Author: Breno Leitao <leitao@debian.org>
> +
> +set -euo pipefail
> +
> +SCRIPTDIR=$(dirname "$(readlink -e "${BASH_SOURCE[0]}")")
> +
> +# Simple script to test dynamic targets in netconsole
> +SRCIF="" # to be populated later
> +SRCIP=192.168.1.1
> +DSTIF="" # to be populated later
> +DSTIP=192.168.1.2
> +
> +PORT="6666"
> +MSG="netconsole selftest"
> +TARGET=$(mktemp -u netcons_XXXXX)
> +NETCONS_CONFIGFS="/sys/kernel/config/netconsole"
> +NETCONS_PATH="${NETCONS_CONFIGFS}"/"${TARGET}"
> +# This will have some tmp values appended to it in set_network()
> +NAMESPACE="netconsns_dst"
> +
> +# IDs for netdevsim
> +NSIM_DEV_1_ID=$((256 + RANDOM % 256))
> +NSIM_DEV_2_ID=$((512 + RANDOM % 256))
> +
> +# Used to create and delete namespaces
> +source "${SCRIPTDIR}"/../../net/lib.sh
> +
> +# Create netdevsim interfaces
> +create_ifaces() {
> +	local NSIM_DEV_SYS_NEW=/sys/bus/netdevsim/new_device
> +
> +	echo "$NSIM_DEV_2_ID" > "$NSIM_DEV_SYS_NEW"
> +	echo "$NSIM_DEV_1_ID" > "$NSIM_DEV_SYS_NEW"
> +	udevadm settle || true
> +
> +	local NSIM_DEV_1_SYS=/sys/bus/netdevsim/devices/netdevsim"$NSIM_DEV_1_ID"
> +	local NSIM_DEV_2_SYS=/sys/bus/netdevsim/devices/netdevsim"$NSIM_DEV_2_ID"
> +
> +	# These are global variables
> +	SRCIF=$(find "$NSIM_DEV_1_SYS"/net -maxdepth 1 -type d ! \
> +		-path "$NSIM_DEV_1_SYS"/net -exec basename {} \;)
> +	DSTIF=$(find "$NSIM_DEV_2_SYS"/net -maxdepth 1 -type d ! \
> +		-path "$NSIM_DEV_2_SYS"/net -exec basename {} \;)
> +}
> +
> +link_ifaces() {
> +	local NSIM_DEV_SYS_LINK="/sys/bus/netdevsim/link_device"
> +	local SRCIF_IFIDX=$(cat /sys/class/net/"$SRCIF"/ifindex)
> +	local DSTIF_IFIDX=$(cat /sys/class/net/"$DSTIF"/ifindex)
> +
> +	exec {NAMESPACE_FD}</var/run/netns/"${NAMESPACE}"
> +	exec {INITNS_FD}</proc/self/ns/net
> +
> +	# Bind the dst interface to namespace
> +	ip link set "${DSTIF}" netns "${NAMESPACE}"
> +
> +	# Linking one device to the other one (on the other namespace}
> +	echo "${INITNS_FD}:$SRCIF_IFIDX $NAMESPACE_FD:$DSTIF_IFIDX" > $NSIM_DEV_SYS_LINK
> +	if [ $? -ne 0 ]; then
> +		echo "linking netdevsim1 with netdevsim2 should succeed"
> +		cleanup
> +		exit ${ksft_skip}
> +	fi
> +}
> +
> +function configure_ip() {
> +	# Configure the IPs for both interfaces
> +	ip netns exec "${NAMESPACE}" ip addr add "${DSTIP}"/24 dev "${DSTIF}"
> +	ip netns exec "${NAMESPACE}" ip link set "${DSTIF}" up
> +
> +	ip addr add "${SRCIP}"/24 dev "${SRCIF}"
> +	ip link set "${SRCIF}" up
> +}
> +
> +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@kernel.org>, Hangbin Liu <liuhangbin@gmail.com>,
Matthieu Baerts (NGI0) <matttbe@kernel.org> who were in the vicinity
in the past.

> +	NAMESPACE=${NS_LIST[0]}
> +
> +	# Create both interfaces, and assign the destination to a different namespace
> +	create_ifaces
> +
> +	# Link both interfaces back to back
> +	link_ifaces
> +
> +	configure_ip
> +}
> +
> +function create_dynamic_target() {
> +	DSTMAC=$(ip netns exec "${NAMESPACE}" ip link show "${DSTIF}" | awk '/ether/ {print $2}')
> +
> +	# Create a dynamic target
> +	mkdir "${NETCONS_PATH}"
> +
> +	echo "${DSTIP}" > "${NETCONS_PATH}"/remote_ip
> +	echo "${SRCIP}" > "${NETCONS_PATH}"/local_ip
> +	echo "${DSTMAC}" > "${NETCONS_PATH}"/remote_mac
> +	echo "${SRCIF}" > "${NETCONS_PATH}"/dev_name
> +
> +	echo 1 > "${NETCONS_PATH}"/enabled
> +}
> +
> +function cleanup() {
> +	local NSIM_DEV_SYS_DEL="/sys/bus/netdevsim/del_device"
> +
> +	# delete netconsole dynamic reconfiguration
> +	echo 0 > "${NETCONS_PATH}"/enabled
> +	# Remove the configfs entry
> +	rmdir "${NETCONS_PATH}"
> +
> +	# Delete netdevsim devices
> +	echo "$NSIM_DEV_2_ID" > "$NSIM_DEV_SYS_DEL"
> +	echo "$NSIM_DEV_1_ID" > "$NSIM_DEV_SYS_DEL"
> +
> +	# this is coming from lib.sh
> +	cleanup_all_ns
> +}
> +
> +function listen_port_and_save_to() {
> +	OUTPUT=${1}

local

> +	# Just wait for 2 seconds
> +	timeout 2 ip netns exec "${NAMESPACE}" socat UDP-LISTEN:"${PORT}",fork "${OUTPUT}"
> +}
> +
> +function validate_result() {
> +	TMPFILENAME="$1"

local

> +
> +	# Check if the file exists
> +	if [ ! -f "$TMPFILENAME" ]; then
> +	    echo "FAIL: File was not generated." >&2
> +	    return ${ksft_fail}

The indentation seems wrong here.

> +	fi
> +
> +	if ! grep -q "${MSG}" "${TMPFILENAME}"; then
> +	    echo "FAIL: ${MSG} not found in ${TMPFILENAME}" >&2
> +	    cat "${TMPFILENAME}" >&2
> +	    return ${ksft_fail}
> +	fi
> +
> +	# Delete the file once it is validated, otherwise keep it
> +	# for debugging purposes
> +	rm "${TMPFILENAME}"

Seeing the removal within the validation function is odd, I would expect
it to be part of cleanup().

> +	return ${ksft_pass}
> +}
> +
> +function check_for_dependencies() {
> +	if [ "$(id -u)" -ne 0 ]; then
> +		echo "This script must be run as root" >&2
> +		exit "${ksft_skip}"
> +	fi
> +
> +	if ! which socat > /dev/null ; then
> +		echo "SKIP: socat(1) is not available" >&2
> +		exit "${ksft_skip}"
> +	fi
> +
> +	if ! which ip > /dev/null ; then
> +		echo "SKIP: ip(1) is not available" >&2
> +		exit "${ksft_skip}"
> +	fi
> +
> +	if ! which udevadm > /dev/null ; then
> +		echo "SKIP: udevadm(1) is not available" >&2
> +		exit "${ksft_skip}"
> +	fi
> +
> +	if [ ! -d "${NETCONS_CONFIGFS}" ]; then
> +		echo "SKIP: directory ${NETCONS_CONFIGFS} does not exist. Check if NETCONSOLE_DYNAMIC is enabled" >&2
> +		exit "${ksft_skip}"
> +	fi
> +
> +	if ip link show "${DSTIF}" 2> /dev/null; then
> +		echo "SKIP: interface ${DSTIF} exists in the system. Not overwriting it."
> +		exit "${ksft_skip}"
> +	fi
> +}
> +
> +# ========== #
> +# Start here #
> +# ========== #
> +modprobe netdevsim || true
> +# The content of kmsg will be save to the following file
> +OUTPUT_FILE="/tmp/${TARGET}"
> +
> +# Check for basic system dependency and exit if not found
> +check_for_dependencies
> +# Remove the namespace, interfaces and netconsole target on exit
> +trap cleanup EXIT
> +# Create one namespace and two interfaces
> +set_network
> +# Create a dynamic target for netconsole
> +create_dynamic_target
> +# Listed for netconsole port inside the namespace and destination interface
> +listen_port_and_save_to "${OUTPUT_FILE}" &
> +
> +# Wait for socat to start and listen to the port.
> +sleep 1
> +# Send the message
> +echo "${MSG}: ${TARGET}" > /dev/kmsg
> +# Wait until socat saves the file to disk
> +sleep 1
> +
> +# Make sure the message was received in the dst part
> +validate_result "${OUTPUT_FILE}"
> +ret=$?
> +
> +exit ${ret}

Hi Petr, Breno,

On 14/08/2024 12:24, Petr Machata wrote:
> 
> Breno Leitao <leitao@debian.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@kernel.org>, Hangbin Liu <liuhangbin@gmail.com>,
> Matthieu Baerts (NGI0) <matttbe@kernel.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

On Wed, Aug 14, 2024 at 12:24:46PM +0200, Petr Machata wrote:
> 
> Breno Leitao <leitao@debian.org> writes:

> > +	fi
> > +
> > +	if ! grep -q "${MSG}" "${TMPFILENAME}"; then
> > +	    echo "FAIL: ${MSG} not found in ${TMPFILENAME}" >&2
> > +	    cat "${TMPFILENAME}" >&2
> > +	    return ${ksft_fail}
> > +	fi
> > +
> > +	# Delete the file once it is validated, otherwise keep it
> > +	# for debugging purposes
> > +	rm "${TMPFILENAME}"
> 
> Seeing the removal within the validation function is odd, I would expect
> it to be part of cleanup().

Thanks for all the other feedbacks, all of them make sense.

Regarding this one, I kept like this, because I only remove the file if
the test succeed, otherwise I keep the file here for debugging purposes,
as described in the comment above.

If that is not a good practice, I am more than happy to move this
to cleanup.


Thanks for the detailed review,
--breno