Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets

[Brett Sheffield <bacs@librecast.net>]

On 2025-08-29 12:19, Simon Horman wrote:
> On Thu, Aug 28, 2025 at 01:42:42PM +0200, Oscar Maes wrote:
> > Add test to check the broadcast ethernet destination field is set
> > correctly.
> > 
> > This test sends a broadcast ping, captures it using tcpdump and
> > ensures that all bits of the 6 octet ethernet destination address
> > are correctly set by examining the output capture file.
> > 
> > Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> > Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
> 
> ...
> 
> > +test_broadcast_ether_dst() {
> > +	local rc=0
> > +	CAPFILE=$(mktemp -u cap.XXXXXXXXXX)
> > +	OUTPUT=$(mktemp -u out.XXXXXXXXXX)
> > +
> > +	echo "Testing ethernet broadcast destination"
> > +
> > +	# start tcpdump listening for icmp
> > +	# 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}" &
> > +	pid=$!
> > +	slowwait 1 grep -qs "listening" "${OUTPUT}"
> > +
> > +	# send broadcast ping
> > +	ip netns exec "${CLIENT_NS}" \
> > +		ping -W0.01 -c1 -b 255.255.255.255 &> /dev/null
> > +
> > +	# wait for tcpdump for exit after receiving packet
> > +	wait "${pid}"
> 
> Hi Oscar and Brett,
> 
> I am concerned that if something goes wrong this may block forever.
> Also, I'm wondering if this test could make use of the tcpdump helpers
> provided in tools/testing/selftests/net/forwarding/lib.sh

Thanks for the review Simon.  Further to previous email after some more thought
and poking at lib.sh

We're starting tcpdump with -c1 so that it exits immediately when the packet is
received, and we catch this with the wait() so that, in the best case, we
continue immediately, and in the worse case the `timeout 2s` kills tcpdump and
we move on to cleanup. I *think* this is pretty safe.

Taking a look at the forwarding/lib.sh it looks like we could use
tcpdump_start() and pass in $TCPDUMP_EXTRA_FLAGS but I don't think this buys us
much here, as we'd still need to wait() or a sleep() or otherwise detect that
tcpdump is finished so we can continue. I don't see anything in lib.sh to aid us
with that?

That said, it might be good to use the helper function anyway and keep the
wait() for consistency. There don't seem to be many tests using the tcpdump
helper functions yet, but it's probably the right way to move.

What do you think, Oscar?  It looks like lib.sh tcpdump_start() takes all the
arguments, including for your namespaces.  Up to you if you want to call that
instead.

Now I know it's there, I'll try to use that for future tests.

I don't *think* there's anything here that needs a v4, unless the timeout() call
is thought to be insufficient to kill tcpdump.  There's a -k switch if we want
to SIGKILL it :-)

> > +
> > +	# compare ethernet destination field to ff:ff:ff:ff:ff:ff
> > +	ether_dst=$(tcpdump -r "${CAPFILE}" -tnne 2>/dev/null | \
> > +			awk '{sub(/,/,"",$3); print $3}')
> > +	if [[ "${ether_dst}" == "ff:ff:ff:ff:ff:ff" ]]; then
> > +		echo "[ OK ]"
> > +		rc="${ksft_pass}"
> > +	else
> > +		echo "[FAIL] expected dst ether addr to be ff:ff:ff:ff:ff:ff," \
> > +			"got ${ether_dst}"
> > +		rc="${ksft_fail}"
> > +	fi
> > +
> > +	return "${rc}"
> > +}
> 
> ...

-- 
Brett Sheffield (he/him)
Librecast - Decentralising the Internet with Multicast
https://librecast.net/
https://blog.brettsheffield.com/