Re: [PATCH can-next v2 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] All four new tests in
  tools/testing/selftests/net/can/test_j1939_multi.c (tp_rx_from_raw,…

--

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/test_j1939_multi.c b/tools/testing/selftests/net/can/test_j1939_multi.c
> new file mode 100644
> index 0000000000000..88a283a3394e0
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_j1939_multi.c

[ ... ]

> +static bool tp_serve_cts_ok(int raw, const uint8_t *payload, size_t size,
> +			    unsigned int num_pkts, unsigned int cts_limit)
> +{
> +	unsigned int sent = 0;
> +	struct can_frame frame;
> +
> +	while (sent < num_pkts) {
> +		unsigned int cts_pkts, next;
> +
> +		if (!recv_kernel_cm_ok(raw, TP_CM, TP_CMD_CTS, &frame,
> +				       J1939_RECV_TIMEOUT_MS))
> +			return false;
> +
> +		cts_pkts = frame.data[TP_CTS_PKTS_OFF];
> +		next = frame.data[TP_CTS_NEXT_OFF];	/* 1-based */
> +		if (!cts_accept_ok(cts_pkts, next, sent, num_pkts, cts_limit))
> +			return false;
> +
> +		if (!send_tp_dt_burst_ok(raw, SA_LOCAL, SA_PEER, next, cts_pkts,
> +					 payload, size))
> +			return false;

[Severity: Medium]
Can this DT burst reach the j1939 stack before the stack has handled the
loopback echo of its own CTS?

can_rcv_filter() delivers to the raw socket's RX_ALL filter before
j1939_can_recv() runs. So the raw socket can see the kernel's CTS before
j1939 has handled the echo.

The DT frames sent here go through raw_sendmsg()->can_send()->netif_rx()
on the test thread's CPU. They can be processed before the echo.

j1939_xtp_rx_cts_one() sets the session state from the echoed CTS:

	session->last_cmd = dat[0];

If a DT arrives while last_cmd is still TP_CMD_RTS,
j1939_xtp_rx_dat_one() cancels the session:

	switch (session->last_cmd) {
	...
	default:
		netdev_info(priv->ndev, "%s: 0x%p: last %02x\n", __func__,
			    session, session->last_cmd);
		goto out_session_cancel;
	}

The ETP tests seem to follow the same pattern.

In etp_rx_serve_cts_ok(), the peer's DPO can be handled before the echo
of the kernel's ETP.CTS. The echo then sets last_cmd to ETP_CMD_CTS, and
the DT frames that follow cancel the session.

In etp_tx_serve_block_ok(), the next ETP.CTS goes out as soon as the
last DT of the previous block is read from the raw socket:

	cm_dat_init(dat, ETP_CMD_CTS);
	dat[ETP_CTS_PKTS_OFF] = cts_pkts;
	put_le24(&dat[ETP_CTS_NEXT_OFF], next);
	if (!peer_send_cm_ok(self->raw, ETP_CM, dat))

If that CTS is handled before the echo of the last DT, the echo hits the
default case in j1939_xtp_rx_dat_one() and cancels the session. This
window opens at every block boundary in etp_tx_to_raw.

The notes below the "---" line describe this:

  "can a multi test fail against a correct kernel?
   Yes, through one known race in the stack ... The fix belongs in the
   stack, not in these tests."

git drops that text when the patch is applied. Neither the commit log
nor test_j1939_multi.c mentions the race.

No patch in the series changes net/can/j1939/. test_j1939_multi.sh is
added to TEST_PROGS, so default kselftest runs can fail at random with
no explanation in the tree.

The later patch "selftests: can: cover J1939 transport abort and timeout
paths" adds tp_rx_cts_block_limit. It reuses tp_rx_roundtrip_ok() with
more CTS windows, which means more chances to hit the race.

Could the race be documented in the commit message or in a comment in
test_j1939_multi.c? Alternatively, could these tests wait until the
stack fix lands?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104-j1939-selftests-v2-0-cf797c83317b%40pengutronix.de