Re: [PATCH can-next v3 7/9] selftests: can: add J1939 TP/ETP multi-frame tests against CAN_RAW

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch adds four TP/ETP tests (tp_rx_from_raw,
  tp_tx_to_raw, etp_rx_from_raw, etp_tx_to_raw) that the author says can…

--

selftests: can: add J1939 TP/ETP multi-frame tests against CAN_RAW

This adds TP and ETP round-trip selftests in both directions. A CAN_RAW
socket acts as the transport peer, so the frames are checked as they
appear on the wire. The TX tests also check that the TX-completion ACK
reaches the error queue only after the peer's EOMA.

> diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile
> index 21898e645ced7..99113184a525a 100644
> --- a/tools/testing/selftests/net/can/Makefile
> +++ b/tools/testing/selftests/net/can/Makefile
> @@ -9,6 +9,7 @@ LOCAL_HDRS := j1939_helpers.h j1939_tests.h ../../clock-helpers.h
>  TEST_PROGS := \
>  	test_j1939_addr.sh \
>  	test_j1939_base.sh \
> +	test_j1939_multi.sh \
>  	test_j1939_opts.sh \
>  	test_j1939_wire.sh \
>  	test_raw_filter.sh \

[ ... ]

> diff --git a/tools/testing/selftests/net/can/test_j1939_multi.c b/tools/testing/selftests/net/can/test_j1939_multi.c
> new file mode 100644
> index 0000000000000..aec96e83305cf
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_j1939_multi.c
> @@ -0,0 +1,682 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> +/*
> + * J1939 multi-frame wire selftests: TP and ETP against CAN_RAW.
> + *

[ ... ]

> + * Known stack race: the stack updates a session from the loopback echo of
> + * its own CTS, RTS and DT frames, and the CAN_RAW peer can read that echo
> + * before the stack has handled it. A peer frame that follows quickly can
> + * then cancel the session, so these tests can fail at random. The fix
> + * belongs in the stack; this file does not work around it.

[Severity: Medium]
test_j1939_multi.sh goes straight into TEST_PROGS. Will a default
kselftest run then see random failures from this race that cannot be told
apart from real regressions?

The raw peer always gets the echo first. raw_init() sets a zero can_mask,
which puts the raw socket on rx[RX_ALL]. j1939 registers on rx[RX_FIL].
can_rcv_filter() walks the two lists in that order:

net/can/af_can.c:can_rcv_filter() {
    ...
	/* check for unfiltered entries */
	hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ALL], list) {
		deliver(skb, rcv);
    ...
	/* check for can_id/mask entries */
	hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_FIL], list) {
    ...
}

In the RX tests, tp_serve_cts_ok() and etp_rx_serve_cts_ok() send the DPO
and DT frames as soon as they read the kernel's CTS. If the stack has not
handled the echo of its own CTS yet, last_cmd still holds the RTS. The
peer's DT then lands in the default case here:

net/can/j1939/transport.c:j1939_xtp_rx_dat_one() {
    ...
	switch (session->last_cmd) {
	case 0xff:
		break;
	case J1939_ETP_CMD_DPO:
    ...
	default:
		netdev_info(priv->ndev, "%s: 0x%p: last %02x\n", __func__,
			    session, session->last_cmd);
		goto out_session_cancel;
	}
    ...
}

In the TX tests, tp_tx_to_raw() and etp_tx_serve_block_ok() send a CTS
right after they read the kernel's RTS or the last DT of a block. If the
peer's CTS is handled before the RTS echo, last_cmd becomes CTS. The late
echo then cancels the session:

net/can/j1939/transport.c:j1939_xtp_rx_rts_session_active() {
    ...
	if (session->last_cmd != 0) {
		/* we received a second rts on the same connection */
    ...
		j1939_session_timers_cancel(session);
		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
    ...
}

A late echo of the last DT of a block can also arrive after the peer's
next ETP.CTS. That echo hits the same default case in
j1939_xtp_rx_dat_one().

This file comment is the only note about the race in the tree. The Q/A
under the --- line gets dropped when the patch is applied, and the commit
message does not mention the race. At the end of the series ("selftests:
can: check SO_J1939_PROMISC receive effect") the comment is the same and
test_j1939_multi.sh is still in TEST_PROGS with nothing to work around
the race.

Could these tests stay out of the default run, be marked as expected
failures, or wait for the stack to handle each echo before the peer
replies, until the stack is fixed? Could the commit message also mention
the race?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-j1939-selftests-v3-0-5a80277ab363%40pengutronix.de