Re: [PATCH can-next v2 5/9] selftests: can: add J1939 addressing and concurrency tests

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · 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] test_j1939_addr.c avoids the kernel's address-claim hold-off
  race only on the success path.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a kernel
  bug that the patch works around rather than fixes.

--

selftests: can: add J1939 addressing and concurrency tests

This adds test_j1939_addr, a selftest for J1939 name based addressing,
address claiming with the kernel's 250 ms hold-off, address claim
conflicts and concurrent TP sessions. The test that loses its address
keeps a socket open until the winning name is mapped, which avoids a hang
when the interface is deleted.

> 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..02f9e504b567f
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_j1939_addr.c
> @@ -0,0 +1,679 @@

[ ... ]

> +TEST_F(j1939_addr, name_addressing)
> +{

[ ... ]

> +	ASSERT_TRUE(forge_address_claim_ok(self->raw, self->ifindex, SA_RX,
> +					   NAME_RX, &t0))
> +		TH_LOG("failed to inject address claim: %d", errno);
> +
> +	self->tx = j1939_open(self->ifindex, SA_TX, J1939_NO_PGN);
> +	ASSERT_GE(self->tx, 0);
> +	ASSERT_TRUE(j1939_connect_name_ok(self->tx, self->ifindex, NAME_RX,
> +					  J1939_NO_ADDR, PGN_TEST))
> +		TH_LOG("connect to peer NAME failed: %d", errno);
> +
> +	ASSERT_TRUE(send_after_claim_ok(self->tx, tx_buf, sizeof(tx_buf), NULL,
> +					&t0, &elapsed_ms))
> +		TH_LOG("send to peer NAME failed after %ld ms: %s",
> +		       elapsed_ms, strerror(errno));

[Severity: Medium]
If one of these fails, does the selftest run hang instead of reporting
the failure?

forge_address_claim_ok() has just caused the kernel to arm the 250 ms
ac_timer for NAME_RX. If j1939_open() or j1939_connect_name_ok() fails
here, the ASSERT_GE()/ASSERT_TRUE() ends the test. Then
FIXTURE_TEARDOWN(j1939_addr) closes tx, rx and raw right away, so the last
J1939 socket on the vcan closes while the timer is still pending.

The commit message describes what happens next:

  If the last socket closes while the hold-off timer of a claim is still
  pending, the kernel keeps a reference to the interface and deleting it
  hangs.

So the ip link delete in cleanup() in j1939_lib.sh would block forever. A
regression in connecting by name, which this test is meant to catch,
would then look like a hang and not like a test failure.

In strict mode, send_after_claim_ok() seems to leave the same window
open. It gives up once elapsed_ms is past aclaim_max_ms():

	if (*elapsed_ms > max_ms) {
		if (ret == want_len)
			errno = ETIMEDOUT;
		return false;
	}

With J1939_STRICT_TIMING that limit is 300 ms after t0. The kernel only
arms its 250 ms timer when it processes the claim, which is after t0, and
the soft hrtimer can fire later still.

The same thing looks possible in a few other places:

  address_claim_send(): send_data_after_claim_ok() fails in strict mode
  while the NAME_CLAIM timer is still pending.

  ac_conflict_probe_ok(): the baseline send_data_after_claim_ok() returns
  false in the same way, and the caller's ASSERT_TRUE() tears down at once.

  ac_conflict_lose(): ASSERT_GE(self->rx, 0) aborts before the wait for
  NAME_LOWER, or the strict-mode send_after_claim_ok() times out while
  that timer is still pending.

Could every exit path wait out the hold-off, for example by keeping a
J1939 socket open until ACLAIM_MAX_MS after the last claim, before the
sockets are closed?

[ ... ]

> +/* A higher-priority competing claim takes the address away from us. */
> +TEST_F(j1939_addr, ac_conflict_lose)
> +{

[ ... ]

> +	/*
> +	 * Keep the sockets open until the kernel maps NAME_LOWER at the end
> +	 * of its hold-off. If the last socket closes while that timer is
> +	 * pending, the kernel keeps a reference to the interface and
> +	 * deleting it hangs.
> +	 */
> +	self->rx = j1939_open(self->ifindex, SA_RX, J1939_NO_PGN);
> +	ASSERT_GE(self->rx, 0);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. The
hang that this comment works around looks like a netdev reference leak in
the kernel. Should it be fixed in net/can/j1939 instead of being avoided
in the test?

When the last J1939 socket closes, j1939_netdev_stop() ends up in
__j1939_rx_release():

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

j1939_ecu_unmap_all() only unmaps. Nothing on this path calls
j1939_ecu_timer_cancel(), whose only caller is j1939_ac_process().

If a timer armed by j1939_ecu_timer_start() fires after this, the handler
maps the ECU again:

net/can/j1939/bus.c:j1939_ecu_timer_handler() {
	write_lock_bh(&priv->lock);
	...
	j1939_ecu_map_locked(ecu);
	...
	j1939_ecu_put(ecu);
	write_unlock_bh(&priv->lock);
}

j1939_ecu_map_locked() takes a new ECU reference, and the handler drops
only the reference the timer held. The ECU then stays mapped in a priv
that is no longer attached to the netdev.

j1939_netdev_notify() looks up priv with j1939_priv_get_by_ndev(), which
now returns NULL. So NETDEV_DOWN and NETDEV_UNREGISTER never unmap the
ECU, and __j1939_ecu_release() never runs. That leaks
ecu->priv_dev_tracker, the priv reference and priv->dev_tracker, and
unregister_netdevice() waits forever.

The claim can come from a remote ECU on the bus as well as from a local
socket. Creating CAN sockets needs no capability. So it looks like an
unprivileged user can make the interface undeletable by closing the last
J1939 socket within 250 ms of a claim.

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