Re: [PATCH v4 2/3] j1939: fix implementation not handling holds correctly

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected concurrent access and modification of J1939 session state variables (`pkt.tx`) between the RX softirq and the TX timer softirq, leading to potential silent data corruption.
--

--- Patch [2]: [PATCH v4 2/3] j1939: fix implementation not handling holds correctly ---
commit eff7a7e71fc39cb6fe94b38a7c7c2264d3d9f7e4
Author: Alexander Hölzl <alexander.hoelzl@gmx.net>

j1939: fix implementation not handling holds correctly

This commit modifies the behavior of j1939_xtp_rx_cts_one to allow the
handling of a hold message. It cancels the TX timer and sets the RX
timeout properly while adding a check against requests of already
transferred and acked packets.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 6f999b18bbca5..ae0dc0a0168aa 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -1488,19 +1530,13 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>  	/* TODO: do not set tx here, do it in txtimer */
>  	session->pkt.tx = session->pkt.tx_acked;

[Severity: High]
This is a pre-existing issue, but I have a question about the concurrent
access to session->pkt.tx.

Can modifying session->pkt.tx here in the RX softirq context race with
readers in the TX timer softirq context?

If the TX timer executes j1939_session_tx_dat() concurrently, it reads
session->pkt.tx multiple times within its transmission loop without locking
or READ_ONCE():

    dat[0] = session->pkt.tx - session->pkt.dpo + 1;
    offset = (session->pkt.tx * 7) - se_skcb->offset;

Because these softirqs can run concurrently on different CPUs, might the
compiler reload session->pkt.tx between the two statements? 

If the RX path overwrites pkt.tx in that microscopic window, could
j1939_session_tx_dat() transmit a frame containing the payload data of one
packet but the sequence number of another, leading to silent data corruption
on the CAN network?

> -	session->last_cmd = dat[0];
> -	if (dat[1]) {
> -		j1939_tp_set_rxtimeout(session, 1250);
> -		if (session->transmission) {
> -			if (session->pkt.tx_acked)
> -				j1939_sk_errqueue(session,
> -						  J1939_ERRQUEUE_TX_SCHED);
> -			j1939_session_txtimer_cancel(session);
> -			j1939_tp_schedule_txtimer(session, 0);
> -		}
> -	} else {
> -		/* CTS(0) */
> -		j1939_tp_set_rxtimeout(session, 550);
> +	j1939_tp_set_rxtimeout(session, 1250);
> +	if (session->transmission) {
> +		if (session->pkt.tx_acked)
> +			j1939_sk_errqueue(session,
> +						J1939_ERRQUEUE_TX_SCHED);
> +		j1939_session_txtimer_cancel(session);
> +		j1939_tp_schedule_txtimer(session, 0);
>  	}
>  	return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706224548.100451-1-alexander.hoelzl@gmx.net?part=2