Re: [PATCH can-next v3 4/9] selftests: can: add J1939 socket option and metadata tests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In TEST_F(j1939_opts, filter_pgn)
  (tools/testing/selftests/net/can/test_j1939_opts.c), the check that…

--

selftests: can: add J1939 socket option and metadata tests

This adds a J1939 selftest that checks socket options and send metadata.
It covers the SO_J1939_SEND_PRIO priority seen on the wire through a
CAN_RAW peer, getsockopt round-trips, the SO_J1939_FILTER PGN whitelist,
unconnected sendto(), a wrong-family bind() and the SO_J1939_ERRQUEUE
TX-completion ACK.

> diff --git a/tools/testing/selftests/net/can/test_j1939_opts.c b/tools/testing/selftests/net/can/test_j1939_opts.c
> new file mode 100644
> index 0000000000000..a729e39c0be4f
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_j1939_opts.c
> @@ -0,0 +1,309 @@

[ ... ]

> +/* SO_J1939_FILTER whitelists PGN_A; PGN_A is received, PGN_B is dropped. */
> +TEST_F(j1939_opts, filter_pgn)
> +{

[ ... ]

> +	/*
> +	 * PGN_B must be dropped by the filter. Send PGN_A after it and read it
> +	 * back: a PGN_B that got through is read before PGN_A or is still
> +	 * queued after it.
> +	 */
> +	ASSERT_TRUE(j1939_sendto_ok(self->tx, tx_buf, sizeof(tx_buf),
> +				    self->ifindex, J1939_NO_NAME, SA_RX, PGN_B))
> +		TH_LOG("sendto PGN_B failed: %d", errno);
> +	ASSERT_TRUE(j1939_sendto_ok(self->tx, tx_buf, sizeof(tx_buf),
> +				    self->ifindex, J1939_NO_NAME, SA_RX, PGN_A))
> +		TH_LOG("sendto PGN_A failed: %d", errno);
> +	ASSERT_TRUE(recvfrom_verify_ok(self->rx, tx_buf, sizeof(tx_buf),
> +				       J1939_RECV_TIMEOUT_MS, &src))
> +		TH_LOG("PGN_A after PGN_B was not received");
> +	EXPECT_EQ(src.can_addr.j1939.pgn, filt.pgn)
> +		TH_LOG("PGN_B was received before PGN_A");
> +	EXPECT_EQ(recv(self->rx, rx_buf, sizeof(rx_buf), MSG_DONTWAIT), -1)
> +		TH_LOG("PGN_B was received but should have been filtered out");
> +	EXPECT_EQ(errno, EAGAIN);
> +}

[Severity: Low]
Do the two looped-back frames always reach the rx socket in send order?
The comment above assumes they do, and so does the v3 changelog entry
"check that the filter drops PGN_B with an ordering barrier, not a
timeout".

The J1939 session queue serializes the two transmissions. Each frame is
then sent from the session's hrtimer:

j1939_tp_txtimer()->j1939_simple_txnext()->j1939_send_one()->can_send()

On vcan without IFF_ECHO, can_send() in net/can/af_can.c loops the frame
back with:

	if (newskb)
		netif_rx(newskb);

This puts it on the backlog of whichever CPU is running at that moment.

If PGN_A is already queued, j1939_sk_queue_activate_next_locked() starts
its timer from the PGN_B timer's softirq. It normally stays on the same
CPU, so the order holds in that case.

If the PGN_B session has already ended before sendto(PGN_A), the PGN_A
session starts from sendmsg on the test task's current CPU. Suppose the
task has moved to another CPU and NET_RX on the first CPU has been
deferred, for example to ksoftirqd or on PREEMPT_RT. Could PGN_A then be
delivered first?

In that case recvfrom_verify_ok() returns PGN_A. The recv(MSG_DONTWAIT)
then returns -1/EAGAIN before a PGN_B that wrongly passed the filter has
been queued.

This can only give a false pass when SO_J1939_FILTER is broken, never a
false failure. Should the comment and the changelog say less about
ordering, or is there a stronger ordering guarantee the test can rely
on here?

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