can: j1939: serialize cancel and rearm of the receive timer

j1939_tp_set_rxtimeout() cancels the receive timer, takes a reference and starts it again, but its callers do not agree on active_session_list_lock:
the receive handlers call it without the lock, while j1939_session_cancel() is reached from the transmit timer path and used to call it with the lock held.

That makes the sequence non-atomic. If one caller starts the timer after another has cancelled it and before that one starts it again, the second hrtimer_start() reprograms an already armed timer. hrtimer_start() does not report that the timer was armed and does not drop the reference the first caller took, so that reference is never released: the session, and the netdev_hold() reference it carries, leak.

The lock cannot simply be taken around the existing helper, because hrtimer_cancel() waits for a running callback and j1939_tp_rxtimer() takes the same lock. Add j1939_tp_arm_rxtimer_locked(), which uses hrtimer_try_to_cancel() instead: it never waits, so it is safe under the lock, and it returns 1 only when the timer was queued, which is the only case in which the caller's reference has to be dropped.

j1939_tp_set_rxtimeout() now holds the lock around the helper, j1939_session_cancel() calls the helper instead of hand-rolling get + hrtimer_start(), and the rx timeout branch of j1939_tp_rxtimer() does the same, removing a second copy of that sequence.

Every existing caller of j1939_tp_set_rxtimeout() is outside the lock, so the added acquisition does not recurse.

---
 net/can/j1939/transport.c | 66 ++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 18 deletions(-)

index 8fcfd13e5e6f..fa23260e1833 100644
@@ -721,15 +721,42 @@ void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
 		      HRTIMER_MODE_REL_SOFT);
 }
 
-static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
-					  int msec)
+/*
+ * Rearm the receive timer as one step under active_session_list_lock.
+ *
+ * j1939_tp_set_rxtimeout() is called from receive handlers that do not hold
+ * the session list lock, while j1939_session_cancel() is called from the
+ * transmit timer path.  If the cancel and the rearm are not serialized, a
+ * concurrent caller can start the timer after this one cancelled it and
+ * before this one starts it again: hrtimer_start() on an already armed timer
+ * reprograms it without dropping the reference taken by that caller, and the
+ * session leaks together with its netdev reference.
+ *
+ * hrtimer_try_to_cancel() does not wait for a running callback, so it can be
+ * called with the lock held even though j1939_tp_rxtimer() takes the same
+ * lock; a callback that is already running drops its own reference.  For the
+ * same reason the return value is only 1 when the timer was queued: when this
+ * runs from j1939_tp_rxtimer() itself, the timer is no longer queued and 0 is
+ * the expected result.
+ */
+static void j1939_tp_arm_rxtimer_locked(struct j1939_session *session, int msec)
 {
-	j1939_session_rxtimer_cancel(session);
+	if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
+		j1939_session_put(session);
+
 	j1939_session_get(session);
 	hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
 		      HRTIMER_MODE_REL_SOFT);
 }
 
+static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
+					  int msec)
+{
+	j1939_session_list_lock(session->priv);
+	j1939_tp_arm_rxtimer_locked(session, msec);
+	j1939_session_list_unlock(session->priv);
+}
+
 static int j1939_session_tx_rts(struct j1939_session *session)
 {
 	u8 dat[8];

@@ -1161,18 +1188,29 @@ static void __j1939_session_cancel(struct j1939_session *session,
 static void j1939_session_cancel(struct j1939_session *session,
 				 enum j1939_xtp_abort err)
 {
+	bool waiting;
+
 	j1939_session_list_lock(session->priv);
 
+	waiting = session->state == J1939_SESSION_WAITING_ABORT;
 	if (session->state >= J1939_SESSION_ACTIVE &&
 	    session->state < J1939_SESSION_WAITING_ABORT) {
-		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
 		__j1939_session_cancel(session, err);
+		j1939_tp_arm_rxtimer_locked(session, J1939_XTP_ABORT_TIMEOUT_MS);
 	}
 
 	j1939_session_list_unlock(session->priv);
 
 	if (!session->sk)
 		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
+
+	/* The abort already went out on an earlier call, so this one armed no
+	 * timer.  Several callers cancel the timers before calling here, which
+	 * would otherwise leave the session on the list with nothing to finish
+	 * it off, so deactivate it now.
+	 */
+	if (waiting)
+		j1939_session_deactivate_activate_next(session);
 }
 
 static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)

@@ -1279,10 +1317,8 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
 		    session->state < J1939_SESSION_ACTIVE_MAX) {
 			netdev_alert(priv->ndev, "%s: 0x%p: rx timeout, send abort\n",
 				     __func__, session);
-			j1939_session_get(session);
-			hrtimer_start(&session->rxtimer,
-				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
-				      HRTIMER_MODE_REL_SOFT);
+			j1939_tp_arm_rxtimer_locked(session,
+						    J1939_XTP_ABORT_TIMEOUT_MS);
 			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
 		}
 		j1939_session_list_unlock(session->priv);

@@ -1729,16 +1765,10 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
 
 		j1939_session_timers_cancel(session);
 		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
-		if (session->transmission) {
-			j1939_session_deactivate_activate_next(session);
-		} else if (session->state == J1939_SESSION_WAITING_ABORT) {
-			/* Force deactivation for the receiver.
-			 * If we rely on the timer starting in j1939_session_cancel,
-			 * a second RTS call here will cancel that timer and fail
-			 * to restart it because the state is already WAITING_ABORT.
-			 */
-			j1939_session_deactivate_activate_next(session);
-		}
+		/* Both roles: the peer is restarting the transfer, so free the
+		 * slot now rather than wait out the abort timeout.
+		 */
+		j1939_session_deactivate_activate_next(session);
 
 		return -EBUSY;
 	}

2.43.0

Hi Oleksij,

Not a secret at all — and thanks for asking rather than assuming. (My mail client lost the thread id of your message, so I am hanging this off the v8 patch it is about; apologies for that.)

The model is DeepSeek V4.1 Flash (deepseek-v4.1-flash), run through an agentic coding harness rather than a chat window: it reads the tree, builds the file, runs checkpatch and drafts the changelog. The analysis, the judgement calls and the mistakes are mine. v9 carries Assisted-by: LLM on all four patches, which is what Documentation/process/coding-assistants.rst asks for — that tag was missing in v8, and that was my omission.

One thing about your attachment: it did not reach me. What I received has the line "In attachment is an extract from my setup" and nothing attached, so I have not seen it. If it is a config or a prompt/setup you use to get better patches out, please paste it inline and I will read it properly — I would rather learn from your setup than guess at it.

On the first sentence: fixed in v9, all four changelogs now open in the imperative.

[1/4] Cancel and rearm the receive timer as one atomic step under
active_session_list_lock.
[2/4] Validate the packet number in ETP.CM_DPO, which is peer controlled and
currently accepted without any check.
[3/4] Check the skb that j1939_session_completed() hands to j1939_sk_recv(),

        which dereferences oskb->sk.

[4/4] Reject a CTS packet number above pkt.total in j1939_xtp_rx_cts_one().

v9 is out. Code is functionally the same as v8 except one comment in [2/4]; the rest is the changelogs, the missing Fixes/Cc: stable tags and the cover letter.

Thanks also for holding the duplicate patch on the Sun Yiqi thread for this one. I would rather get it right than get it in first.

Chaosheng

v8 addresses the three findings from the sashiko review of v7.

1/4 is rewritten. The v7 version cancelled the receive timer before taking active_session_list_lock and started it again under the lock, so two concurrent callers could both reach hrtimer_start(): the second one reprograms an already armed timer and the reference taken for the first arm is never dropped, leaking the session and its netdev reference. The new j1939_tp_arm_rxtimer_locked() uses hrtimer_try_to_cancel(), which never waits for a running callback, so the cancel and the start happen together under the lock. j1939_tp_set_rxtimeout() and the rx timeout branch of j1939_tp_rxtimer() use the same helper, so there is one arming path instead of three.

2/4 now calls j1939_session_timers_cancel() before aborting on an out-of-range EDPO, as the other receive-path abort handlers do. j1939_session_cancel() only rearms the abort timer, so the transmit timer stayed armed on an aborting session: on the transmitter path the retries kept firing until J1939_XTP_TX_RETRY_LIMIT and overwrote session->err with -ENETUNREACH, on the receiver path they sent CTS or EOMA and clobbered the abort timeout. It also carries the Fixes tag for the NULL dereference.

3/4 and 4/4 are unchanged from v7.

Chaosheng Qu (4):
can: j1939: serialize cancel and rearm of the receive timer
can: j1939: validate the EDPO packet number
can: j1939: drop the message when the reassembled skb is gone
can: j1939: reject a CTS that names more packets than the message has

 net/can/j1939/transport.c | 119 +++++++++++++++++++++++++++++++-------
 1 file changed, 97 insertions(+), 22 deletions(-)

2.43.0

Your cover letter should provide a brief, informative overview of the series. In addition, a brief, concise change log in the form of a bulleted list should be included.

All patches should be a reply to the cover letter.

Consider to switch to "b4" using the "web endpoint" to send your patches:

https://b4.docs.kernel.org/en/latest/contributor/send.html#configuring-the-web-endpoint

regards,
Marc

Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Hi Marc,

All three points were right. v9 is out with them fixed.

The threading one was the worst and it was entirely my fault: I generated the five mails with my own script and never set In-Reply-To, so the four patches went out as four separate threads. They are not separate now — v9 goes through b4 send, the patches are replies to the cover letter, and the series carries a change-id and a base-commit. The hand-rolled script is retired for this series.

The cover letter now has an overview of the two bugs and the two hardening changes, the testing status, and a bulleted change log for v9 and v8.

All four changelogs start in the imperative.

Two things I fixed on my own while rerolling, because they would have cost you review time:

- [1/4] and [4/4] were missing Fixes: 9d71dd0c7009. All four patches fix behaviour that dates from the original J1939 commit, so they all carry the tag now, plus Cc: [email protected].
- [2/4] said the CTS bound was "a separate fix" while [4/4] of the same series does it. That now says where it lives.

Functionally nothing changed since v8 except one comment in [2/4].

Thanks,
Chaosheng