Re: [PATCH net v6 2/2] can: j1939: serialize TX with session shutdown

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message covers only part of what the patch changes.

--

can: j1939: serialize TX with session shutdown

This patch adds a per-session tx_send_lock and a tx_send_shutdown flag. It
routes session frame transmission through a new j1939_session_tx_send()
helper, which drops frames with -ESHUTDOWN once shutdown has been
published or the state is no longer ACTIVE. The state writes in activate,
deactivate and cancel now happen under the new lock.

Link: https://bugtracker.nebusec.ai/f/11143

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index fbeb858956e03..390923a050469 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -1350,10 +1390,13 @@ j1939_session_deactivate_activate_next(struct j1939_session *session)
>  
>  static bool j1939_session_fail(struct j1939_session *session, int err)
>  {
> -	bool deactivate = false;
> +	bool deactivate = false, tx_send_shutdown;
>  
>  	j1939_session_list_lock(session->priv);
> -	if (session->state == J1939_SESSION_ACTIVE) {
> +	spin_lock(&session->tx_send_lock);
> +	tx_send_shutdown = session->tx_send_shutdown;
> +	spin_unlock(&session->tx_send_lock);
> +	if (!tx_send_shutdown && session->state == J1939_SESSION_ACTIVE) {
>  		session->err = err;
>  		deactivate = j1939_session_deactivate_locked(session);
>  	}

[Severity: Low]

This isn't a bug, but the commit message only covers part of this change:

  Prevent a timer callback from sending session frames after a concurrent
  terminal state transition. Serialize the send with the state change and
  reject frames once shutdown has been published.

That description matches j1939_session_tx_send(). The patch also adds
tx_send_shutdown checks to paths that do not send frames:

  - j1939_session_fail() stops recording session->err and stops
    deactivating once tx_send_shutdown is set.

  - __j1939_session_cancel() returns early, so no abort frame is sent and
    j1939_sk_send_loop_abort() is not called. The abort frame is also now
    sent while tx_send_lock is held.

  - j1939_session_cancel() gets an outer copy of the same check.

  - __j1939_tp_set_rxtimeout() gets a new check.

  - The new -ESHUTDOWN result skips the netdev_alert() in
    j1939_session_tx_dat() and is silently dropped in j1939_tp_txtimer().

Are most of these checks needed? Only j1939_session_timers_shutdown_locked()
sets tx_send_shutdown. It runs under active_session_list_lock and sets
rxtimer_shutdown in the same section, and rxtimer_shutdown is never
cleared:

j1939_session_timers_shutdown_locked() {
	lockdep_assert_held(&session->priv->active_session_list_lock);
	j1939_session_tx_send_shutdown(session);
	spin_lock(&session->rxtimer_lock);
	session->rxtimer_shutdown = true;
	spin_unlock(&session->rxtimer_lock);
}

As a result, the new check in __j1939_tp_set_rxtimeout() repeats the
existing rxtimer_shutdown test.

The check in __j1939_session_cancel() can never fire either, because
j1939_session_cancel() already tests the same flag under the same list
lock.

Behaviour only seems to change in j1939_simple_recv(). It publishes the
shutdown while the state is still ACTIVE, drops the list lock, drains the
timers, and only deactivates later:

	j1939_session_list_lock(priv);
	session = j1939_session_get_simple(priv, skb);
	if (session)
		j1939_session_timers_shutdown_locked(session);
	j1939_session_list_unlock(priv);

Within that window, a txtimer error such as -ENOMEM from
j1939_simple_txnext() reaches j1939_session_fail(). It is now ignored,
where before it set session->err and deactivated the session. That may be
intended, since the echo has already confirmed the send.

Could the commit message mention this change and explain why it is safe?

The patch is tagged for stable and adds a new lock around j1939_send_one()
on every session TX path. Could the message also describe the user-visible
symptom being fixed? For example, stray TP.DT, CTS, EOMA or simple frames
showing up on the bus after a concurrent terminal transition.

Could the redundant checks also be dropped?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793788955.git.zihanx%40nebusec.ai