[PATCH v7 0/5] net: can: isotp-fixes

[PATCH v7 0/5] net: can: isotp-fixes

[Oliver Hartkopp via B4 Relay]

As sashiko-bot was not able to check the second patch this bundle is
re-posted with b4 preparation.

Fixed the remarks reported by sashiko-bot of V6 patchset.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
Changes in v7:
- patch 3: add helper so->tx_gen to monitor new ISOTP_SENDING attempts
- patch 4: prevent missasignment of sk_err report with new so->tx_gen
- Link to v6: https://patch.msgid.link/20260710-isotp-fixes-v6-0-a13c2539093d@hartkopp.net

Changes in v6:
- patch 2: check for rx state and tx state to be IDLE in isotp_bind() (sashiko-bot)
- patch 3: double check rx/tx states after all hrtimer_cancel() calls (sashiko-bot)
           move ISOTP_IDLE in err_event_drop for the same reason
- patch 4: use READ_ONCE() to prevent compiler load tearing as remarked by sashiko-bot
- Link to v5: https://patch.msgid.link/20260710-isotp-fixes-v5-0-15174d82d6f1@hartkopp.net

Changes in v5:
- patch 3: new: state change re-check after hrtimer_cancel()
- patch 4: new: handle state transistion with cmpxchg()
- Link to v4: https://patch.msgid.link/20260710-isotp-fixes-v4-0-2a4af437f61b@hartkopp.net

Changes in v4:
- patch 2: use a simpler method to reject a re-binding that can be implemented
  inside the lock: No waitqueue waiting just exit.
- Link to v3: https://patch.msgid.link/20260710-isotp-fixes-v3-0-08db68e27d0b@hartkopp.net

Changes in v3:
- patch 2: go back to initial idea without hard resetting the state machine and
  timers but let these come to a graceful end. Even a new bind() at netdev
  shutdown now leads to this graceful statemachine shutdown via the wait queue.
- Link to v2: https://patch.msgid.link/20260710-isotp-fixes-v2-0-bc57e26594b2@hartkopp.net

Changes in v2:
- patch 2: shutdown state machine to fix sashiko-bot complains
  Link: https://lore.kernel.org/linux-can/20260710094807.A50DD1F000E9@smtp.kernel.org/
- Link to v1: https://patch.msgid.link/20260710-isotp-fixes-v1-0-75a1d11d7df9@hartkopp.net

---
Oliver Hartkopp (5):
      can: isotp: use unconditional synchronize_rcu() in isotp_release()
      can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER
      can: isotp: fix race between RX/TX timers and frame reception
      can: isotp: fix lock-free state transition in tx timer handler
      can: isotp: fix state machine corruption on signal interruption

 net/can/isotp.c | 176 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 135 insertions(+), 41 deletions(-)
---
base-commit: a635d6748234582ea287c5ffeae28b9b23f91c7e
change-id: 20260710-isotp-fixes-ead7480d67bf

Best regards,
--  
Oliver Hartkopp <socketcan@hartkopp.net>

[PATCH v7 1/5] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

isotp_notify() unregisters the (RCU) CAN filters via can_rx_unregister()
and clears so->bound without waiting for a grace period. isotp_release()
uses so->bound to decide whether it needs to call synchronize_rcu()
before cancelling so->rxtimer, so when NETDEV_UNREGISTER runs first it
skips that synchronize_rcu() and can cancel the timer while an
in-flight isotp_rcv() is still executing and about to re-arm it via
isotp_send_fc(), leading to a use-after-free timer callback on the
freed socket.

sakisho-bot remarked a problem with rtnl_lock held in isotp_notify(),
therefore make isotp_release() always call synchronize_rcu() before
cancelling the timers, regardless of so->bound. This still closes the
original race (isotp_notify() clearing so->bound without waiting for
in-flight isotp_rcv() callers before isotp_release() cancels the RX
timer) without adding any RCU wait to the netdevice notifier path.

Fixes: 14a4696bc311 ("can: isotp: isotp_release(): omit unintended hrtimer restart on socket release")
Closes: https://lore.kernel.org/linux-can/20260707085210.6B6C01F000E9@smtp.kernel.org/
Reported-by: Nico Yip <zdi-disclosures@trendmicro.com> (ZDI-CAN-31764)
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index c48b4a818297..d30937345bcd 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1235,15 +1235,22 @@ static int isotp_release(struct socket *sock)
 
 				can_rx_unregister(net, dev, so->txid,
 						  SINGLE_MASK(so->txid),
 						  isotp_rcv_echo, sk);
 				dev_put(dev);
-				synchronize_rcu();
 			}
 		}
 	}
 
+	/* Always wait for a grace period before touching the timers below.
+	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
+	 * filters and cleared so->bound in isotp_notify() without waiting
+	 * for in-flight isotp_rcv() callers to finish, so this call must not
+	 * be skipped just because so->bound is already 0 here.
+	 */
+	synchronize_rcu();
+
 	hrtimer_cancel(&so->txfrtimer);
 	hrtimer_cancel(&so->txtimer);
 	hrtimer_cancel(&so->rxtimer);
 
 	so->ifindex = 0;

-- 
2.53.0

[PATCH v7 2/5] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

isotp_release() looked up the bound network device via dev_get_by_index()
using the stored ifindex. During device unregistration the device is
unlisted from the ifindex hash before the NETDEV_UNREGISTER notifier
chain runs, so a concurrent isotp_release() could find no device, skip
can_rx_unregister() entirely, and still proceed to free the socket.
Since isotp_release() had already removed itself from the isotp
notifier list at that point, isotp_notify() would never get a chance to
clean up either, leaving a stale CAN filter that keeps pointing at the
freed socket.

Fix this the same way raw.c already does: hold a tracked reference to
the bound net_device in the socket (so->dev/so->dev_tracker) from
bind() onward instead of re-resolving it from the ifindex, and
serialize bind()/release() with rtnl_lock() so that so->dev is always
consistent with what the NETDEV_UNREGISTER notifier sees. so->dev
stays valid regardless of ifindex-hash unlisting, and is only ever
cleared by whichever of isotp_release()/isotp_notify() gets there
first, so the filter is always removed exactly once.

isotp_bind() now rejects a (re)bind with -EAGAIN while so->[tx|rx].state
isn't ISOTP_IDLE yet, so a timer left running by a prior
NETDEV_UNREGISTER can't act on a newly bound so->ifindex. Both checks
share the same lock_sock() section, so there is no window in which a
concurrent isotp_notify() clearing so->bound could be missed.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-can/20260707101420.47F261F000E9@smtp.kernel.org/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 87 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 28 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index d30937345bcd..44c044eb83e1 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -150,10 +150,12 @@ struct tpcon {
 
 struct isotp_sock {
 	struct sock sk;
 	int bound;
 	int ifindex;
+	struct net_device *dev;
+	netdevice_tracker dev_tracker;
 	canid_t txid;
 	canid_t rxid;
 	ktime_t tx_gap;
 	ktime_t lastrxcf_tstamp;
 	struct hrtimer rxtimer, txtimer, txfrtimer;
@@ -976,10 +978,18 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
 		if (err)
 			goto err_event_drop;
 	}
 
+	/* so->bound is only checked once above - a wakeup may have
+	 * unbound/rebound the socket meanwhile, so re-validate it
+	 */
+	if (!so->bound) {
+		err = -EADDRNOTAVAIL;
+		goto err_out_drop;
+	}
+
 	/* PDU size > default => try max_pdu_size */
 	if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {
 		u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
 
 		if (newbuf) {
@@ -1217,32 +1227,34 @@ static int isotp_release(struct socket *sock)
 		spin_lock(&isotp_notifier_lock);
 	}
 	list_del(&so->notifier);
 	spin_unlock(&isotp_notifier_lock);
 
+	rtnl_lock();
 	lock_sock(sk);
 
-	/* remove current filters & unregister */
-	if (so->bound) {
-		if (so->ifindex) {
-			struct net_device *dev;
-
-			dev = dev_get_by_index(net, so->ifindex);
-			if (dev) {
-				if (isotp_register_rxid(so))
-					can_rx_unregister(net, dev, so->rxid,
-							  SINGLE_MASK(so->rxid),
-							  isotp_rcv, sk);
-
-				can_rx_unregister(net, dev, so->txid,
-						  SINGLE_MASK(so->txid),
-						  isotp_rcv_echo, sk);
-				dev_put(dev);
-			}
-		}
+	/* remove current filters & unregister
+	 * tracked reference so->dev is taken at bind() time with rtnl_lock
+	 */
+	if (so->bound && so->dev) {
+		if (isotp_register_rxid(so))
+			can_rx_unregister(net, so->dev, so->rxid,
+					  SINGLE_MASK(so->rxid),
+					  isotp_rcv, sk);
+
+		can_rx_unregister(net, so->dev, so->txid,
+				  SINGLE_MASK(so->txid),
+				  isotp_rcv_echo, sk);
+		netdev_put(so->dev, &so->dev_tracker);
 	}
 
+	so->ifindex = 0;
+	so->bound = 0;
+	so->dev = NULL;
+
+	rtnl_unlock();
+
 	/* Always wait for a grace period before touching the timers below.
 	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
 	 * filters and cleared so->bound in isotp_notify() without waiting
 	 * for in-flight isotp_rcv() callers to finish, so this call must not
 	 * be skipped just because so->bound is already 0 here.
@@ -1251,13 +1263,10 @@ static int isotp_release(struct socket *sock)
 
 	hrtimer_cancel(&so->txfrtimer);
 	hrtimer_cancel(&so->txtimer);
 	hrtimer_cancel(&so->rxtimer);
 
-	so->ifindex = 0;
-	so->bound = 0;
-
 	sock_orphan(sk);
 	sock->sk = NULL;
 
 	release_sock(sk);
 	sock_prot_inuse_add(net, sk->sk_prot, -1);
@@ -1308,17 +1317,29 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	}
 
 	if (!addr->can_ifindex)
 		return -ENODEV;
 
+	rtnl_lock();
 	lock_sock(sk);
 
 	if (so->bound) {
 		err = -EINVAL;
 		goto out;
 	}
 
+	/* A transmission or reception that outlived a previous binding
+	 * (unbound by NETDEV_UNREGISTER) may still be draining; the FC/echo
+	 * and RX watchdog timers bound how long this takes. Checked together
+	 * with so->bound in the same lock_sock() section above, so there is
+	 * no window in which a concurrent isotp_notify() could be missed.
+	 */
+	if (so->tx.state != ISOTP_IDLE || so->rx.state != ISOTP_IDLE) {
+		err = -EAGAIN;
+		goto out;
+	}
+
 	/* ensure different CAN IDs when the rx_id is to be registered */
 	if (isotp_register_rxid(so) && rx_id == tx_id) {
 		err = -EADDRNOTAVAIL;
 		goto out;
 	}
@@ -1327,18 +1348,16 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	if (!dev) {
 		err = -ENODEV;
 		goto out;
 	}
 	if (dev->type != ARPHRD_CAN) {
-		dev_put(dev);
 		err = -ENODEV;
-		goto out;
+		goto out_put_dev;
 	}
 	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
-		dev_put(dev);
 		err = -EINVAL;
-		goto out;
+		goto out_put_dev;
 	}
 	if (!(dev->flags & IFF_UP))
 		notify_enetdown = 1;
 
 	ifindex = dev->ifindex;
@@ -1352,20 +1371,29 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 
 	/* register for echo skb's */
 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
 			isotp_rcv_echo, sk, "isotpe", sk);
 
-	dev_put(dev);
-
 	/* switch to new settings */
 	so->ifindex = ifindex;
 	so->rxid = rx_id;
 	so->txid = tx_id;
 	so->bound = 1;
 
+	/* bind() ok -> hold a reference for so->dev so that isotp_release()
+	 * can safely reach the device later, even if a concurrent
+	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
+	 */
+	so->dev = dev;
+	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
+
+out_put_dev:
+	/* remove potential reference from dev_get_by_index() */
+	dev_put(dev);
 out:
 	release_sock(sk);
+	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);
@@ -1564,11 +1592,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 	struct sock *sk = &so->sk;
 
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (so->ifindex != dev->ifindex)
+	if (so->dev != dev)
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
@@ -1580,14 +1608,16 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 						  isotp_rcv, sk);
 
 			can_rx_unregister(dev_net(dev), dev, so->txid,
 					  SINGLE_MASK(so->txid),
 					  isotp_rcv_echo, sk);
+			netdev_put(so->dev, &so->dev_tracker);
 		}
 
 		so->ifindex = 0;
 		so->bound  = 0;
+		so->dev = NULL;
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);
@@ -1643,10 +1673,11 @@ static int isotp_init(struct sock *sk)
 {
 	struct isotp_sock *so = isotp_sk(sk);
 
 	so->ifindex = 0;
 	so->bound = 0;
+	so->dev = NULL;
 
 	so->opt.flags = CAN_ISOTP_DEFAULT_FLAGS;
 	so->opt.ext_address = CAN_ISOTP_DEFAULT_EXT_ADDRESS;
 	so->opt.rx_ext_address = CAN_ISOTP_DEFAULT_EXT_ADDRESS;
 	so->opt.rxpad_content = CAN_ISOTP_DEFAULT_PAD_CONTENT;

-- 
2.53.0

[PATCH v7 3/5] can: isotp: fix race between RX/TX timers and frame reception

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

When receiving a Consecutive Frame, Flow Control frame, or local echo
frame, hrtimer_cancel() is called to stop the corresponding watchdog
timer. If the timer handler was already running concurrently on another
CPU, hrtimer_cancel() waits for it to finish.

By the time it returns, the timer handler may have already reported a
timeout error and reset the state machine. Blindly continuing corrupts
the newly reset or idle state, and can let a concurrent sendmsg() claim
the same IDLE state at the same time.

Fix this by re-checking the state right after canceling the timer in all
three call sites (isotp_rcv_cf(), isotp_rcv_fc(), isotp_rcv_echo()). If
the state changed, the transfer has already timed out, so drop the frame
instead of resuming it.

However, the state check alone cannot tell a genuinely resumed transfer
from a new one: while hrtimer_cancel() waits, a concurrent sendmsg()
could claim ISOTP_IDLE and start a new transfer that happens to reach
the very same state again. so->tx_gen, bumped each time sendmsg() claims
a new transfer, disambiguates the two and is checked alongside the state.

For the same reason, isotp_sendmsg()'s err_event_drop path must cancel
so->txfrtimer/txtimer before setting so->tx.state to ISOTP_IDLE, not
after: otherwise a concurrent sendmsg() could claim ISOTP_SENDING and
start filling so->tx.buf while a still-armed timer from the aborted
transfer fires and sends a stale frame from it.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-can/20260710142146.BDAE61F000E9@smtp.kernel.org/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 44c044eb83e1..f05703ddd2ea 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -164,10 +164,11 @@ struct isotp_sock {
 	struct can_isotp_ll_options ll;
 	u32 frame_txtime;
 	u32 force_tx_stmin;
 	u32 force_rx_stmin;
 	u32 cfecho; /* consecutive frame echo tag */
+	u32 tx_gen; /* generation, bumped per new tx transfer */
 	struct tpcon rx, tx;
 	struct list_head notifier;
 	wait_queue_head_t wait;
 	spinlock_t rx_lock; /* protect single thread state machine */
 };
@@ -369,17 +370,27 @@ static int check_pad(struct isotp_sock *so, struct canfd_frame *cf,
 static void isotp_send_cframe(struct isotp_sock *so);
 
 static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
 {
 	struct sock *sk = &so->sk;
+	u32 gen = READ_ONCE(so->tx_gen);
 
 	if (so->tx.state != ISOTP_WAIT_FC &&
 	    so->tx.state != ISOTP_WAIT_FIRST_FC)
 		return 0;
 
 	hrtimer_cancel(&so->txtimer);
 
+	/* hrtimer_cancel() may have let isotp_tx_timer_handler() give up on
+	 * this job (state changed) or a new sendmsg() claim and reach the
+	 * same state again (tx_gen changed) - either way this FC is stale.
+	 */
+	if ((so->tx.state != ISOTP_WAIT_FC &&
+	     so->tx.state != ISOTP_WAIT_FIRST_FC) ||
+	    READ_ONCE(so->tx_gen) != gen)
+		return 1;
+
 	if ((cf->len < ae + FC_CONTENT_SZ) ||
 	    ((so->opt.flags & ISOTP_CHECK_PADDING) &&
 	     check_pad(so, cf, ae + FC_CONTENT_SZ, so->opt.rxpad_content))) {
 		/* malformed PDU - report 'not a data message' */
 		sk->sk_err = EBADMSG;
@@ -575,10 +586,18 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
 		so->lastrxcf_tstamp = skb->tstamp;
 	}
 
 	hrtimer_cancel(&so->rxtimer);
 
+	/* isotp_rx_timer_handler() may have raced us for so->rx.state
+	 * while hrtimer_cancel() above waited for it to finish, already
+	 * reporting ETIMEDOUT and resetting the reception; don't process
+	 * this CF into a reassembly that has already been given up on.
+	 */
+	if (so->rx.state != ISOTP_WAIT_DATA)
+		return 1;
+
 	/* CFs are never longer than the FF */
 	if (cf->len > so->rx.ll_dl)
 		return 1;
 
 	/* CFs have usually the LL_DL length */
@@ -868,10 +887,11 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,
 static void isotp_rcv_echo(struct sk_buff *skb, void *data)
 {
 	struct sock *sk = (struct sock *)data;
 	struct isotp_sock *so = isotp_sk(sk);
 	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+	u32 gen = READ_ONCE(so->tx_gen);
 
 	/* only handle my own local echo CF/SF skb's (no FF!) */
 	if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
 		return;
 
@@ -879,10 +899,18 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
 	hrtimer_cancel(&so->txtimer);
 
 	/* local echo skb with consecutive frame has been consumed */
 	so->cfecho = 0;
 
+	/* hrtimer_cancel() may have let isotp_tx_timer_handler() give up
+	 * on this job (state changed) or a new sendmsg() claim and reach
+	 * ISOTP_SENDING again (tx_gen changed) - either way this echo is
+	 * stale.
+	 */
+	if (so->tx.state != ISOTP_SENDING || READ_ONCE(so->tx_gen) != gen)
+		return;
+
 	if (so->tx.idx >= so->tx.len) {
 		/* we are done */
 		so->tx.state = ISOTP_IDLE;
 		wake_up_interruptible(&so->wait);
 		return;
@@ -960,10 +988,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0;
 	int wait_tx_done = (so->opt.flags & CAN_ISOTP_WAIT_TX_DONE) ? 1 : 0;
 	s64 hrtimer_sec = ISOTP_ECHO_TIMEOUT;
 	int off;
 	int err;
+	u32 my_gen;
 
 	if (!so->bound || so->tx.state == ISOTP_SHUTDOWN)
 		return -EADDRNOTAVAIL;
 
 	while (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {
@@ -978,10 +1007,17 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
 		if (err)
 			goto err_event_drop;
 	}
 
+	/* new transfer: bump generation and drop any echo tag left behind
+	 * by a previously aborted transfer
+	 */
+	my_gen = so->tx_gen + 1;
+	WRITE_ONCE(so->tx_gen, my_gen);
+	so->cfecho = 0;
+
 	/* so->bound is only checked once above - a wakeup may have
 	 * unbound/rebound the socket meanwhile, so re-validate it
 	 */
 	if (!so->bound) {
 		err = -EADDRNOTAVAIL;
@@ -1141,17 +1177,24 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	}
 
 	return size;
 
 err_event_drop:
-	/* got signal: force tx state machine to be idle */
-	so->tx.state = ISOTP_IDLE;
+	/* got signal: cancel timers before touching state, so a concurrent
+	 * sendmsg() can't claim ISOTP_SENDING while a stale timer is armed
+	 */
 	hrtimer_cancel(&so->txfrtimer);
 	hrtimer_cancel(&so->txtimer);
 err_out_drop:
-	/* drop this PDU and unlock a potential wait queue */
-	so->tx.state = ISOTP_IDLE;
+	/* drop this PDU and unlock a potential wait queue, but only if no
+	 * concurrent sendmsg() has since claimed the socket for a new
+	 * transfer while we were cancelling timers above
+	 */
+	if (READ_ONCE(so->tx_gen) == my_gen) {
+		so->cfecho = 0;
+		so->tx.state = ISOTP_IDLE;
+	}
 	wake_up_interruptible(&so->wait);
 
 	return err;
 }
 

-- 
2.53.0

[PATCH v7 4/5] can: isotp: fix lock-free state transition in tx timer handler

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

Commit 051737439eae ("can: isotp: fix race between isotp_sendsmg() and
isotp_release()") introduced a lock-free state machine check
to prevent race conditions between the TX timer and concurrent state
updates. However, the original patch missed replacing the initial
state checks and left the late assignment of ISOTP_IDLE as a blind,
non-atomic write.

Fix this by properly sampling the initial state into 'old_state' and using
cmpxchg() to atomically move the state to ISOTP_IDLE. If the state changed
concurrently (e.g., due to an incoming echo or a new sendmsg), the timeout
is stale and we bail out safely without corrupting the state machine.

Fixes: 43a08c3bdac4cb ("can: isotp: isotp_sendmsg(): fix TX buffer concurrent access in isotp_sendmsg()")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-can/20260710142146.BDAE61F000E9@smtp.kernel.org/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index f05703ddd2ea..fdb2109d7c27 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -940,24 +940,37 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
 static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
 {
 	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
 					     txtimer);
 	struct sock *sk = &so->sk;
+	u32 gen = READ_ONCE(so->tx_gen);
+	u32 old_state = READ_ONCE(so->tx.state);
 
 	/* don't handle timeouts in IDLE or SHUTDOWN state */
-	if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
+	if (old_state == ISOTP_IDLE || old_state == ISOTP_SHUTDOWN)
+		return HRTIMER_NORESTART;
+
+	/* Only claim timeout if state is unchanged to avoid overwriting
+	 * concurrent state updates from isotp_sendmsg/isotp_rcv_echo.
+	 */
+	if (cmpxchg(&so->tx.state, old_state, ISOTP_IDLE) != old_state)
 		return HRTIMER_NORESTART;
 
 	/* we did not get any flow control or echo frame in time */
 
-	/* report 'communication error on send' */
-	sk->sk_err = ECOMM;
-	if (!sock_flag(sk, SOCK_DEAD))
-		sk_error_report(sk);
+	/* Once the cmpxchg() above released ISOTP_IDLE, a concurrent
+	 * sendmsg() may already have claimed it for a new transfer
+	 * (tx_gen changed); don't taint that new transfer with an error
+	 * that belongs to the one that just timed out.
+	 */
+	if (READ_ONCE(so->tx_gen) == gen) {
+		/* report 'communication error on send' */
+		sk->sk_err = ECOMM;
+		if (!sock_flag(sk, SOCK_DEAD))
+			sk_error_report(sk);
+	}
 
-	/* reset tx state */
-	so->tx.state = ISOTP_IDLE;
 	wake_up_interruptible(&so->wait);
 
 	return HRTIMER_NORESTART;
 }
 

-- 
2.53.0

[PATCH v7 5/5] can: isotp: fix state machine corruption on signal interruption

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

If a thread blocked in isotp_sendmsg() waiting for a previous transfer
to finish is interrupted by a signal, jumping to the cleanup labels
incorrectly cancels active timers and forces 'so->tx.state' back to
ISOTP_IDLE. Since this thread never owned the active transmission, it
abruptly aborts another thread's legitimate transfer, so return the
interruption error directly.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index fdb2109d7c27..c9cd78df3a6e 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1017,11 +1017,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			return -EADDRNOTAVAIL;
 
 		/* wait for complete transmission of current pdu */
 		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
 		if (err)
-			goto err_event_drop;
+			return err;
 	}
 
 	/* new transfer: bump generation and drop any echo tag left behind
 	 * by a previously aborted transfer
 	 */

-- 
2.53.0

Re: [PATCH v7 5/5] can: isotp: fix state machine corruption on signal interruption

[sashiko-bot]

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

Pre-existing issues:
- [High] Unconditional timer cancellation in abort paths and lockless softirq handlers can inadvertently cancel a concurrent transmission's timers, causing a permanent stall.
--

--- Patch [5]: [PATCH v7 5/5] can: isotp: fix state machine corruption on signal interruption ---
commit 0650cd1b992a580e80131b223aa61fb92013fdb6
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: fix state machine corruption on signal interruption

If a thread blocked in isotp_sendmsg() waiting for a previous transfer
to finish is interrupted by a signal, jumping to the cleanup labels
incorrectly cancels active timers and forces 'so->tx.state' back to
ISOTP_IDLE. This commit returns the error directly instead of jumping
to err_event_drop.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index fdb2109d7c27e..c9cd78df3a6e7 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1019,7 +1019,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  		/* wait for complete transmission of current pdu */
>  		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>  		if (err)
> -			goto err_event_drop;
> +			return err;
>  	}
>  
>  	/* new transfer: bump generation and drop any echo tag left behind

[Severity: High]
This isn't a bug introduced by this patch, but does the same problem exist
later in isotp_sendmsg() when wait_tx_done is true?

net/can/isotp.c:isotp_sendmsg() {
...
	if (wait_tx_done) {
		/* wait for complete transmission of current pdu */
		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
		if (err)
			goto err_event_drop;

		err = sock_error(sk);
...
}

If a signal interrupts this second wait_event_interruptible() loop, it jumps
to err_event_drop, which unconditionally cancels timers before verifying
tx_gen ownership.

Additionally, is there a similar race window in the softirq handlers?
In isotp_rcv_fc() and isotp_rcv_echo(), the timer is unconditionally
cancelled after a lockless state check:

net/can/isotp.c:isotp_rcv_fc() {
...
	if (so->tx.state != ISOTP_WAIT_FC &&
	    so->tx.state != ISOTP_WAIT_FIRST_FC)
		return 0;

	hrtimer_cancel(&so->txtimer);
...
}

If the softirq checks the state but is preempted or delayed before calling
hrtimer_cancel(), could a concurrent new transfer start and have its timer
wrongfully cancelled, causing a permanent stall of the socket?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-isotp-fixes-v7-0-f03af24d8397@hartkopp.net?part=5

Re: [PATCH v7 4/5] can: isotp: fix lock-free state transition in tx timer handler

[sashiko-bot]

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

New issues:
- [High] Silent suppression of timeout errors in isotp_tx_timer_handler leads to false success reports.
- [High] isotp_sendmsg evaluates an uninitialized variable and unconditionally cancels the timers of an unrelated active transmission when interrupted by a signal.
- [Medium] Time-of-Check to Time-of-Use race in isotp_tx_timer_handler falsely taints new transfers with ECOMM.

Pre-existing issues:
- [High] Incomplete fix for lock-free state transitions leaves identical vulnerable blind writes in isotp_rcv_echo, isotp_rcv_fc, and isotp_sendmsg.
--

--- Patch [4]: [PATCH v7 4/5] can: isotp: fix lock-free state transition in tx timer handler ---
commit b88e08d8412be9198adca1f2fc2749aff27ff947
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: fix lock-free state transition in tx timer handler

Commit 051737439eae introduced a lock-free state machine check to
prevent race conditions between the TX timer and concurrent state
updates. However, the original patch missed replacing initial state
checks and left the late assignment of ISOTP_IDLE as a blind write.
Fix this by sampling the initial state and using cmpxchg() to
atomically move to ISOTP_IDLE, safely bailing out if the timeout is
stale.

Link: https://lore.kernel.org/linux-can/20260710142146.BDAE61F000E9@smtp.kernel.org/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index f05703ddd2eae..fdb2109d7c27e 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -942,20 +942,33 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>  	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
>  					     txtimer);
>  	struct sock *sk = &so->sk;
> +	u32 gen = READ_ONCE(so->tx_gen);
> +	u32 old_state = READ_ONCE(so->tx.state);
>  
>  	/* don't handle timeouts in IDLE or SHUTDOWN state */
> -	if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
> +	if (old_state == ISOTP_IDLE || old_state == ISOTP_SHUTDOWN)
> +		return HRTIMER_NORESTART;
> +
> +	/* Only claim timeout if state is unchanged to avoid overwriting
> +	 * concurrent state updates from isotp_sendmsg/isotp_rcv_echo.
> +	 */
> +	if (cmpxchg(&so->tx.state, old_state, ISOTP_IDLE) != old_state)
>  		return HRTIMER_NORESTART;

[Severity: Medium]
When the timer handler successfully releases the state machine to ISOTP_IDLE
here, doesn't it open a race window?

If a concurrent isotp_sendmsg() claims the state (ISOTP_SENDING), but hasn't
yet bumped so->tx_gen, the old generation matches the check below, and the new
transfer is falsely tainted with ECOMM.

>  
>  	/* we did not get any flow control or echo frame in time */
>  
> -	/* report 'communication error on send' */
> -	sk->sk_err = ECOMM;
> -	if (!sock_flag(sk, SOCK_DEAD))
> -		sk_error_report(sk);
> +	/* Once the cmpxchg() above released ISOTP_IDLE, a concurrent
> +	 * sendmsg() may already have claimed it for a new transfer
> +	 * (tx_gen changed); don't taint that new transfer with an error
> +	 * that belongs to the one that just timed out.
> +	 */
> +	if (READ_ONCE(so->tx_gen) == gen) {
> +		/* report 'communication error on send' */
> +		sk->sk_err = ECOMM;
> +		if (!sock_flag(sk, SOCK_DEAD))
> +			sk_error_report(sk);
> +	}

[Severity: High]
If a concurrent isotp_sendmsg() immediately claims the socket and updates
tx_gen before this check, the ECOMM error is skipped entirely. 

Does this mean the timeout error is silently suppressed, and an application
waiting for the transfer to finish is falsely notified of success (returning
byte size instead of error)?

>  
> -	/* reset tx state */
> -	so->tx.state = ISOTP_IDLE;
>  	wake_up_interruptible(&so->wait);
>  
>  	return HRTIMER_NORESTART;


[Severity: High]
This is a pre-existing issue, but do other paths still perform non-atomic
blind writes to ISOTP_IDLE?

For instance, in isotp_rcv_echo():

net/can/isotp.c:isotp_rcv_echo() {
    ...
	if (so->tx.state != ISOTP_SENDING || READ_ONCE(so->tx_gen) != gen)
		return;

	if (so->tx.idx >= so->tx.len) {
		/* we are done */
		so->tx.state = ISOTP_IDLE;
		wake_up_interruptible(&so->wait);
		return;
	}
    ...
}

If this thread is preempted or delayed immediately after checking tx_gen, and
the timer handler runs and releases the state so a new transfer can start,
could the delayed thread wake up and blindly force the new transfer's state
to ISOTP_IDLE?

A similar blind write to ISOTP_IDLE occurs in isotp_rcv_fc():

net/can/isotp.c:isotp_rcv_fc() {
    ...
	default:
		/* stop this tx job */
		so->tx.state = ISOTP_IDLE;
		wake_up_interruptible(&so->wait);
	}
    ...
}


[Severity: High]
Is there a problem with uninitialized variables and unconditional timer
cancellations in the error paths of isotp_sendmsg()?

net/can/isotp.c:isotp_sendmsg() {
    ...
		/* wait for complete transmission of current pdu */
		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
		if (err)
			goto err_event_drop;
	}

	/* new transfer: bump generation and drop any echo tag left behind
	 * by a previously aborted transfer
	 */
	my_gen = so->tx_gen + 1;
    ...
err_event_drop:
	/* got signal: cancel timers before touching state, so a concurrent
	 * sendmsg() can't claim ISOTP_SENDING while a stale timer is armed
	 */
	hrtimer_cancel(&so->txfrtimer);
	hrtimer_cancel(&so->txtimer);
err_out_drop:
	/* drop this PDU and unlock a potential wait queue, but only if no
	 * concurrent sendmsg() has since claimed the socket for a new
	 * transfer while we were cancelling timers above
	 */
	if (READ_ONCE(so->tx_gen) == my_gen) {
		so->cfecho = 0;
		so->tx.state = ISOTP_IDLE;
	}
    ...
}

If wait_event_interruptible() is interrupted by a signal, it jumps to
err_event_drop. At this point, my_gen is uninitialized. The code then evaluates
if (READ_ONCE(so->tx_gen) == my_gen), which compares against uninitialized
stack memory.

Furthermore, could calling hrtimer_cancel() unconditionally here cancel the
active timers of an unrelated, ongoing transmission that currently owns the
socket?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-isotp-fixes-v7-0-f03af24d8397@hartkopp.net?part=4

Re: [PATCH v7 1/5] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[sashiko-bot]

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

New issues:
- [Medium] Unconditional synchronize_rcu() in socket release path allows unprivileged users to spam RCU grace periods.

Pre-existing issues:
- [Critical] Missing rtnl_lock() in isotp_bind() allows race with NETDEV_UNREGISTER, leading to leaked CAN RX filters and a use-after-free on the socket.
--

--- Patch [1]: [PATCH v7 1/5] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit ab2ff810b0276df9b5c38648b4470daa9abe3c97
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: use unconditional synchronize_rcu() in isotp_release()

This commit addresses a use-after-free timer callback on a freed socket. It
makes isotp_release() unconditionally call synchronize_rcu() before cancelling
timers, regardless of so->bound, to prevent races with NETDEV_UNREGISTER.

Fixes: 14a4696bc311 ("can: isotp: isotp_release(): omit unintended hrtimer restart on socket release")
Closes: https://lore.kernel.org/linux-can/20260707085210.6B6C01F000E9@smtp.kernel.org/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297e..d30937345bcdc 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1237,11 +1237,18 @@ static int isotp_release(struct socket *sock)
[ ... ]
>  				dev_put(dev);
> -				synchronize_rcu();
>  			}
>  		}
>  	}
>  
> +	/* Always wait for a grace period before touching the timers below.
> +	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
> +	 * filters and cleared so->bound in isotp_notify() without waiting
> +	 * for in-flight isotp_rcv() callers to finish, so this call must not
> +	 * be skipped just because so->bound is already 0 here.
> +	 */
> +	synchronize_rcu();

[Severity: Medium]
Will this unconditional synchronize_rcu() allow unprivileged users to spam RCU
grace periods?

If an unprivileged user repeatedly creates and immediately closes unbound CAN
ISOTP sockets, it seems this path will be hit every time. While previously this
required a successful bind to a valid CAN interface, this patch makes the call
unconditional, which might allow it to be triggered on any system with the CAN
ISOTP module available.

> +
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->rxtimer);

[Severity: Critical]
This is a pre-existing issue, but does isotp_bind() lack rtnl_lock()
protection?

Looking at isotp_bind(), it acquires lock_sock() but does not appear to acquire
rtnl_lock(). Can this allow a race with device unregistration?

net/can/isotp.c:isotp_bind() {
    ...
	dev = dev_get_by_index(net, addr->can_ifindex);
	...
	if (isotp_register_rxid(so))
		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
				isotp_rcv, sk, "isotp", sk);
    ...
}

If an unprivileged user concurrently binds a CAN ISOTP socket to a virtual CAN
device while deleting the device, it seems this could race with the device
unregistration notifier in isotp_notify().

If the notifier runs before so->ifindex is set, it might fail to unregister the
filters, allowing isotp_bind() to register the filters on the dying device.

When the socket is closed, isotp_release() handles it:

net/can/isotp.c:isotp_release() {
    ...
		dev = dev_get_by_index(net, so->ifindex);
		if (dev) {
			if (isotp_register_rxid(so))
				can_rx_unregister(net, dev, so->rxid,...);
    ...
}

If dev_get_by_index() fails because the device is now unlisted, it appears the
CAN RX filters are never unregistered, which could leave a dangling
struct receiver in the device's dev_rcv_lists.

If a delayed packet is processed before the device is freed, could isotp_rcv()
be invoked with the freed socket pointer, resulting in a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-isotp-fixes-v7-0-f03af24d8397@hartkopp.net?part=1

Re: [PATCH v7 3/5] can: isotp: fix race between RX/TX timers and frame reception

[sashiko-bot]

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

New issues:
- [High] Time-of-check to time-of-use (TOCTOU) race in `err_out_drop` corrupts the state of concurrent transfers.
- [High] Reordering timer cancellation before state reset allows RX handlers to re-arm stale timers on an idle socket.
- [High] Missing memory barrier in `err_out_drop` allows store reordering, causing data corruption.
- [High] Suppressing the timeout error in the TX timer handler causes false success and indefinite blocking.

Pre-existing issues:
- [High] `hrtimer_cancel()` calls in RX paths race with concurrent `sendmsg()`, permanently hanging new transfers.
- [High] `isotp_rcv_echo()` unconditionally clears `cfecho` before generation validation, corrupting new transfers.
--

--- Patch [3]: [PATCH v7 3/5] can: isotp: fix race between RX/TX timers and frame reception ---
commit c41687d56855df1b5e33cb5d7468ecf5646f96cd
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: fix race between RX/TX timers and frame reception

This commit addresses a race where hrtimer_cancel() waits for a concurrent
timer handler to finish, potentially allowing the handler to reset the state
machine. It introduces a generation counter to disambiguate new transfers
from aborted ones.

Link: https://lore.kernel.org/linux-can/20260710142146.BDAE61F000E9@smtp.kernel.org/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 44c044eb83e12..f05703ddd2eae 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -371,6 +372,7 @@ static void isotp_send_cframe(struct isotp_sock *so);
>  static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
>  {
>  	struct sock *sk = &so->sk;
> +	u32 gen = READ_ONCE(so->tx_gen);
>  
>  	if (so->tx.state != ISOTP_WAIT_FC &&
>  	    so->tx.state != ISOTP_WAIT_FIRST_FC)
> @@ -378,6 +380,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
>  
>  	hrtimer_cancel(&so->txtimer);
>  

[Severity: High]
This is a pre-existing issue, but could canceling the timer before validating
the generation counter create a race that permanently hangs new transfers?

If the TX timer concurrently times out, the handler resets the state to
ISOTP_IDLE. A concurrent sendmsg() could then immediately claim the socket,
increment tx_gen, and arm txtimer for a new transfer.

When hrtimer_cancel() resumes from blocking in isotp_rcv_fc(), wouldn't it
inadvertently cancel the newly armed timer? Even though the subsequent tx_gen
check aborts processing the stale frame, the new transfer is left without a
watchdog timer and could hang permanently if a frame is dropped.

This exact same race appears to exist in isotp_rcv_echo() as well.

> +	/* hrtimer_cancel() may have let isotp_tx_timer_handler() give up on
> +	 * this job (state changed) or a new sendmsg() claim and reach the
> +	 * same state again (tx_gen changed) - either way this FC is stale.
> +	 */
> +	if ((so->tx.state != ISOTP_WAIT_FC &&
> +	     so->tx.state != ISOTP_WAIT_FIRST_FC) ||
> +	    READ_ONCE(so->tx_gen) != gen)
> +		return 1;
> +
[ ... ]
> @@ -870,6 +889,7 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
>  	struct sock *sk = (struct sock *)data;
>  	struct isotp_sock *so = isotp_sk(sk);
>  	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> +	u32 gen = READ_ONCE(so->tx_gen);
>  
>  	/* only handle my own local echo CF/SF skb's (no FF!) */
>  	if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
> @@ -881,6 +901,14 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
>  	/* local echo skb with consecutive frame has been consumed */
>  	so->cfecho = 0;
>  

[Severity: High]
This is a pre-existing issue, but does unconditionally clearing the echo tag
here corrupt concurrent new transfers?

Since the cleanup happens before the tx_gen check, if a new transfer starts
while hrtimer_cancel() is blocking, the new transfer will set its own cfecho
tag. Won't this delayed store to cfecho overwrite the new transfer's tag,
causing its future echo frames to be dropped and the transfer to hang?

> +	/* hrtimer_cancel() may have let isotp_tx_timer_handler() give up
> +	 * on this job (state changed) or a new sendmsg() claim and reach
> +	 * ISOTP_SENDING again (tx_gen changed) - either way this echo is
> +	 * stale.
> +	 */
> +	if (so->tx.state != ISOTP_SENDING || READ_ONCE(so->tx_gen) != gen)
> +		return;
> +
[ ... ]
> @@ -1143,13 +1179,20 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  	return size;
>  
>  err_event_drop:
> -	/* got signal: force tx state machine to be idle */
> -	so->tx.state = ISOTP_IDLE;
> +	/* got signal: cancel timers before touching state, so a concurrent
> +	 * sendmsg() can't claim ISOTP_SENDING while a stale timer is armed
> +	 */
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);

[Severity: High]
By reordering the timer cancellation before the state reset, is there a risk
that RX handlers could re-arm stale timers?

While spinning here to cancel timers, the socket is left in an active state
(like ISOTP_SENDING). A concurrent RX handler, such as isotp_rcv_echo(), could
pass its state checks and arm a timer (txfrtimer or txtimer) after we have
already finished canceling them here.

When the state is subsequently forced to ISOTP_IDLE below, it leaves an armed
timer on an idle socket. If a new sendmsg() then claims the socket, could
this stale timer fire and overwrite the new transfer's timeout or falsely abort
it?

>  err_out_drop:
> -	/* drop this PDU and unlock a potential wait queue */
> -	so->tx.state = ISOTP_IDLE;
> +	/* drop this PDU and unlock a potential wait queue, but only if no
> +	 * concurrent sendmsg() has since claimed the socket for a new
> +	 * transfer while we were cancelling timers above
> +	 */
> +	if (READ_ONCE(so->tx_gen) == my_gen) {
> +		so->cfecho = 0;
> +		so->tx.state = ISOTP_IDLE;
> +	}

[Severity: High]
Could this non-atomic check create a TOCTOU (time-of-check to time-of-use)
race that corrupts concurrent transfers?

If err_event_drop cancels timers, a concurrent RX handler could have already
set the state to ISOTP_IDLE. A concurrent sendmsg() could then claim the
socket and set the state to ISOTP_SENDING.

If this thread evaluates READ_ONCE(so->tx_gen) == my_gen just before the new
transfer bumps the generation, it will then execute so->tx.state = ISOTP_IDLE.
Won't this unconditionally corrupt the new transfer's lockless state machine?

[Severity: High]
Is a memory barrier missing here when releasing the state machine?

Since there is no smp_store_release(), the compiler or CPU might reorder
the stores to cfecho and tx.state. If ISOTP_IDLE becomes visible first, a
concurrent sendmsg() could claim the socket and set a new cfecho tag.

Wouldn't the delayed store to so->cfecho = 0 then erroneously clear the tag
for the new transfer, causing it to drop its own echoes and hang?

>  	wake_up_interruptible(&so->wait);
>  
>  	return err;

Looking at isotp_tx_timer_handler() in the same file:

	if (READ_ONCE(so->tx_gen) == gen) {
		/* report 'communication error on send' */
		sk->sk_err = ECOMM;
		if (!sock_flag(sk, SOCK_DEAD))
			sk_error_report(sk);
	}

[Severity: High]
Will suppressing the timeout error here cause the original waiting thread to
block indefinitely?

If a new transfer claims the socket (tx_gen != gen), the error is skipped,
but the original sendmsg() thread waiting in wait_tx_done is still woken up.
Because it checks so->tx.state == ISOTP_IDLE (which is false, since the new
transfer set it to ISOTP_SENDING), it goes back to sleep, effectively waiting
for an unrelated transfer to finish.

When it finally wakes up, sock_error() will return 0 because the error was
suppressed, masking the timeout failure as a successful transmission.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-isotp-fixes-v7-0-f03af24d8397@hartkopp.net?part=3