Re: [PATCH v3] can: j1939: fix potential race condition in BAM segmentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: j1939: session reference leak on concurrent txtimer scheduling
--

commit 8433158f9fc2fee1a98accad978bba5e01cfc621
Author: Alexander Hölzl <alexander.hoelzl@gmx.net>

can: j1939: fix potential race condition in BAM segmentation

This patch addresses a race condition in segmented BAM messages by moving
the tx timer scheduling such that it is scheduled when the tx ack is
received. This aims to prevent session stalls when a BAM frame is stuck and
fixes the behavior for consecutive data frames.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..c61a021880c01 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
[ ... ]
> @@ -1961,6 +1974,12 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
>  	} else if (remain) {
>  		if (!session->transmission)
>  			j1939_tp_set_rxtimeout(session, 750);
> +		else if (j1939_cb_is_broadcast(&session->skcb)) {
> +			/* cancel the txtimer to avoid leaking session references */
> +			j1939_session_txtimer_cancel(session);
> +			j1939_tp_schedule_txtimer(session, J1939_TP_BAM_FRAME_SPACING_MS);
> +			j1939_tp_set_rxtimeout(session, J1939_TP_BAM_ECHO_TIMEOUT_MS);
> +		}
>  	} else if (do_cts_eoma) {
>  		j1939_tp_set_rxtimeout(session, 1250);
>  		if (!session->transmission)
>  			j1939_tp_schedule_txtimer(session, 0);

[Severity: High]
Does this do_cts_eoma path leak session references when multiple packets are
processed rapidly?

When a receiver session processes data packets in j1939_xtp_rx_dat_one(), it
evaluates do_cts_eoma = true if pkt.rx >= pkt.last. Because pkt.last is
initialized to 0 and never updated for receiver sessions, this condition is
true for every data packet.

Consequently, j1939_tp_schedule_txtimer(session, 0) is called for every
packet without first cancelling any pending timer.

Since j1939_tp_schedule_txtimer() unconditionally calls
j1939_session_get(session) before starting the timer, calling it multiple
times before the timer fires (e.g. in a NAPI batch) will re-arm the timer
and leak the extra references, as the timer callback only drops a single
reference.

Should this path also call j1939_session_txtimer_cancel(session) first,
similar to the new block added above for remain?

>  	} else {

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