[PATCH 0/1] selftests: net: fix file owner for broadcast_ether_dst test

[PATCH 0/1] selftests: net: fix file owner for broadcast_ether_dst test

[Ross Porter]

The broadcast_ether_dst test can spuriously fail due to file
permissions, depending on which flags tcpdump is compiled with: If
tcpdump is compiled with the `--with-user` flag then tcpdump will
step down to the provided user after opening the device. In this
case the test fails due to permission issues, as the output file
is owned by a different user.
Debian ships tcpdump with this flag enabled, so this bug
appears as part of our kernel testing.

To fix this, we can ensure tcpdump remains as root (regardless of
whether it was compiled with `--with-user`) by passing the `-Z root`
argument when invoking it.

This is my first kernel contribution, so please let me know if I've 
missed anything.

Ross Porter (1):
  selftests: net: fix file owner for broadcast_ether_dst test

 tools/testing/selftests/net/broadcast_ether_dst.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.53.0

[PATCH 1/1] selftests: net: fix file owner for broadcast_ether_dst test

[Ross Porter]

Ensure the output file is always owned by root (even if tcpdump was 
compiled with `--with-user`), by passing the `-Z root` argument when 
invoking it.

Cc: stable@vger.kernel.org
Reported-by: Edoardo Canepa <edoardo.canepa@canonical.com>
Closes: https://bugs.launchpad.net/ubuntu-kernel-tests/+bug/2129815
Fixes: bf59028ea8d4 ("selftests: net: add test for destination in broadcast packets")
Suggested-by: Edoardo Canepa <edoardo.canepa@canonical.com>
Tested-by: Ross Porter <ross.porter@canonical.com>
Signed-off-by: Ross Porter <ross.porter@canonical.com>
---
 tools/testing/selftests/net/broadcast_ether_dst.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/broadcast_ether_dst.sh b/tools/testing/selftests/net/broadcast_ether_dst.sh
index 334a7eca8a80..5e7a8fe23c7a 100755
--- a/tools/testing/selftests/net/broadcast_ether_dst.sh
+++ b/tools/testing/selftests/net/broadcast_ether_dst.sh
@@ -44,7 +44,7 @@ test_broadcast_ether_dst() {
 	# tcpdump will exit after receiving a single packet
 	# timeout will kill tcpdump if it is still running after 2s
 	timeout 2s ip netns exec "${CLIENT_NS}" \
-		tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp &> "${OUTPUT}" &
+		tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp -Z root &> "${OUTPUT}" &
 	pid=$!
 	slowwait 1 grep -qs "listening" "${OUTPUT}"
 
-- 
2.53.0

Re: [PATCH 1/1] selftests: net: fix file owner for broadcast_ether_dst test

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests: net: fix file owner for broadcast_ether_dst test

This commit ensures the tcpdump output file in the broadcast_ether_dst test
is always owned by root by passing the -Z root argument.

> diff --git a/tools/testing/selftests/net/broadcast_ether_dst.sh b/tools/testing/selftests/net/broadcast_ether_dst.sh
> --- a/tools/testing/selftests/net/broadcast_ether_dst.sh
> +++ b/tools/testing/selftests/net/broadcast_ether_dst.sh
> @@ -44,7 +44,7 @@ test_broadcast_ether_dst() {
>  	# tcpdump will exit after receiving a single packet
>  	# timeout will kill tcpdump if it is still running after 2s
>  	timeout 2s ip netns exec "${CLIENT_NS}" \
> -		tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp &> "${OUTPUT}" &
> +		tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp -Z root &> "${OUTPUT}" &

[Severity: Medium]
Will this tcpdump command fail with a syntax error in test_broadcast_ether_dst()
on systems that use musl libc or when POSIXLY_CORRECT is set?

By placing the -Z root option after the positional filter expression icmp,
standard POSIX option parsing will stop at the first non-option argument.

While this works on glibc systems because getopt() permutes arguments by
default, on strict POSIX systems tcpdump might treat -Z root as part of the
filter expression. This would concatenate the arguments into "icmp -Z root"
and cause a fatal pcap filter compilation error, breaking the selftest
unconditionally in those environments.

Could the -Z root argument be moved before the icmp filter expression?
-- 
pw-bot: cr