Re: [PATCH 2/2] can: j1939: check received packet count before completing session

[sashiko-bot@kernel.org]

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

New issues:
- [High] Canceling session timers during an already-aborting session permanently leaks the session and blocks the address.
- [Medium] The patch inadvertently suppresses the pre-existing diagnostic warning for EOMA size mismatches on transmitter sessions.

Pre-existing issues:
- [High] AB-BA deadlock between `active_session_list_lock` and the `rxtimer` hrtimer callback.
--

commit 66292f8eec0fad08cd8484aafc6afd0d99105fbf
Author: Liu Chao <liuc63@xiaopeng.com>

can: j1939: check received packet count before completing session

This commit adds a packet receive count check in j1939_xtp_rx_eoma_one() to
abort sessions with missing data packets instead of delivering short messages.
It also tightens the EOMA size mismatch warning to abort receive sessions.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fdce57924042..2ecce49ada40a 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1409,11 +1410,23 @@ j1939_xtp_rx_eoma_one(struct j1939_session *session, struct sk_buff *skb)
>  	else
>  		len = j1939_tp_ctl_to_size(dat);
>  
> -	if (session->total_message_size != len) {
> +	if (!session->transmission && session->total_message_size != len) {

[Severity: Medium]
Does this inadvertently suppress the diagnostic warning for EOMA size
mismatches on transmitter sessions? By adding the !session->transmission
condition here, protocol violations (a receiver sending an EOMA with an
incorrect size) are now silently ignored on the transmitter instead of
logging a warning. Could we keep the warning for transmitters but only
bypass the newly-added abort?

> +		netdev_warn_once(session->priv->ndev,
> +				 "%s: 0x%p: EOMA size mismatch, expected %i got %i\n",
> +				 __func__, session, session->total_message_size,
> +				 len);
> +		goto out_session_cancel;
> +	}

[ ... ]

>  	j1939_session_timers_cancel(session);
>  	/* transmitted without problems */
>  	j1939_session_completed(session);
> +	return;
> +
> +out_session_cancel:
> +	j1939_session_timers_cancel(session);
> +	j1939_session_cancel(session, J1939_XTP_ABORT_FAULT);
>  }

[Severity: High]
Could this error path cause a permanent leak of the session structure
and permanently block the SA/DA address pair?

If a receive session times out or is aborted, it enters the
J1939_SESSION_WAITING_ABORT state with a timer scheduled to eventually
deactivate it. If a malformed EOMA arrives while waiting, this path will
call j1939_session_timers_cancel() (which cancels the deactivation timer).

Then, j1939_session_cancel() is called:

j1939_session_cancel() {
    ...
    if (session->state >= J1939_SESSION_ACTIVE &&
        session->state < J1939_SESSION_WAITING_ABORT) {
        j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
    ...
}

Because the session is already in J1939_SESSION_WAITING_ABORT, the
condition fails and the deactivation timer is never restarted. The session
remains in the active_session_list indefinitely.


[Severity: High]
This is a pre-existing issue, but looking at j1939_session_cancel() exposed
by this path, could it trigger an AB-BA deadlock?

j1939_session_cancel() takes the list lock, then cancels the rx timer:

j1939_session_cancel() {
    j1939_session_list_lock(session->priv);
    ...
    j1939_tp_set_rxtimeout(session, ...); // internally calls hrtimer_cancel
}

Meanwhile, the timer callback attempts to acquire the same list lock:

j1939_tp_rxtimer() {
    ...
    j1939_session_list_lock(session->priv);
}

If j1939_session_cancel() is called exactly when the session's receive
timeout expires, the CPU executing j1939_session_cancel() would spin
waiting for the timer to finish, while the CPU executing j1939_tp_rxtimer()
spins waiting for the list lock, resulting in a deadlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907145640.1106170-1-liuc63@xiaopeng.com?part=2