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

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

[Chaosheng Qu]

This series fixes two bugs in the j1939 transport layer, plus two small
hardening changes that fall out of them.

The first bug is a timer reference leak.  The receive timer is cancelled and
restarted from three places, and they do not agree on
active_session_list_lock: the receive handlers call the helper without the
lock, j1939_session_cancel() called it with the lock held, and the rx timeout
branch of j1939_tp_rxtimer() open-coded the same sequence.  Two callers can
interleave between the cancel and the start, and the second hrtimer_start()
reprograms an already armed timer without dropping the reference the first
caller took, so the session and its netdev_hold() reference leak.  [1/4]
replaces the three copies with one helper that cancels with
hrtimer_try_to_cancel() under the lock, so the cancel and the start are one
step.

The second bug is a NULL dereference driven by a peer.  ETP.CM_DPO is peer
controlled and its packet number is not validated; a value at or past
pkt.total moves pkt.dpo past the reassembled buffer, the transfer still
completes, and the local loopback of the receiver's own EOMA hands a NULL
skb to j1939_sk_recv(), which dereferences oskb->sk.  [2/4] rejects that
EDPO.  [3/4] checks the skb in j1939_session_completed() anyway, so a future
regression in the offset arithmetic degrades to a dropped message instead of
a panic.  [4/4] rejects an oversized CTS where it is parsed, instead of
letting it come back one step later as a bad EDPO offset.

Testing: [2/4] was verified on a local vcan link -- ETP.CM_RTS with a size of
1786 bytes (256 packets), then ETP.CM_DPO packet 256 and an ETP.DT with
sequence 0, dereferenced a NULL skb 3/3 times before the bound check and was
rejected 3/3 times after it.  The seq 0 case is the one that made
j1939_xtp_rx_dat_one() compute the last in-order packet, so the transfer
completed and the receiver's own EOMA came back through the loopback, as
described in [2/4].  For the timer side: two unparsable ETP.DT frames
followed by "ip link del" left unregister_netdevice() waiting for the session
reference; with the abort timer rearmed the session is deactivated about
500 ms after the second frame.  Each patch builds standalone (net/can/j1939
built in, x86_64) and passes scripts/checkpatch.pl with no errors or
warnings.  I have no j1939 hardware, so none of this ran on a real CAN bus,
and the race in [1/4] is argued from the lock ordering rather than caught in
the act.

Changes in v9:

- Send the patches as replies to the cover letter; in v8 they went out as
  four separate threads.
- Start the first sentence of every changelog in the imperative.
- Add the Assisted-by tag required by
  Documentation/process/coding-assistants.rst, which v8 was missing.
- Add the missing Fixes tags and Cc: stable for [1/4] and [4/4]; all four
  patches fix behaviour that dates from the original J1939 commit.
- Say in [2/4] where the CTS bound lives, instead of calling it a separate
  fix while [4/4] of the same series does it.
- Give this cover letter an overview of the series and this changelog.

Functionally the patches are unchanged since v8; the only code text that
changed is one comment in [2/4].

Changes in v8:

- [1/4] rewritten: the v7 version armed the timer after cancelling it outside
  the lock, which left the race it was meant to close.
- [2/4] cancels the session timers before aborting, and carries the Fixes tag
  for the NULL dereference.
- [3/4] and [4/4] 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(-)
---
base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e
change-id: 20261111-j1939-edpo-v9-daeeb60af993

Best regards,
--  
Chaosheng Qu <quchaosheng000406@163.com>

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

[Chaosheng Qu]

From: Chaosheng Qu <quchaosheng000406@163.com>

Cancel and rearm the receive timer as one atomic step under
active_session_list_lock.  j1939_tp_set_rxtimeout() cancels the timer,
takes a reference and starts it again, but its callers do not agree on that
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.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Assisted-by: LLM
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 8fcfd13e5..fa23260e1 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

[PATCH net v9 2/4] can: j1939: validate the EDPO packet number

[Chaosheng Qu]

From: Chaosheng Qu <quchaosheng000406@163.com>

Validate the packet number in ETP.CM_DPO, which is peer controlled and
currently accepted without any check.  It names the packet a transmission
window starts at, and j1939_xtp_rx_dat_one() places incoming data at
"dat[0] - 1 + pkt.dpo", so a value past the end of the message moves
pkt.dpo beyond the reassembled buffer.

The transfer still completes normally in that case.  With
pkt.dpo == pkt.total, a closing ETP.DT frame with dat[0] == 0 evaluates to
packet pkt.total - 1, which is the last in-order packet, so pkt.rx reaches
pkt.total and the receiver emits its own EOMA.  The local loopback of that
EOMA runs j1939_session_completed(), which asks j1939_session_skb_get()
for offset "pkt.dpo * 7" -- one full packet past the end of the queued
skb -- gets NULL, and passes it straight to j1939_sk_recv(), which
dereferences oskb->sk.

Reject a packet number at or past pkt.total and abort the session.  The
check has to be ">=" and not ">": pkt.total is a packet count, so the last
valid packet is pkt.total - 1.  The abort code depends on the role: a
receiver rejects a bad offset with J1939_XTP_ABORT_BAD_EDPO_OFFSET, while a
transmitter only builds an EDPO from pkt.tx_acked, which comes from the
responder's ETP.CM_CTS, so an out-of-range value there is an oversized CTS
and is reported as J1939_XTP_ABORT_ECTS_TOO_BIG.

Cancel the session timers before aborting, as the other receive-path abort
handlers do.  j1939_session_cancel() only rearms the abort timer and
leaves session->txtimer armed, so on the transmitter path the retry timer
keeps firing on an aborting session, retries up to J1939_XTP_TX_RETRY_LIMIT
and overwrites session->err with -ENETUNREACH; on the receiver path the
retries transmit CTS or EOMA and clobber the abort timeout.

The CTS packet number that feeds the transmitter-side EDPO is bounded in
4/4, on its own, because it is a different input parsed in a different
function; this patch only stops an EDPO value from being used as an
offset.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index fa23260e1..258009182 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1853,14 +1853,42 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,
 				 struct sk_buff *skb)
 {
 	const u8 *dat = skb->data;
+	unsigned int dpo;
 
 	if (j1939_xtp_rx_cmd_bad_pgn(session, skb))
 		return;
 
 	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
 
+	dpo = j1939_etp_ctl_to_packet(dat);
+
+	/* EDPO is peer controlled.  j1939_xtp_rx_dat_one() places data at
+	 * "dat[0] - 1 + pkt.dpo", so a value at or past pkt.total moves
+	 * pkt.dpo past the reassembled buffer, and the ETP completion path
+	 * then looks up an offset no queued skb covers.
+	 *
+	 * This bounds the receiver-side writer only.  j1939_session_tx_dpo()
+	 * copies pkt.tx_acked, which comes from an ETP.CM_CTS packet number
+	 * that the responder controls, and 4/4 bounds it where it is parsed.
+	 */
+	if (dpo >= session->pkt.total) {
+		netdev_warn(session->priv->ndev,
+			    "%s: 0x%p: EDPO %u out of range (total %u), aborting\n",
+			    __func__, session, dpo, session->pkt.total);
+		j1939_session_timers_cancel(session);
+		/* The transmitter only sees this on the echo of an EDPO it built
+		 * from an oversized ETP.CM_CTS packet number, so it is the
+		 * responder's CTS that is too big, not the EDPO offset.
+		 */
+		j1939_session_cancel(session,
+				     session->transmission ?
+				     J1939_XTP_ABORT_ECTS_TOO_BIG :
+				     J1939_XTP_ABORT_BAD_EDPO_OFFSET);
+		return;
+	}
+
 	/* transmitted without problems */
-	session->pkt.dpo = j1939_etp_ctl_to_packet(skb->data);
+	session->pkt.dpo = dpo;
 	session->last_cmd = dat[0];
 	j1939_tp_set_rxtimeout(session, 750);
 

-- 
2.43.0

[PATCH net v9 3/4] can: j1939: drop the message when the reassembled skb is gone

[Chaosheng Qu]

From: Chaosheng Qu <quchaosheng000406@163.com>

Check the skb that j1939_session_completed() hands to j1939_sk_recv(),
which dereferences oskb->sk.  A NULL here is the last step of the EDPO
out-of-range sequence fixed by the previous patch; the bounds check makes
the lookup succeed, but the dereference should not depend on that.

Drop the message instead, which is what the other caller of
j1939_session_skb_get(), j1939_simple_txnext(), already does.

Assisted-by: LLM
Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 258009182..fc2840f33 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1281,9 +1281,17 @@ static void j1939_session_completed(struct j1939_session *session)
 
 	if (!session->transmission) {
 		se_skb = j1939_session_skb_get(session);
-		/* distribute among j1939 receivers */
-		j1939_sk_recv(session->priv, se_skb);
-		consume_skb(se_skb);
+
+		/* Defense in depth: pkt.dpo is range checked when an EDPO is
+		 * received, so the lookup should always find the skb.  Drop
+		 * the message rather than hand a NULL skb to j1939_sk_recv()
+		 * if the offset arithmetic ever regresses.
+		 */
+		if (se_skb) {
+			/* distribute among j1939 receivers */
+			j1939_sk_recv(session->priv, se_skb);
+			consume_skb(se_skb);
+		}
 	}
 
 	j1939_session_deactivate_activate_next(session);

-- 
2.43.0

[PATCH net v9 4/4] can: j1939: reject a CTS that names more packets than the message has

[Chaosheng Qu]

From: Chaosheng Qu <quchaosheng000406@163.com>

Reject a CTS packet number above pkt.total in j1939_xtp_rx_cts_one().
The function rejects a zero packet number and a window above pkt.block,
but not a packet number above pkt.total.  It then stores "pkt - 1" in
pkt.tx_acked, and the transmitter reports that value back in an
ETP.CM_DPO.

That value is what j1939_xtp_rx_dpo_one() now bounds, so an oversized CTS
is currently caught one step later and reported as a bad EDPO offset.
Reject it where it originates instead, with the abort code that describes
it: J1939_XTP_ABORT_ECTS_TOO_BIG, "ECTS requested packets exceeds message
size".

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index fc2840f33..377991d36 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1512,6 +1512,15 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
 		goto out_session_cancel;
 	else if (dat[1] > session->pkt.block /* 0xff for etp */)
 		goto out_session_cancel;
+	else if (pkt > session->pkt.total) {
+		/* The responder asks for more packets than the message has.
+		 * pkt - 1 becomes pkt.tx_acked below, and the transmitter
+		 * reports that value in an ETP.CM_DPO, where it would name a
+		 * packet past the end of the message.
+		 */
+		err = J1939_XTP_ABORT_ECTS_TOO_BIG;
+		goto out_session_cancel;
+	}
 
 	/* set packet counters only when not CTS(0) */
 	session->pkt.tx_acked = pkt - 1;

-- 
2.43.0

Re: [PATCH net v9 4/4] can: j1939: reject a CTS that names more packets than the message has

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: j1939: j1939_xtp_rx_cts_one() rejects standard CTS(0)
  frames
--

commit f78a7d16999ed9ed258185b1a1c2d5e0047f0b04
Author: Chaosheng Qu <quchaosheng000406@163.com>

can: j1939: reject a CTS that names more packets than the message has

This commit aborts a J1939 session with J1939_XTP_ABORT_ECTS_TOO_BIG in
j1939_xtp_rx_cts_one() when the received CTS packet number exceeds
session->pkt.total.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index fc2840f334b5b..377991d36dc6a 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1512,6 +1512,15 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>  		goto out_session_cancel;
>  	else if (dat[1] > session->pkt.block /* 0xff for etp */)
>  		goto out_session_cancel;
> +	else if (pkt > session->pkt.total) {
> +		/* The responder asks for more packets than the message has.
> +		 * pkt - 1 becomes pkt.tx_acked below, and the transmitter
> +		 * reports that value in an ETP.CM_DPO, where it would name a
> +		 * packet past the end of the message.
> +		 */
> +		err = J1939_XTP_ABORT_ECTS_TOO_BIG;
> +		goto out_session_cancel;
> +	}
>  
>  	/* set packet counters only when not CTS(0) */
>  	session->pkt.tx_acked = pkt - 1;

[Severity: Medium]
Does this regression reject standard CTS(0) connection-hold frames?

Per SAE J1939-21 and ISO 11783-6, a CTS(0) frame (dat[1] == 0) sets the
next packet number field to 0xff for TP or 0xffffff for ETP.  Because this
check runs unconditionally even when dat[1] is 0, any session with
session->pkt.total smaller than 0xff (TP) or 0xffffff (ETP) will abort with
J1939_XTP_ABORT_ECTS_TOO_BIG upon receiving a CTS(0) instead of reaching
the CTS(0) timeout handler further down in j1939_xtp_rx_cts_one():

net/can/j1939/transport.c:j1939_xtp_rx_cts_one() {
	...
	if (dat[1]) {
		...
	} else {
		/* CTS(0) */
		j1939_tp_set_rxtimeout(session, 550);
	}
}

Also note that updating session->pkt.tx_acked, session->pkt.last, and
session->pkt.tx right below the new check when dat[1] == 0 contradicts the
"set packet counters only when not CTS(0)" comment unless both the new
bounds check and the packet counter updates are gated on dat[1] != 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-j1939-edpo-v9-v9-1-e779ca334a81@163.com?part=4

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] can: j1939: session ref leak in j1939_tp_arm_rxtimer_locked()
- [Medium] can: j1939: armed rxtimer left on deactivated session in
  cancel paths
- [Medium] can: j1939: race on session->state in j1939_tp_rxtimer()
--

commit 1460925f6781f194bfa61b943a4417a2108a6cee
Author: Chaosheng Qu <quchaosheng000406@163.com>

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

This commit introduces j1939_tp_arm_rxtimer_locked() to cancel and rearm
session->rxtimer under active_session_list_lock using
hrtimer_try_to_cancel(), and updates session cancellation and RTS handling
paths.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..fa23260e1833f 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)
> +{
> +	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);
>  }

[Severity: High]
Can j1939_tp_arm_rxtimer_locked() still leak a session reference when
session->rxtimer is armed more than once while j1939_tp_rxtimer() is
running?

kernel/time/hrtimer.c:hrtimer_try_to_cancel() {
    ...
	if (!hrtimer_callback_running(timer)) {
		ret = remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE);
		if (ret)
			trace_hrtimer_cancel(timer);
	}
    ...
}

__run_hrtimer() keeps base->running set to &session->rxtimer for the entire
duration of j1939_tp_rxtimer() until after the callback returns, so
hrtimer_try_to_cancel() skips remove_hrtimer() and returns -1 even if
session->rxtimer has already been re-enqueued.

CPU0                              CPU1
j1939_tp_rxtimer()
                                  j1939_tp_set_rxtimeout()
                                    j1939_session_list_lock()
                                    j1939_tp_arm_rxtimer_locked()
                                      hrtimer_try_to_cancel() -> -1
                                      j1939_session_get(session)
                                      hrtimer_start(&session->rxtimer, ...)
                                    j1939_session_list_unlock()
  j1939_session_list_lock()
  j1939_tp_arm_rxtimer_locked()
    hrtimer_try_to_cancel() -> -1
    j1939_session_get(session)
    hrtimer_start(&session->rxtimer, ...)  <-- leaks CPU1's session ref

Because hrtimer_try_to_cancel() returns -1 on CPU0 while the callback is
running, j1939_session_put() is skipped and hrtimer_start() reprograms the
queued timer in place without releasing the first reference, permanently
leaking the session and its netdev_hold() reference.

[ ... ]

> @@ -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);

[Severity: Medium]
Can j1939_tp_rxtimer() race with j1939_session_cancel() on session->state now
that j1939_session_cancel() no longer calls hrtimer_cancel(&session->rxtimer)
before setting J1939_SESSION_WAITING_ABORT?

CPU0                              CPU1
j1939_tp_rxtimer()
  if (session->state ==
      J1939_SESSION_WAITING_ABORT)
    -> false
                                  j1939_session_cancel()
                                    j1939_session_list_lock()
                                    __j1939_session_cancel()
                                      session->state =
                                        J1939_SESSION_WAITING_ABORT
                                    j1939_tp_arm_rxtimer_locked()
                                    j1939_session_list_unlock()
  j1939_session_list_lock()
  if (session->state >= J1939_SESSION_ACTIVE &&
      session->state < J1939_SESSION_ACTIVE_MAX)
    __j1939_session_cancel()  <-- cancels already-aborted session again

Because J1939_SESSION_WAITING_ABORT is less than J1939_SESSION_ACTIVE_MAX,
CPU0 sends a second abort, overwrites session->err with
J1939_XTP_ABORT_TIMEOUT, and rearms session->rxtimer while the callback is
running.  If CPU1 instead finishes before CPU0's unlocked check at the top of
j1939_tp_rxtimer(), CPU0 sees J1939_SESSION_WAITING_ABORT on the old timer's
expiration and immediately deactivates the session while the new abort timer
remains queued.

> @@ -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;
>  	}

[Severity: Medium]
Does deactivating the session in j1939_xtp_rx_rts_session_active() and
j1939_session_cancel() leave session->rxtimer armed after the session is
removed from priv->active_session_list?

When session->last_cmd != 0 on an active transmitter session,
j1939_session_cancel() transitions the session to J1939_SESSION_WAITING_ABORT
and arms session->rxtimer for J1939_XTP_ABORT_TIMEOUT_MS, and
j1939_session_deactivate_activate_next() then immediately sets session->state
to J1939_SESSION_DONE and removes it from priv->active_session_list without
cancelling that timer.  The same happens when j1939_session_cancel() sees
waiting == true via callers such as j1939_tp_txtimer() that do not cancel
session->rxtimer beforehand, or when another CPU arms session->rxtimer between
j1939_session_timers_cancel() and j1939_session_list_lock().  Because the
deactivated session is no longer on priv->active_session_list,
j1939_cancel_active_session() cannot cancel the timer during socket close or
netdev teardown, and when the timer expires on an RX session (!session->sk) in
J1939_SESSION_DONE it falls into the else branch of j1939_tp_rxtimer() and
emits a duplicate J1939_ERRQUEUE_RX_ABORT notification.

Also, the commit message does not mention these state-machine changes in
j1939_session_cancel() and j1939_xtp_rx_rts_session_active(), and says
j1939_session_cancel() previously hand-rolled get + hrtimer_start() when it
actually called j1939_tp_set_rxtimeout().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-j1939-edpo-v9-v9-1-e779ca334a81@163.com?part=1

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 3 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The rxtimer can still be armed twice, so the session reference
  still leaks.
- [Medium] The patch makes functional changes the commit message does not
  mention, and one statement in the message is wrong.
- [Medium] j1939_session_cancel() now always deactivates a session that
  is already in J1939_SESSION_WAITING_ABORT, whether or not the caller…
- [Medium] can: j1939: race on session->state in j1939_tp_rxtimer()

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the txtimer
  has the same 'hrtimer_start() on an already armed timer leaks the…

--

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

This patch adds j1939_tp_arm_rxtimer_locked(), which cancels and rearms
the session rxtimer under active_session_list_lock using
hrtimer_try_to_cancel(). It also makes j1939_session_cancel()
force-deactivate sessions that are already in WAITING_ABORT. The
second-RTS path in j1939_xtp_rx_rts_session_active() now always
deactivates the session.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..fa23260e1833f 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);
>  }
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the txtimer have the
same leak that this patch is trying to close for the rxtimer?

j1939_tp_schedule_txtimer() always takes a reference and calls
hrtimer_start(). It never drops a reference held by an earlier arm:

net/can/j1939/transport.c:j1939_tp_schedule_txtimer() {
	j1939_session_get(session);
	hrtimer_start(&session->txtimer, ms_to_ktime(msec),
		      HRTIMER_MODE_REL_SOFT);
}

On the receiver side, j1939_xtp_rx_dat_one() calls it without any cancel
when a DAT frame completes a block:

	} else if (do_cts_eoma) {
		j1939_tp_set_rxtimeout(session, 1250);
		if (!session->transmission)
			j1939_tp_schedule_txtimer(session, 0);

Suppose the peer resends the block-ending DAT before the soft txtimer has
run. The duplicate passes the checks: pkt.rx is not incremented, but
pkt.rx >= pkt.last still holds. So do_cts_eoma is set again and
j1939_tp_schedule_txtimer() runs a second time.

The txtimer can still be queued at that point in two ways:

  - both frames were handled in the same NET_RX softirq batch, or
  - a CTS send hit -ENOBUFS and the 10-25 ms retry from j1939_tp_txtimer()
    is pending.

Would the second hrtimer_start() then reprogram the queued timer? That
would leave one session reference, and its netdev_hold() reference, that
is never put.

j1939_xtp_rx_cts_one() and the RTS path have a similar pattern. They call
j1939_session_txtimer_cancel() and then j1939_tp_schedule_txtimer(), with
nothing serializing the two calls.

> -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.
> + *

[ ... ]

> + * 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);
>  }

[Severity: High]
Can the rxtimer still be armed twice while j1939_tp_rxtimer() is running?

hrtimer_try_to_cancel() returns -1 whenever the callback is running. In
that case it does not dequeue the timer, even if the timer has been
requeued:

kernel/time/hrtimer.c:hrtimer_try_to_cancel() {
	...
	if (!hrtimer_callback_running(timer)) {
		ret = remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE);
	...
}

A timer that is rearmed during its own callback stays on the running base.
switch_hrtimer_base() returns early when hrtimer_callback_running() is
true. So every arm during one callback window sees -1 and takes a fresh
reference.

One possible sequence:

CPU A (rxtimer softirq)
j1939_tp_rxtimer()
  session is J1939_SESSION_ACTIVE, takes the else branch
  j1939_session_list_lock() spins

CPU B (CAN rx softirq, CTS/DAT/DPO from the peer)
j1939_tp_set_rxtimeout()
  j1939_session_list_lock()
  hrtimer_try_to_cancel() == -1, no put
  j1939_session_get()
  hrtimer_start()                 <- timer queued, holds a ref
  j1939_session_list_unlock()

CPU A
  takes the lock, state still < J1939_SESSION_ACTIVE_MAX
  j1939_tp_arm_rxtimer_locked()
    hrtimer_try_to_cancel() == -1, no put
    j1939_session_get()
    hrtimer_start()               <- reprograms the queued timer

That takes two references, but only one expiry will put one. Would that
leak the session and its netdev_hold() reference? If so,
unregister_netdevice() would wait forever for canX to become free.

The same thing looks possible in two other cases:

  - CPU B is j1939_session_cancel() called from j1939_tp_txtimer().
    J1939_SESSION_WAITING_ABORT also passes the
    < J1939_SESSION_ACTIVE_MAX check.
  - Two rx-path j1939_tp_set_rxtimeout() calls happen during one rxtimer
    callback window, for example while the callback spins on the lock in
    its WAITING_ABORT branch.

Before this patch, j1939_tp_set_rxtimeout() used hrtimer_cancel(). That
waited for the callback and then removed the timer the callback had
rearmed, returning 1 and dropping the reference. So the race between the
rx handler and the rx timeout callback used to be balanced. Does this
patch add a new way to hit the leak?

The comment above the helper also says:

    when this runs from j1939_tp_rxtimer() itself, the timer is no longer
    queued and 0 is the expected result.

Is that accurate? hrtimer_active() is true while base->running == timer.
So the result from inside the callback is -1, and another CPU may already
have queued the timer.

Does this need something other than the try_to_cancel() return value? For
example, an armed flag kept under active_session_list_lock, or a
hrtimer_is_queued() check in the -1 case. The later patches in this series
don't appear to touch j1939_tp_arm_rxtimer_locked().

> +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);
> +}
> +

[ ... ]

> @@ -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);
>  }

[Severity: Medium]
Can this deactivation leave a queued abort rxtimer on a session that is
already J1939_SESSION_DONE?

The comment says several callers cancel the timers first. The callers in
j1939_tp_txtimer() don't:

	case -EOVERFLOW:
		j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
	...
	default:
		...
		if (session->skcb.addr.type != J1939_SIMPLE) {
			j1939_session_cancel(session, J1939_XTP_ABORT_OTHER);

Take a receiver session with the txtimer armed:

  1. The rxtimer times out first. j1939_tp_rxtimer() sets WAITING_ABORT
     and arms the abort rxtimer, but leaves the txtimer alone.
  2. The txtimer runs j1939_xtp_txnext_receiver(). An error such as
     -ENOENT from j1939_sk_recv_match(), after the receiving socket is
     closed, reaches the default case.
  3. j1939_session_cancel() sees waiting == true and deactivates the
     session at once. That skips the J1939_XTP_ABORT_TIMEOUT_MS wait and
     activates the next queued session early.
  4. The stale abort rxtimer fires later. The state is
     J1939_SESSION_DONE, so it skips the WAITING_ABORT branch and lands
     in the rx timeout else branch:

		j1939_session_list_unlock(session->priv);

		if (!session->sk)
			j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);

Would the receiving sockets then get a second RX_ABORT errqueue
notification? Before this patch, that timer took the WAITING_ABORT branch
and deactivated the session silently.

>  
>  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);

[Severity: Medium]
Can j1939_tp_rxtimer() act on a session->state value that a
concurrent j1939_session_cancel() changes underneath it?

The WAITING_ABORT test at the top of the callback runs without
holding active_session_list_lock:

	if (session->state == J1939_SESSION_WAITING_ABORT) {
		...
		j1939_session_deactivate_activate_next(session);

Before this patch, j1939_session_cancel() went through
j1939_tp_set_rxtimeout(). Its hrtimer_cancel() waited for a running
rxtimer callback before __j1939_session_cancel() changed the state,
at the risk of the deadlock the commit message describes.

The new helper uses hrtimer_try_to_cancel(), which returns -1 and
carries on. So j1939_session_cancel() called from j1939_tp_txtimer()
(the -EOVERFLOW and default cases) can now change the state while an
expiring rxtimer callback sits between its unlocked state check and
taking the lock.

Two orderings look possible. In the first one:

CPU A (rxtimer softirq)              CPU B (txtimer softirq)
j1939_tp_rxtimer()                   j1939_session_cancel()
                                       j1939_session_list_lock()
                                       __j1939_session_cancel()
                                         state = WAITING_ABORT
                                       j1939_tp_arm_rxtimer_locked()
                                         abort rxtimer queued
                                       j1939_session_list_unlock()
  state == WAITING_ABORT
  j1939_session_deactivate_activate_next()

Here the expiry of the old rx timeout is treated as the expiry of the
abort timeout. As a result:

  - the session is deactivated at once,
  - the next queued session is activated early, and
  - the abort rxtimer CPU B just armed stays queued on a
    J1939_SESSION_DONE session.

When that timer fires, it falls into the rx timeout else branch and
calls j1939_sk_errqueue() again.

In the other ordering, CPU A reads J1939_SESSION_ACTIVE first, takes
the else branch and spins on the lock while CPU B runs the same
steps. Once CPU A gets the lock, the test
session->state < J1939_SESSION_ACTIVE_MAX still passes for
J1939_SESSION_WAITING_ABORT. CPU A then rearms the rxtimer a second
time, which is the same double reference described above, and calls
__j1939_session_cancel() again. That:

  - sends a second abort frame to the peer,
  - overwrites session->err with the errno for
    J1939_XTP_ABORT_TIMEOUT instead of the error CPU B reported, and
  - calls j1939_sk_send_loop_abort() a second time.

j1939_session_cancel() limits its check to state <
J1939_SESSION_WAITING_ABORT. Should the else branch here use the same
bound? And should the WAITING_ABORT test at the top of the callback be
made under active_session_list_lock, so the callback only acts on a
state it holds the lock for?

[Severity: Medium]
The commit message doesn't seem to describe some of the functional
changes in this patch. That may matter for the Cc: stable backport.

The new waiting logic in j1939_session_cancel() force-deactivates sessions
that are already in WAITING_ABORT. That looks like a fix for a separate
session/netdev refcount leak:

  - j1939_xtp_rx_cts_one(), j1939_xtp_rx_dat_one() and
    j1939_xtp_rx_rts_session_active() call j1939_session_timers_cancel()
    and then j1939_session_cancel() on a session that is already aborting.
  - Before this patch, nothing was left to deactivate that session.

It also changes teardown timing for every caller of
j1939_session_cancel(). Could this be split out, or at least described in
the commit message?

This hunk replaces the role-specific logic and comment added by commit
1809c82aa073 ("net: can: j1939: j1939_xtp_rx_rts_session_active():
deactivate session upon receiving the second rts"), but doesn't refer to
that commit.

For receivers, isn't this explicit deactivate now a no-op? The waiting
branch in j1939_session_cancel() has already deactivated the session.

The commit message also says:

    j1939_session_cancel() calls the helper instead of hand-rolling
    get + hrtimer_start()

Before this patch, j1939_session_cancel() called j1939_tp_set_rxtimeout()
with the lock held. Only j1939_tp_rxtimer() open-coded the get +
hrtimer_start() sequence.

The helper is also called after __j1939_session_cancel() in
j1939_session_cancel(), but before it in j1939_tp_rxtimer(). Is there a
reason for the different order?

>  
>  		return -EBUSY;
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-j1939-edpo-v9-v9-0-e779ca334a81%40163.com

Re: [PATCH net v9 2/4] can: j1939: validate the EDPO packet number

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In j1939_xtp_rx_dpo_one(), the abort code for transmitter
  sessions rests on a false claim.
- [Low] The new comment in j1939_xtp_rx_dpo_one()
  (net/can/j1939/transport.c, around line 1872 at this commit) points to…

--

can: j1939: validate the EDPO packet number

This commit checks the packet number carried in ETP.CM_DPO in
j1939_xtp_rx_dpo_one(). A value at or past pkt.total now aborts the
session. The session timers are cancelled before the abort, and the
abort code is J1939_XTP_ABORT_ECTS_TOO_BIG for transmitter sessions and
J1939_XTP_ABORT_BAD_EDPO_OFFSET for receiver sessions.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index fa23260e1833f..2580091827fc6 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1853,14 +1853,42 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,
>  				 struct sk_buff *skb)
>  {
>  	const u8 *dat = skb->data;
> +	unsigned int dpo;
>
>  	if (j1939_xtp_rx_cmd_bad_pgn(session, skb))
>  		return;
>
>  	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
>
> +	dpo = j1939_etp_ctl_to_packet(dat);
> +
> +	/* EDPO is peer controlled.  j1939_xtp_rx_dat_one() places data at
> +	 * "dat[0] - 1 + pkt.dpo", so a value at or past pkt.total moves
> +	 * pkt.dpo past the reassembled buffer, and the ETP completion path
> +	 * then looks up an offset no queued skb covers.
> +	 *
> +	 * This bounds the receiver-side writer only.  j1939_session_tx_dpo()
> +	 * copies pkt.tx_acked, which comes from an ETP.CM_CTS packet number
> +	 * that the responder controls, and 4/4 bounds it where it is parsed.
> +	 */

[Severity: Low]
This isn't a bug, but could this comment name the companion patch by
subject instead of by series position? "4/4" stops meaning anything once
the series is merged or the patches are backported to stable one at a
time. The commit message has the same problem:

  The CTS packet number that feeds the transmitter-side EDPO is bounded in
  4/4, on its own, because it is a different input parsed in a different
  function

At this commit nothing bounds it yet. j1939_xtp_rx_cts_one() still does:

	session->pkt.tx_acked = pkt - 1;

with no check of pkt against pkt.total, and j1939_session_tx_dpo() still
copies that value into pkt.dpo. The bound is added later in the series by
"can: j1939: reject a CTS that names more packets than the message has".

Also, the comment says "This bounds the receiver-side writer only", but
the check right below it also aborts transmitter sessions, and the
session->pkt.dpo = dpo assignment runs for both roles. Could the wording
be changed to match the code?

> +	if (dpo >= session->pkt.total) {
> +		netdev_warn(session->priv->ndev,
> +			    "%s: 0x%p: EDPO %u out of range (total %u), aborting\n",
> +			    __func__, session, dpo, session->pkt.total);
> +		j1939_session_timers_cancel(session);
> +		/* The transmitter only sees this on the echo of an EDPO it built
> +		 * from an oversized ETP.CM_CTS packet number, so it is the
> +		 * responder's CTS that is too big, not the EDPO offset.
> +		 */
> +		j1939_session_cancel(session,
> +				     session->transmission ?
> +				     J1939_XTP_ABORT_ECTS_TOO_BIG :
> +				     J1939_XTP_ABORT_BAD_EDPO_OFFSET);

[Severity: Medium]
Is it true that a transmitter session only reaches this branch through
the echo of its own EDPO?

j1939_can_recv() in net/can/j1939/main.c sets J1939_ECU_LOCAL_SRC from
the source address alone:

	if (j1939_address_is_unicast(skcb->addr.sa) &&
	    priv->ents[skcb->addr.sa].nusers)
		skcb->flags |= J1939_ECU_LOCAL_SRC;

j1939_tp_cmd_recv() then sends every LOCAL_SRC DPO to the transmitter
session:

	case J1939_ETP_CMD_DPO:
		if (skcb->addr.type != J1939_ETP)
			return;

		if (j1939_tp_im_transmitter(skcb))
			j1939_xtp_rx_dpo(priv, skb, true);

So another node on the bus that sends an ETP.CM_DPO with our SA (CAN has
no source authentication, or two ECUs may be configured with the same
address) and a packet number >= pkt.total reaches this branch with
session->transmission == true. That node sent no oversized CTS, but the
session is aborted with J1939_XTP_ABORT_ECTS_TOO_BIG, and that reason goes
out on the bus.

Once the later patch "can: j1939: reject a CTS that names more packets
than the message has" is applied, pkt in j1939_xtp_rx_cts_one() stays
within [1, pkt.total]. tx_acked is then at most pkt.total - 1, and
j1939_session_tx_dpo() always builds an in-range EDPO. The echo case
described in the comment can then no longer happen.

In the final tree, a foreign or spoofed DPO is the only way into the
transmitter branch. Would J1939_XTP_ABORT_BAD_EDPO_OFFSET be the right
reason for both roles? The comment here and the matching paragraph in the
commit message would need the same update.

> +		return;
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-j1939-edpo-v9-v9-0-e779ca334a81%40163.com

Re: [PATCH net v9 4/4] can: j1939: reject a CTS that names more packets than the message has

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message gets the current behaviour wrong.
- [Medium] can: j1939: j1939_xtp_rx_cts_one() rejects standard CTS(0)
  frames
- [Low] The commit message presents the patch as an ETP-only relocation
  of a check that already fires ("the transmitter reports that value…
- [Low] The comment and the role-based abort code in
  j1939_xtp_rx_dpo_one() are stale once this patch lands.
- [Low] The new check in j1939_xtp_rx_cts_one() covers both TP and ETP
  sessions: pkt = dat[2] for J1939_TP_CMD_CTS and…

--

can: j1939: reject a CTS that names more packets than the message has

This patch adds a check to j1939_xtp_rx_cts_one() that rejects a CTS
whose packet number is larger than pkt.total. The session is aborted
with J1939_XTP_ABORT_ECTS_TOO_BIG. The commit message describes this as
moving an existing check from the ETP.CM_DPO path to where the CTS is
parsed.

> That value is what j1939_xtp_rx_dpo_one() now bounds, so an oversized CTS
> is currently caught one step later and reported as a bad EDPO offset.
> Reject it where it originates instead, with the abort code that describes
> it: J1939_XTP_ABORT_ECTS_TOO_BIG, "ECTS requested packets exceeds message
> size".

[Severity: Medium]
Is this description of the current behaviour accurate?

The transmitter handles the CTS in j1939_xtp_rx_cts_one() and builds the
ETP.CM_DPO from pkt.tx_acked in j1939_session_tx_dpo(). It only sees that
DPO again on the local echo. The echo has J1939_ECU_LOCAL_SRC set and
takes this path, with session->transmission set:

j1939_tp_cmd_recv()
  j1939_xtp_rx_dpo(priv, skb, true)
    j1939_xtp_rx_dpo_one()

The bound in j1939_xtp_rx_dpo_one() already picks the ECTS code for the
transmitter:

net/can/j1939/transport.c:j1939_xtp_rx_dpo_one() {
    ...
		j1939_session_cancel(session,
				     session->transmission ?
				     J1939_XTP_ABORT_ECTS_TOO_BIG :
				     J1939_XTP_ABORT_BAD_EDPO_OFFSET);
    ...
}

So the local transmitter already reports J1939_XTP_ABORT_ECTS_TOO_BIG
before this patch. The commit message of the earlier patch in this series,
"can: j1939: validate the EDPO packet number", says the same.

This patch is tagged for stable. Could the changelog describe what
actually changes? The out-of-range DPO no longer goes out on the bus, and
TP sessions are now covered as well.

[Severity: Low]
The changelog presents this as an ETP-only move of a check that already
fires. However, j1939_xtp_rx_cts_one() also handles TP.CM_CTS (pkt =
dat[2]). TP never sends a DPO, so before this patch nothing caught an
oversized TP CTS.

For a TP transmitter, pkt.block is min(j1939_tp_block ?: 255, total). A
CTS with dat[2] > total therefore passes the old checks, and tx_acked and
pkt.tx become pkt - 1 >= total. After that:

j1939_xtp_txnext_transmiter()
  j1939_session_tx_dat()
    se_skb = j1939_session_skb_get_by_offset(session, session->pkt.tx * 7);
    if (!se_skb)
        return -ENOBUFS;

j1939_tp_txtimer() retries on -ENOBUFS every 10-25 ms, up to
J1939_XTP_TX_RETRY_LIMIT times. The 1250 ms rx timeout set in
j1939_xtp_rx_cts_one() normally expires first, and j1939_tp_rxtimer()
then aborts with J1939_XTP_ABORT_TIMEOUT. When the txtimer retries run
out, the session ends with -ENETUNREACH.

For ETP, before this patch the out-of-range ETP.CM_DPO was actually sent
on the bus by j1939_tp_tx_ctl() in j1939_session_tx_dpo(). Only its local
echo triggered the abort.

Could the commit message mention both cases? They look like the
user-visible fixes behind the Cc: stable tag.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index fc2840f334b5b..377991d36dc6a 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1512,6 +1512,15 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>  		goto out_session_cancel;
>  	else if (dat[1] > session->pkt.block /* 0xff for etp */)
>  		goto out_session_cancel;
> +	else if (pkt > session->pkt.total) {
> +		/* The responder asks for more packets than the message has.
> +		 * pkt - 1 becomes pkt.tx_acked below, and the transmitter
> +		 * reports that value in an ETP.CM_DPO, where it would name a
> +		 * packet past the end of the message.
> +		 */

[Severity: Medium]
This bound is checked before dat[1] is looked at, so it also applies
to a CTS(0). That is the hold message, with dat[1] == 0, that a
responder sends to keep the connection open without taking more data.

SAE J1939-21 and ISO 11783-6 have the responder set the next packet
number in such a hold message to 0xff for TP.CM_CTS and to 0xffffff
for ETP.CM_CTS. dat[2] and j1939_etp_ctl_to_packet() pass those
values through unchanged. For any session whose pkt.total is smaller,
pkt > session->pkt.total is then true, and the session is cancelled
with J1939_XTP_ABORT_ECTS_TOO_BIG.

Before this patch such a frame got past the checks and reached the
CTS(0) branch at the end of j1939_xtp_rx_cts_one(), which only calls
j1939_tp_set_rxtimeout(session, 550). Now a peer that pauses a TP or
ETP transfer this way has the whole transfer aborted. With the
Cc: stable tag, that interoperability regression would also land in
stable kernels.

Should the new bound only apply when dat[1] is non-zero?

The comment just below, "set packet counters only when not CTS(0)",
suggests the tx_acked, last and tx updates were meant to be skipped
for CTS(0) as well. The code does them unconditionally, though.
Before this patch, a 0xff or 0xffffff packet number in a hold message
ended up in pkt.tx_acked and was used by j1939_session_skb_drop_old().

Would it be cleaner to handle CTS(0) before both the new bound and
the counter updates?

[Severity: Low]
This check covers both TP.CM_CTS and ETP.CM_CTS, but the comment only
gives the ETP.CM_DPO reason. For TP, the consequence is that
j1939_session_tx_dat() looks up pkt.tx * 7 past the queued skb.

Could the comment describe the TP case too?

On the abort code: enum j1939_xtp_abort and j1939_xtp_abort_to_str()
document J1939_XTP_ABORT_ECTS_TOO_BIG only as "ECTS requested packets
exceeds message size". The documentation for code 9, by contrast, gives
separate ETP and TP meanings.

The -EOVERFLOW path in j1939_tp_txtimer() already sends this code for TP,
so the choice here matches existing practice. That TP use just isn't
documented anywhere.

> +		err = J1939_XTP_ABORT_ECTS_TOO_BIG;
> +		goto out_session_cancel;
> +	}
>  
>  	/* set packet counters only when not CTS(0) */
>  	session->pkt.tx_acked = pkt - 1;

[Severity: Low]
With this check in place, tx_acked = pkt - 1 is at most pkt.total - 1.
The only other writer is j1939_xtp_rx_eoma_one(), which sets tx_acked =
total but then completes the session.

j1939_session_tx_dpo() copies tx_acked into pkt.dpo. So a DPO the
transmitter built itself can no longer hit "dpo >= session->pkt.total" in
j1939_xtp_rx_dpo_one().

Should the comments that the earlier patch in this series added there be
updated? Both look stale now:

net/can/j1939/transport.c:j1939_xtp_rx_dpo_one() {
    ...
	 * This bounds the receiver-side writer only.  j1939_session_tx_dpo()
	 * copies pkt.tx_acked, which comes from an ETP.CM_CTS packet number
	 * that the responder controls, and 4/4 bounds it where it is parsed.
	 */
    ...
		/* The transmitter only sees this on the echo of an EDPO it built
		 * from an oversized ETP.CM_CTS packet number, so it is the
		 * responder's CTS that is too big, not the EDPO offset.
		 */
    ...
}

"4/4" is a series index and means nothing once merged. Could it name
j1939_xtp_rx_cts_one() instead?

After this patch, the transmitter branch can only be reached by a DPO
that carries a locally claimed SA but was not built by this host. main.c
sets J1939_ECU_LOCAL_SRC for any received frame whose SA is in priv->ents
with users, and that includes a conflicting or spoofing node. In that
case, J1939_XTP_ABORT_ECTS_TOO_BIG blames a responder CTS that was valid.

Should the session->transmission ? ... selection in j1939_xtp_rx_dpo_one()
be revisited as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-j1939-edpo-v9-v9-0-e779ca334a81%40163.com

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

[Chaosheng Qu]

Hi,

Replying once for the series rather than per patch.

1/4

The High holds. hrtimer_try_to_cancel() returns -1 while the callback runs
and leaves alone a timer that was requeued from inside it, so an arm that
races the expiry still takes a second reference for a single queued timer.
The list lock does not help, because the running callback sits on the other
side of it.

The three Mediums hold as well. Deactivating on a WAITING_ABORT cancel does
leave the abort rxtimer queued on a DONE session, and that expiry takes the
generic else branch and queues a second RX_ABORT. j1939_tp_rxtimer() reads
session->state before taking the lock, and with try_to_cancel() no longer
waiting, a concurrent cancel can change state underneath it; the callback
then calls __j1939_session_cancel() again, which puts a second abort frame on
the bus and overwrites session->err. And the message is wrong in two places:
before this patch j1939_session_cancel() called j1939_tp_set_rxtimeout(), it
did not hand-roll get + hrtimer_start(), only j1939_tp_rxtimer() did; and it
replaces the 1809c82aa073 workaround without saying so.

The pre-existing txtimer leak is real too: j1939_tp_schedule_txtimer() takes
a reference per hrtimer_start() with no cancel in front, and the do_cts_eoma
path in j1939_xtp_rx_dat_one() calls it that way, so duplicate DAT frames in
one softirq batch leak the same reference.

So 1/4 needs the reference pinned across the whole queued and running
lifetime of the timer, not the queued part alone: a flag owned by
active_session_list_lock, where the clear-to-set transition takes the
reference and the callback drops it, and both armers and the callback test it
under the lock. That also fixes the state check in j1939_tp_rxtimer() and
lets the WAITING_ABORT deactivate go away. It is a rewrite, not another pass
at this hunk, so it will not be a v10 tomorrow. Zihan Xi is working the same
reference-lifetime problem and Oleksij has asked him to sync with Tetsuo; I
will see where that lands before sending and say so in the cover letter.

2/4

The abort code rests on a false premise. J1939_ECU_LOCAL_SRC is set from the
source address alone in j1939_can_recv(), so a second node using our address,
or a raw frame injected on the interface, is delivered to the transmitter
session. It sent no CTS, yet the session is aborted with ECTS_TOO_BIG and
that reason goes out on the bus. Once j1939_xtp_rx_cts_one() bounds the
packet number, pkt.tx_acked cannot exceed pkt.total - 1, so the next version
uses BAD_EDPO_OFFSET for both roles and drops the split. The comment will
name j1939_xtp_rx_cts_one() instead of "4/4", and the "bounds the
receiver-side writer only" wording goes.

4/4

The changelog describes the wrong current behaviour. An oversized
ETP.CM_CTS already ends in ECTS_TOO_BIG: j1939_xtp_rx_cts_one() stores pkt-1
in pkt.tx_acked, j1939_session_tx_dpo() copies it into the DPO, and the local
echo reaches the transmitter branch with session->transmission set. What
changes is that the out-of-range DPO is no longer sent on the bus. The visible
change is the TP case: there is no DPO, so pkt.tx >= pkt.total made
j1939_session_tx_dat() return -ENOBUFS and the session ran until the 1250ms
rxtimer aborted with TIMEOUT or the retry limit ended it with -ENETUNREACH.
That leads the new changelog. The comment and the role split follow 2/4.

3/4

Both nits accepted: WARN_ON_ONCE(!se_skb) and session->err = -EBADMSG instead
of the silent drop, and the comment in j1939_xtp_rx_dpo_one() rewritten as
part of the EDPO patch.

Thanks,
Chaosheng

pw-bot: cr