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

[netdev-bot+sashiko@kernel.org]

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