Re: [PATCH net v7 1/4] can: j1939: arm the abort timer under the session list lock

[netdev-bot+sashiko@kernel.org]

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

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

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 commit message says the rearms take the list lock "so both
  armers are serialized".
- [High] This is a new regression: two j1939_session_cancel() calls
  running at the same time on the same session now double-arm the…
- [Medium] j1939_session_cancel() now also re-arms the rxtimer when the
  state is already WAITING_ABORT.
- [Low] The patch fixes a deadlock (hrtimer_cancel() under
  active_session_list_lock against j1939_tp_rxtimer(), which takes that…
- [Low] The subject, 'can: j1939: arm the abort timer under the session
  list lock', does not describe the change.
- [Low] This behaviour change is not described in the commit message.
- [Low] The flag abort_sent and the comment 'whether the abort frame just
  went out or the session is already in WAITING_ABORT' in…

--

can: j1939: arm the abort timer under the session list lock

This patch moves the rxtimer cancel in j1939_session_cancel() outside of
active_session_list_lock, to avoid a deadlock against j1939_tp_rxtimer().
It then re-arms the abort timer under the lock. It also re-arms the timer
for sessions that are already in J1939_SESSION_WAITING_ABORT.

> can: j1939: arm the abort timer under the session list lock

[Severity: Low]
This isn't a bug, but does the subject describe the change? Before this
patch, j1939_session_cancel() already armed the abort timer under the lock,
because j1939_tp_set_rxtimeout() was called inside
j1939_session_list_lock().

The patch makes two changes. It moves the cancel out of the lock, which is
the deadlock fix. It also re-arms WAITING_ABORT sessions, which is the
stuck-session fix.

Would these be easier to bisect and backport as two separate patches?

[ ... ]

> Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>

[Severity: Low]
Should this have a Fixes: tag and Cc: stable? git blame shows that
j1939_session_cancel(), with j1939_tp_set_rxtimeout() called inside the
lock, comes from commit d966635b384b.

The related fix 1809c82aa073 for the same session leak class carried
"Fixes: 9d71dd0c7009" and "Cc: stable@vger.kernel.org".

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..590a04c453b56 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1161,12 +1161,38 @@ static void __j1939_session_cancel(struct j1939_session *session,
>  static void j1939_session_cancel(struct j1939_session *session,
>  				 enum j1939_xtp_abort err)
>  {
> +	bool abort_sent = false;
> +
> +	/* Cancel the abort timer before taking the list lock.
> +	 *
> +	 * hrtimer_cancel() waits for a callback that is already running, and
> +	 * j1939_tp_rxtimer() takes the same lock, so cancelling the timer while
> +	 * holding active_session_list_lock can deadlock: the lock holder waits
> +	 * for the callback, the callback waits for the lock.
> +	 */
> +	j1939_session_rxtimer_cancel(session);
> +
>  	j1939_session_list_lock(session->priv);
>  
>  	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);
> +		abort_sent = true;
> +	}

[Severity: Low]
This isn't a bug, but the name abort_sent doesn't quite match the code.
__j1939_session_cancel() skips j1939_xtp_tx_abort() for broadcast
sessions:

net/can/j1939/transport.c:__j1939_session_cancel() {
    ...
	session->state = J1939_SESSION_WAITING_ABORT;
	/* do not send aborts on incoming broadcasts */
	if (!j1939_cb_is_broadcast(&session->skcb)) {
    ...
}

abort_sent is still set to true in that case. __j1939_session_cancel()
also always sets J1939_SESSION_WAITING_ABORT, so the state check below is
already true whenever abort_sent is.

Is the flag needed at all? The comment below ("whether the abort frame just
went out or the session is already in WAITING_ABORT") describes two cases
that the code never distinguishes.

> +
> +	/* Re-arm under the lock, whether the abort frame just went out or the
> +	 * session is already in WAITING_ABORT and only needs its timer back.
> +	 *
> +	 * hrtimer_start() does not wait for a running callback, so arming here
> +	 * cannot deadlock, and taking the lock closes the window against
> +	 * j1939_tp_set_rxtimeout(), which rearms the same timer from the
> +	 * receive path without holding it.
> +	 */

[Severity: High]
Does taking the lock here actually close the window against
j1939_tp_set_rxtimeout()? That function is unchanged, and it still does
cancel, get and start without active_session_list_lock:

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

None of its callers hold the list lock either. That covers the RX softirq
paths and the txtimer paths: j1939_session_tx_rts(),
j1939_session_tx_dpo(), j1939_session_tx_dat(), j1939_session_tx_cts(),
j1939_session_tx_eoma() and j1939_simple_txnext().

Only one of the two armers takes the lock. Are they really serialized, as
the commit message says ("so both armers are serialized")?

The cancel is now outside the lock. Can this sequence leak a session
reference?

CPU A (j1939_session_cancel)       CPU B (j1939_tp_set_rxtimeout)
j1939_session_rxtimer_cancel()
                                   j1939_session_rxtimer_cancel()
                                     (nothing pending)
                                   j1939_session_get()
                                   hrtimer_start()
j1939_session_list_lock()
__j1939_session_cancel()
j1939_session_get()
hrtimer_start()
  (re-enqueues the already queued timer)

Both sides take a reference, but only one timer is queued.
j1939_tp_rxtimer() calls j1939_session_put() once per expiry. The reverse
order leaks the same way.

Once a kref leaks, j1939_session_destroy() never runs. So netdev_put(),
j1939_priv_put() and, for TX sessions, the sock_put() in
__j1939_session_drop() never happen either. Wouldn't unregister_netdevice()
then hang, which is the leak the commit message says this patch closes?

j1939_cancel_active_session() at unregister doesn't look able to recover
this. It only drops one rxtimer reference through hrtimer_try_to_cancel(),
plus the list reference.

Would it help to use hrtimer_try_to_cancel() under the lock and put when it
returns 1, as j1939_cancel_active_session() does? Another option is to tie
the get to whether the timer was actually inactive.

> +	if (abort_sent || session->state == J1939_SESSION_WAITING_ABORT) {
> +		j1939_session_get(session);
> +		hrtimer_start(&session->rxtimer,
> +			      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
> +			      HRTIMER_MODE_REL_SOFT);
>  	}

[Severity: High]
Can two concurrent j1939_session_cancel() calls on the same session
double-arm the rxtimer here? For example, CPU A is in
j1939_tp_txtimer()->j1939_session_cancel(). CPU B is in an RX
out_session_cancel path such as j1939_xtp_rx_dat_one() or
j1939_xtp_rx_cts_one():

CPU A                                CPU B
j1939_session_rxtimer_cancel()
  hrtimer_cancel() returns 1, put
                                     j1939_session_rxtimer_cancel()
                                       (nothing pending)
j1939_session_list_lock()
__j1939_session_cancel()
  state = WAITING_ABORT
j1939_session_get()
hrtimer_start()
j1939_session_list_unlock()
                                     j1939_session_list_lock()
                                     state == WAITING_ABORT
                                     j1939_session_get()
                                     hrtimer_start()
                                       (re-enqueues the queued timer)
                                     j1939_session_list_unlock()

That is two gets for a single expiry of j1939_tp_rxtimer(), so one kref
would never be dropped.

Before this patch, CPU B saw WAITING_ABORT under the lock and did nothing.
The WAITING_ABORT re-arm combined with the lockless cancel seems to be what
introduces this.

The commit message says "The reference accounting stays balanced: one
j1939_session_get() per hrtimer_start()". Doesn't that only hold if
hrtimer_start() is never called on a timer that is already queued?

[Severity: Medium]
Does the WAITING_ABORT re-arm conflict with the existing workaround in
j1939_xtp_rx_rts_session_active()? That code still assumes
j1939_session_cancel() won't restart the timer in WAITING_ABORT:

net/can/j1939/transport.c:j1939_xtp_rx_rts_session_active() {
    ...
		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);
		}
    ...
}

Take a receive session where the peer sends a second RTS. The first
j1939_session_timers_cancel() and j1939_session_cancel() pair at the top of
this function moves the session to WAITING_ABORT and arms the timer.

The second pair then kills that timer. With this patch,
j1939_session_cancel() re-arms it with a new reference.
j1939_session_deactivate_activate_next() then moves the session to DONE
without cancelling the timer.

When that timer fires on the DONE session, j1939_tp_rxtimer() takes the
generic else branch. The state check fails, but it still runs:

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

Doesn't this deliver an extra J1939_ERRQUEUE_RX_ABORT to matching sockets
for a session that has already finished? It also keeps the session, netdev
and priv references alive for another J1939_XTP_ABORT_TIMEOUT_MS.

The comment in j1939_xtp_rx_rts_session_active() would also no longer
match what j1939_session_cancel() does.

[Severity: Low]
Is this change in timeout semantics intended? Every j1939_session_cancel()
on a session already in WAITING_ABORT now cancels the pending abort timer
and starts a fresh J1939_XTP_ABORT_TIMEOUT_MS timer.

That includes the calls from j1939_tp_txtimer(), which don't cancel the
timers first:

		if (session->skcb.addr.type != J1939_SIMPLE) {
			j1939_session_cancel(session, J1939_XTP_ABORT_OTHER);

Before this patch, those calls did nothing and the original abort deadline
stood. Now the forced deactivation happens 500ms after the last cancel,
not 500ms after the abort.

A peer could keep sending malformed TP frames less than 500ms apart, for
example DT frames with skb->len != 8 that reach out_session_cancel in
j1939_xtp_rx_dat_one(). Wouldn't that keep the session in WAITING_ABORT on
the active list indefinitely?

The commit message only says such sessions "also gets its timer back".
Could it mention that an existing pending deadline is also pushed back?

>  
>  	j1939_session_list_unlock(session->priv);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179410166344.60581.3898011861151549156%40163.com