Re: [PATCH net v3 3/3] can: j1939: enable context analysis

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Medium] The commit message says "Keep the analysis opt-in and its
  annotations separate from the J1939 timer and send synchronization…
- [Low] The patch marks the hrtimer object itself as guarded: `struct
  hrtimer txtimer __guarded_by(&txtimer_lock)`…

--

can: j1939: enable context analysis

This enables Clang context analysis for net/can/j1939/transport.o. It
annotates the TX timer state and the TX-send shutdown flag with
__guarded_by() and adds __acquires()/__releases() to the session-list lock
wrappers. It also adds a few comments around the TX timer pinning logic.

> Keep the analysis opt-in and its annotations separate from the J1939
> timer and send synchronization fixes.

[Severity: Medium]
Is this description accurate for the series?

The earlier fix in this series, "can: j1939: fix session reference leaks
on timer rearm" (Fixes: 9d71dd0c7009, Cc: stable), already added this to
j1939_session_new():

	spin_lock_init(&session->tx_send_lock);
	scoped_guard(spinlock_init, &session->txtimer_lock) {
		hrtimer_setup(&session->txtimer, j1939_tp_txtimer,
			      CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
	}

At runtime the spinlock_init guard does nothing beyond spin_lock_init().
It only makes the analysis treat txtimer_lock as held while guarded
members are initialized. Here that is only needed because of the
__guarded_by(&txtimer_lock) on txtimer that this patch adds. By
comparison, tx_send_lock uses plain spin_lock_init().

The same fix also added the "Keep this alias across unlock" comment in
j1939_session_txtimer_cancel(). That comment seems to belong with the
context_unsafe_alias() added here.

The spinlock_init guard class comes from the recent commit d084a73714f8
("compiler-context-analysis: Introduce scoped init guards"). A stable
tree older than that has no class_spinlock_init. Would a backport of the
reference leak fix fail to build as written?

Could the scoped_guard(spinlock_init) move into this patch, next to the
__guarded_by() it supports? The stable fix would then use plain
spin_lock_init() and hrtimer_setup(). Otherwise, could the separation
claim be dropped from the commit message?

> diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
> index 6bf55b7dfffcc..d664f32bbb8f2 100644
> --- a/net/can/j1939/j1939-priv.h
> +++ b/net/can/j1939/j1939-priv.h

[ ... ]

> @@ -293,22 +293,22 @@ struct j1939_session {

[ ... ]

> +	/* Protects TX timer rearming and cancellation. */
> +	spinlock_t txtimer_lock;
> +	struct hrtimer txtimer __guarded_by(&txtimer_lock);
> +	bool txtimer_qref __guarded_by(&txtimer_lock);

[ ... ]

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 570e2b8d08263..7fcf66e93afd7 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -331,6 +333,7 @@ static void j1939_session_txtimer_cancel(struct j1939_session *session)
>  	session->txtimer_shutdown = true;
>  	timer = &session->txtimer;
>  	/* Keep this alias across unlock; blocking cancel must run outside the lock. */
> +	context_unsafe_alias(timer);
>  	ret = hrtimer_try_to_cancel(timer);
>  	if (ret == 1) {
>  		WARN_ON_ONCE(!session->txtimer_qref);

[Severity: Low]
Does the __guarded_by(&txtimer_lock) on txtimer match how this function
uses it?

The pointer is taken under txtimer_lock. Later in the same function, the
blocking cancel runs after the lock has been dropped:

	spin_unlock_bh(&session->txtimer_lock);

	if (ret < 0) {
		/* Keep the blocking cancel outside txtimer_lock. */
		ret = hrtimer_cancel(timer);

This has to run unlocked because j1939_tp_txtimer() takes txtimer_lock
itself. So the code deliberately uses the object without the lock that
the new annotation says guards it.

include/linux/compiler-context-analysis.h documents context_unsafe_alias()
as a context lock alias barrier:

 * @p: pointer aliasing a context lock or object containing context locks

struct hrtimer is not a context lock and does not contain one. Here the
helper seems to be used only so the analysis loses track of the alias.
That also stops checking the locked hrtimer_try_to_cancel(timer) call
right after it.

Would context_unsafe() around the unlocked hrtimer_cancel(), with a
stated reason, fit better? Another option is to leave the hrtimer
unannotated and guard only the bookkeeping bools.

The RX peer, j1939_session_rxtimer_cancel_sync(), uses the same pattern:
the alias is taken under the lock and hrtimer_cancel() is then called
unlocked. It has neither __guarded_by() nor an alias helper, so the two
timers end up documented differently.

Could the commit message also say that this unlocked access is exempted
from the analysis?

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