Re: [PATCH can-next v3 5/9] selftests: can: add J1939 addressing and concurrency 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] The ACLAIM_* block comment in
  tools/testing/selftests/net/can/test_j1939_addr.c (lines 99-100) says:…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  depends on it and works around it.

--

selftests: can: add J1939 addressing and concurrency tests

This commit adds a J1939 selftest. It covers NAME-based addressing, the
Address Claimed frame and its 250 ms hold-off, address claim conflicts,
and a stress run of concurrent TP sessions. To avoid a hang when the
interface is deleted, the tests keep a socket open until the hold-off
timer has run.

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

[ ... ]

> +/*
> + * The kernel lets a NAME use a claimed address only ACLAIM_HOLDOFF_MS after
> + * the Address Claimed frame (j1939_ecu_timer_start() in
> + * net/can/j1939/bus.c). Before that, a send from or to that NAME fails with
> + * EADDRNOTAVAIL.
> + *
> + * The first send must succeed no later than ACLAIM_MAX_MS (strict:
> + * ACLAIM_STRICT_MAX_MS). A defended address must stay ours, and the
> + * contender must get none, until that latest time. The tests that time the
> + * first send also check that it does not succeed before ACLAIM_MIN_MS.

[Severity: Low]
Is this comment accurate? Only name_addressing and address_claim_send
check EXPECT_GE(elapsed_ms, ACLAIM_MIN_MS).

ac_conflict_lose also times the first send to NAME_LOWER after the forged
claim with send_after_claim_ok(). It only uses elapsed_ms in TH_LOG():

	EXPECT_TRUE(send_after_claim_ok(self->rx, data, sizeof(data), &winner,
					&forge_t0, &elapsed_ms))
		TH_LOG("send to NAME_LOWER failed after %ld ms: %s",
		       elapsed_ms, strerror(errno));

The baseline send_data_after_claim_ok() in ac_conflict_probe_ok() has no
lower bound check either.

The kernel makes the winning NAME reachable only once the hold-off timer
fires, so a lower bound check in ac_conflict_lose would be valid. As
written, a kernel that maps the winning contender right away would still
pass ac_conflict_lose.

Should the comment be reworded, or should ac_conflict_lose add an
EXPECT_GE(elapsed_ms, ACLAIM_MIN_MS) check?

> + * ACLAIM_MARGIN_MS is a guess.
> + */

[ ... ]

> +FIXTURE_TEARDOWN(j1939_addr)
> +{
> +	/*
> +	 * A failed test can stop while the hold-off timer of a claim is
> +	 * pending (see ac_conflict_lose). The teardown does not know when the
> +	 * claim started, so wait as long as a claim may take.
> +	 */
> +	/* POSIX allows usleep() to fail with EINVAL for 1 s or more */
> +	if (_metadata->exit_code != KSFT_PASS)
> +		poll(NULL, 0, ACLAIM_MAX_MS);

[Severity: High]
This isn't a bug introduced by this patch, but this teardown works around
a leak in net/can/j1939. Because of the workaround, the suite cannot catch
that leak.

j1939_ac_process() arms the hold-off timer and then drops its own
reference. During the hold-off, only ecu->ac_timer holds the new ECU:

net/can/j1939/address-claim.c:j1939_ac_process() {
    ...
	j1939_ecu_timer_start(ecu);
 out_ecu_put:
	j1939_ecu_put(ecu);
}

When the last socket closes, __j1939_rx_release() tears down the priv but
does not cancel a pending ac_timer:

net/can/j1939/main.c:__j1939_rx_release() {
    ...
	j1939_can_rx_unregister(priv);
	j1939_ecu_unmap_all(priv);
	j1939_priv_set(priv->ndev, NULL);
    ...
}

j1939_ecu_unmap_all() only walks priv->ents[], so it skips an ECU that is
still in its hold-off:

	for (i = 0; i < ARRAY_SIZE(priv->ents); i++)
		if (priv->ents[i].ecu)
			j1939_ecu_unmap_locked(priv->ents[i].ecu);

When the timer fires later, j1939_ecu_timer_handler() maps the ECU
without any check. That takes a new mapping reference:

	write_lock_bh(&priv->lock);
	j1939_ecu_map_locked(ecu);
	j1939_ecu_put(ecu);
	write_unlock_bh(&priv->lock);

Nothing unmaps the ECU after that. The priv is already detached from the
ndev, so j1939_netdev_notify() returns early on NETDEV_UNREGISTER:

	priv = j1939_priv_get_by_ndev(ndev);
	if (!priv)
		goto notify_done;

The ECU keeps its netdev_hold() and j1939_priv_get(), and the priv keeps
its own netdev_hold(). unregister_netdevice() then waits forever.

can_create() and J1939 bind/sendto check no capabilities. Can a local
unprivileged user trigger this by sending an Address Claimed frame and
closing the socket within 250 ms? A claim from a remote ECU that arrives
just before the last local socket closes looks like it would take the same
path.

This poll(), and ac_conflict_lose keeping a socket open until NAME_LOWER
is mapped, keep the suite out of that window. The suite therefore passes
on a kernel that has the bug. The commit notes say "the fix belongs in the
stack". Could a test that closes the last socket during the hold-off and
then deletes the interface be added once the stack is fixed?

> +
> +	if (self->tx >= 0)
> +		close(self->tx);

[ ... ]

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