Re: [PATCH 1/1] can: j1939: fix session reference leak on TX timer restart

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] can: j1939: session refcount underflow in j1939_tp_schedule_txtimer()
- [High] can: j1939: race between j1939_tp_schedule_txtimer() and txtimer cancellation
--

commit 132d2449245f60a20ddade9b51a9c2efc0a02136
Author: Zihan Xi <zihanx@nebusec.ai>

can: j1939: fix session reference leak on TX timer restart

This patch adds a spinlock to serialize TX timer rearming in
j1939_tp_schedule_txtimer() and reuses the existing session reference when
restarting an already queued TX timer.

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

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..e9af8547c69b4 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -716,9 +716,17 @@ static int j1939_xtp_tx_abort(struct j1939_priv *priv,
>  
>  void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
>  {
> -	j1939_session_get(session);
> +	int ret;
> +
> +	spin_lock_bh(&session->txtimer_lock);
> +	/* Keep the timer's reference when only changing its expiry. */
> +	ret = hrtimer_try_to_cancel(&session->txtimer);

[Severity: High]
Could temporarily dequeuing session->txtimer here race with callers that
cancel the timer without holding session->txtimer_lock?

In net/can/j1939/transport.c, neither j1939_session_txtimer_cancel() nor
j1939_cancel_active_session() acquires session->txtimer_lock:

static void j1939_session_txtimer_cancel(struct j1939_session *session)
{
	if (hrtimer_cancel(&session->txtimer))
		j1939_session_put(session);
}

and in j1939_cancel_active_session():

	if (hrtimer_try_to_cancel(&session->txtimer) == 1)
		j1939_session_put(session);

Previously, j1939_tp_schedule_txtimer() called hrtimer_start() directly on
an already queued timer, updating its expiry under the hrtimer base lock
without transitioning hrtimer_active(&session->txtimer) to false.

With this change, when session->txtimer is queued and its callback is not
running, hrtimer_try_to_cancel(&session->txtimer) dequeues the timer and
returns 1, making hrtimer_active(&session->txtimer) false until
hrtimer_start() runs.

If another CPU calls j1939_session_txtimer_cancel() (for example via
j1939_session_timers_cancel() on session abort or completion, or from
j1939_xtp_rx_cts_one()) or j1939_cancel_active_session() during that window,
hrtimer_cancel() / hrtimer_try_to_cancel() will see the timer as inactive
and return 0. j1939_tp_schedule_txtimer() will then call hrtimer_start() and
re-arm session->txtimer on a session whose timers were already cancelled.

> +	/* A running callback may already have requeued the timer. */
> +	if (ret != 1 && !hrtimer_is_queued(&session->txtimer))
> +		j1939_session_get(session);
>  	hrtimer_start(&session->txtimer, ms_to_ktime(msec),
>  		      HRTIMER_MODE_REL_SOFT);

[Severity: Critical]
What prevents a queued session->txtimer from expiring or being cancelled
between the lockless hrtimer_is_queued() check and hrtimer_start(), causing
a refcount underflow on session->kref?

In kernel/time/hrtimer.c, hrtimer_try_to_cancel() leaves the timer queued
and returns -1 whenever the timer callback is currently running:

	if (!hrtimer_callback_running(timer)) {
		ret = remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE);
		if (ret)
			trace_hrtimer_cancel(timer);
	}

Consider the following sequence across two CPUs when j1939_tp_txtimer() is
running on CPU 0 and has already requeued session->txtimer once (holding one
session->kref reference for the running callback and a second reference for
the queued timer):

1. CPU 1 calls j1939_tp_schedule_txtimer() and takes session->txtimer_lock.
   Because j1939_tp_txtimer() is still running on CPU 0,
   hrtimer_try_to_cancel(&session->txtimer) returns -1 without removing the
   queued timer.

2. CPU 1 checks hrtimer_is_queued(&session->txtimer), which observes
   timer->is_queued == 1, so CPU 1 skips j1939_session_get(session) intending
   to reuse the queued timer's reference.

3. Before CPU 1 enters hrtimer_start() and takes the hrtimer base lock, the
   running j1939_tp_txtimer() callback on CPU 0 finishes (dropping its own
   reference via j1939_session_put(session)), and the queued timer either:
   - immediately expires in __hrtimer_run_queues() / __run_hrtimer() (which
     does not take session->txtimer_lock), dequeuing the timer and running a
     second j1939_tp_txtimer() that drops the second session->kref reference:

	j1939_session_put(session);

	return HRTIMER_NORESTART;

   - or is cancelled by an unlocked j1939_session_txtimer_cancel() or
     j1939_cancel_active_session() on another CPU, which also dequeues the
     timer and drops the second session->kref reference.

4. CPU 1 then executes hrtimer_start(&session->txtimer, ...), which enqueues
   session->txtimer from the inactive state without having taken a new
   session reference.

When that newly started timer subsequently fires in j1939_tp_txtimer() or is
cancelled, j1939_session_put(session) will drop a reference that was never
taken, underflowing session->kref and freeing struct j1939_session while it
is still in use.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792979091.git.zihanx@nebusec.ai?part=1