Skip to contentSkip to conversation

can: j1939: Use hrtimer_cancel in j1939_cancel_active_session

PATCH In review Dudu Lu 2 participants 2 messages Apr 13

Subsystems (3)
CAN network drivers CAN network layer CAN-J1939 network layer
lore
Share & tools
lore

Dudu Lu

author
#0 lore raw

PATCH 1/1 can: j1939: Use hrtimer_cancel in j1939_cancel_active_session

j1939_cancel_active_session() uses hrtimer_try_to_cancel() for both txtimer and rxtimer. When hrtimer_try_to_cancel() returns -1, it means the timer callback is currently executing. In this case, the function neither cancels the timer nor drops the session reference via j1939_session_put(). The session is then deactivated while the timer callback may still be running, leading to a potential use-after-free if the callback accesses the session after it has been cleaned up.

Replace hrtimer_try_to_cancel() with hrtimer_cancel() which will wait for an in-progress callback to complete before returning, ensuring the timer is fully stopped before the session is deactivated.

Note: This changes the function to potentially sleep (hrtimer_cancel waits for the callback). The function is called with active_session_list_lock held (a spinlock), so an alternative approach would be to handle the -1 return from hrtimer_try_to_cancel() without blocking. However, if the lock can be converted or the cancel moved outside the lock, hrtimer_cancel() is the cleaner fix.

Signed-off-by Dudu Lu <[email protected]>
---
 net/can/j1939/transport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/can/j1939/transport.c +2 -2
index df93d57907da..996d4c2cbbb0 100644
@@ -2230,9 +2230,9 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
 				 &priv->active_session_list,
 				 active_session_list_entry) {
 		if (!sk || sk == session->sk) {
-			if (hrtimer_try_to_cancel(&session->txtimer) == 1)
+			if (hrtimer_cancel(&session->txtimer))
 				j1939_session_put(session);
-			if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
+			if (hrtimer_cancel(&session->rxtimer))
 				j1939_session_put(session);
 
 			session->err = ESHUTDOWN;
Signature · 1 line
2.39.3 (Apple Git-145)
maintainer
#1 lore raw

Hi Dudu,

Quoted 12 lines · Dudu Lu

j1939_cancel_active_session() uses hrtimer_try_to_cancel() for both txtimer and rxtimer. When hrtimer_try_to_cancel() returns -1, it means the timer callback is currently executing. In this case, the function neither cancels the timer nor drops the session reference via j1939_session_put(). The session is then deactivated while the timer callback may still be running, leading to a potential use-after-free if the callback accesses the session after it has been cleaned up.

Replace hrtimer_try_to_cancel() with hrtimer_cancel() which will wait for an in-progress callback to complete before returning, ensuring the timer is fully stopped before the session is deactivated.

Quoted 6 lines

Note: This changes the function to potentially sleep (hrtimer_cancel waits for the callback). The function is called with active_session_list_lock held (a spinlock), so an alternative approach would be to handle the -1 return from hrtimer_try_to_cancel() without blocking. However, if the lock can be converted or the cancel moved outside the lock, hrtimer_cancel() is the cleaner fix.

Is it real or academical issue? Are there some bug report or exploit?

Best regards,
Oleksij

Signature · 4 lines
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |