Re: [PATCH net v5 1/3] can: j1939: fix TX timer reference accounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] can: j1939: race in j1939_session_txtimer_cancel_async_locked() drops ref
--

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

can: j1939: fix TX timer reference accounting

This commit reworks J1939 session TX timer reference accounting so a single
kref reference covers the queued and running lifetime of session->txtimer,
adding synchronization around timer arming, cancellation, and callback
completion.

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

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..4958f7c47ca44 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -301,9 +301,78 @@ void j1939_session_put(struct j1939_session *session)
>  	kref_put(&session->kref, __j1939_session_release);
>  }
>  
> -static void j1939_session_txtimer_cancel(struct j1939_session *session)
> +static void j1939_session_txtimer_cancel(struct j1939_session *session,
> +					 bool shutdown)
>  {
> -	if (hrtimer_cancel(&session->txtimer))
> +	bool put_ref = false;
> +	int ret;
> +
> +	spin_lock_bh(&session->txtimer_lock);
> +	session->txtimer_cancelers++;
> +	if (shutdown)
> +		session->txtimer_shutdown = true;
> +	ret = hrtimer_try_to_cancel(&session->txtimer);
> +	if (ret == 1) {
> +		if (!WARN_ON_ONCE(!session->txtimer_ref)) {
> +			session->txtimer_ref = false;
> +			put_ref = true;
> +		}
> +	}
> +	spin_unlock_bh(&session->txtimer_lock);
> +
> +	/* Never wait for a softirq callback while holding txtimer_lock. */
> +	if (ret < 0) {
> +		ret = hrtimer_cancel(&session->txtimer);
> +		if (ret == 1) {
> +			spin_lock_bh(&session->txtimer_lock);
> +			if (!WARN_ON_ONCE(!session->txtimer_ref)) {
> +				session->txtimer_ref = false;
> +				put_ref = true;
> +			}
> +			spin_unlock_bh(&session->txtimer_lock);
> +		}
> +	}
> +	spin_lock_bh(&session->txtimer_lock);
> +	if (WARN_ON_ONCE(!session->txtimer_cancelers))
> +		session->txtimer_cancelers = 0;
> +	else
> +		session->txtimer_cancelers--;
> +	spin_unlock_bh(&session->txtimer_lock);
> +
> +	if (put_ref)
> +		j1939_session_put(session);
> +}
> +
> +static bool
> +j1939_session_txtimer_cancel_async_locked(struct j1939_session *session)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&session->txtimer_lock);
> +	session->txtimer_shutdown = true;
> +	ret = hrtimer_try_to_cancel(&session->txtimer);
> +	/* Let a queued successor finish promptly after callback exit. */
> +	if (ret < 0 && session->txtimer_ref &&
> +	    hrtimer_is_queued(&session->txtimer))
> +		hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);

[Severity: Critical]
Can j1939_session_txtimer_cancel_async_locked() race with a concurrent
hrtimer_cancel() in j1939_session_txtimer_cancel() and leave session->txtimer
queued without a session reference?

Unlike j1939_tp_schedule_txtimer(), j1939_session_txtimer_cancel_async_locked()
does not check session->txtimer_cancelers before calling hrtimer_start(), and
hrtimer_cancel() in j1939_session_txtimer_cancel() runs without holding
session->txtimer_lock.

If j1939_tp_txtimer() on CPU 0 has re-armed a successor timer and is exiting
after releasing session->txtimer_lock:

CPU 0 (j1939_tp_txtimer)
  queued = hrtimer_is_queued(&session->txtimer); /* true, keeps txtimer_ref */
  spin_unlock_bh(&session->txtimer_lock);
  return HRTIMER_NORESTART;
  /* still running in __run_hrtimer() before clearing base->running */

CPU 1 (j1939_session_txtimer_cancel)
  spin_lock_bh(&session->txtimer_lock);
  session->txtimer_cancelers++;
  ret = hrtimer_try_to_cancel(&session->txtimer); /* returns -1 */
  spin_unlock_bh(&session->txtimer_lock);
  ret = hrtimer_cancel(&session->txtimer);

CPU 2 (j1939_session_txtimer_cancel_async_locked)
  /* holds session->txtimer_lock */
  ret = hrtimer_try_to_cancel(&session->txtimer); /* returns -1 */
  if (ret < 0 && session->txtimer_ref &&
      hrtimer_is_queued(&session->txtimer)) /* evaluates true */

If CPU 0 then clears base->running in __run_hrtimer() and CPU 1's
hrtimer_cancel() dequeues the pending successor timer and returns 1 before
CPU 2 calls hrtimer_start():

CPU 2
  hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);
  /* re-queues session->txtimer and returns false (ret == -1) */

CPU 1
  if (ret == 1) {
      spin_lock_bh(&session->txtimer_lock);
      session->txtimer_ref = false;
      put_ref = true;
      spin_unlock_bh(&session->txtimer_lock);
  }
  ...
  j1939_session_put(session);

Wouldn't this leave session->txtimer queued in the hrtimer subsystem with
session->txtimer_ref set to false and its kref dropped, leading to a
WARN_ON_ONCE(!session->txtimer_ref) and a use-after-free of struct
j1939_session when j1939_tp_txtimer() fires? Should
j1939_session_txtimer_cancel_async_locked() avoid calling hrtimer_start() when
session->txtimer_cancelers is non-zero (since the synchronous canceler in
j1939_session_txtimer_cancel() is already waiting to cancel the timer)?

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