[PATCH net v8 0/4] can: j1939: fix the abort timer and bound EDPO

[PATCH net v8 0/4] can: j1939: fix the abort timer and bound EDPO

[Chaosheng Qu]

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

[PATCH net v8 1/4] can: j1939: serialize cancel and rearm of the receive timer

[Chaosheng Qu]

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.

Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 66 ++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..fa23260e1833 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -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

Re: [PATCH net v8 1/4] can: j1939: serialize cancel and rearm of the receive timer

[Chaosheng Qu]

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

Re: [PATCH net v8 0/4] can: j1939: fix the abort timer and bound EDPO

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On 11.11.2026 15:20:00, Chaosheng Qu wrote:
> v8 addresses the three findings from the sashiko review of v7.

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net v8 0/4] can: j1939: fix the abort timer and bound EDPO

[Chaosheng Qu]

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: stable@vger.kernel.org`.
- [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