Re: [PATCH can-next v2 9/9] selftests: can: check SO_J1939_PROMISC receive effect

[netdev-bot+sashiko]

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, promisc_receive)
  (tools/testing/selftests/net/can/test_j1939_opts.c), the post-promisc…

--

selftests: can: check SO_J1939_PROMISC receive effect

This adds a J1939 selftest that checks SO_J1939_PROMISC changes what the
RX path delivers. It sends two probe frames, each missing either the bound
address or the bound PGN, once before and once after enabling the flag.
Only the frames sent after enabling the flag are expected to arrive.

> diff --git a/tools/testing/selftests/net/can/test_j1939_opts.c b/tools/testing/selftests/net/can/test_j1939_opts.c
> index 10d0e37984027..bbf2698de7d94 100644
> --- a/tools/testing/selftests/net/can/test_j1939_opts.c
> +++ b/tools/testing/selftests/net/can/test_j1939_opts.c

[ ... ]

> +static const struct {
> +	uint8_t da;
> +	pgn_t pgn;
> +} promisc_probes[] = {
> +	{ SA_RX, PGN_B },	/* the bound address, another PGN */
> +	{ SA_OTHER, PGN_A },	/* another address, the bound PGN */
> +};
> +
> +/* SO_J1939_PROMISC delivers frames that do not match the bound (addr, pgn). */
> +TEST_F(j1939_opts, promisc_receive)
> +{
> +	uint8_t tx_buf[CAN_MAX_DLEN];
> +	uint8_t rx_buf[CAN_MAX_DLEN];
> +	int enable = 1;
> +	size_t i;
> +
> +	fill_pattern(tx_buf, sizeof(tx_buf));

[ ... ]

> +	for (i = 0; i < ARRAY_SIZE(promisc_probes); i++) {
> +		ASSERT_TRUE(j1939_sendto_ok(self->tx, tx_buf, sizeof(tx_buf),
> +					    self->ifindex, J1939_NO_NAME,
> +					    promisc_probes[i].da,
> +					    promisc_probes[i].pgn))
> +			TH_LOG("sendto probe %zu failed: %d", i, errno);
> +		EXPECT_EQ(recv_timeout(self->rx, rx_buf, sizeof(rx_buf),
> +				       J1939_NEG_RECV_TIMEOUT_MS), -1)
> +			TH_LOG("probe %zu was received without promisc", i);
> +	}

[ ... ]

> +	for (i = 0; i < ARRAY_SIZE(promisc_probes); i++) {
> +		ASSERT_TRUE(j1939_sendto_ok(self->tx, tx_buf, sizeof(tx_buf),
> +					    self->ifindex, J1939_NO_NAME,
> +					    promisc_probes[i].da,
> +					    promisc_probes[i].pgn))
> +			TH_LOG("sendto probe %zu failed: %d", i, errno);
> +		EXPECT_TRUE(recv_verify_ok(self->rx, tx_buf, sizeof(tx_buf),
> +					   J1939_RECV_TIMEOUT_MS))
> +			TH_LOG("probe %zu not received despite promisc", i);

[Severity: Low]
Can this check confirm that the message it read is really
promisc_probes[i]?

tx_buf is filled once by fill_pattern(), so all four frames carry the same
payload. recv_verify_ok() passes src = NULL, and recvfrom_verify_ok() in
j1939_tests.h only checks the length and the payload:

tools/testing/selftests/net/can/j1939_tests.h:recvfrom_verify_ok() {
    ...
	ok = n == want_len && !memcmp(want, rx_buf, len);
    ...
}

This means a stale or duplicated message in the rx queue can pass the
check for a different probe. For example, on a buggy kernel:

1. The pre-promisc probe 1 ({SA_OTHER, PGN_A}) gets delivered after the
   300 ms J1939_NEG_RECV_TIMEOUT_MS window, so the
   EXPECT_EQ(recv_timeout(...), -1) check still passes.

2. Post-promisc probe 0 then reads that stale probe 1 message and passes.

3. Post-promisc probe 1 reads the real probe 0 and passes, even if
   promisc never delivered probe 1.

Both the leak before promisc and the missing delivery after promisc would
go unnoticed. A kernel that delivers duplicates could hide a missing probe
in the same way.

Under normal timing this does catch the regression the commit message
targets (flag stored but ignored), because every post-promisc
recv_verify_ok() times out.

The v2 notes say the test should "check the address and the PGN filter
each on its own". The kernel reports the PGN in msg_name, and the two
probes use different PGNs. Could the post-promisc loop call
recvfrom_verify_ok() with a struct sockaddr_can and check that
src.can_addr.j1939.pgn matches promisc_probes[i].pgn? Giving each probe
its own payload would work too.

> +	}
> +}

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