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

[Chaosheng Qu <quchaosheng000406@163.com>]

Hi,

1/4 was rewritten in v8 and v9 is current, but both High findings were real
in v7 as posted.

The commit message claim was wrong. Only j1939_session_cancel() took
active_session_list_lock; j1939_tp_set_rxtimeout() still did cancel, get and
start without it, so the lock only serialized the cancels against each other.

The double-arm is the same problem, and the WAITING_ABORT re-arm is what made
it reachable: the second caller found the state already changed and armed on
top of the timer the first one had queued. Two gets, one expiry, one put, so
the session and its netdev reference leak and unregister_netdevice() waits.

v8 moves the lock into j1939_tp_set_rxtimeout() and routes both armers
through one helper:

	if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
		j1939_session_put(session);
	j1939_session_get(session);
	hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
		      HRTIMER_MODE_REL_SOFT);

and drops the WAITING_ABORT re-arm, so only the ACTIVE -> WAITING_ABORT
transition arms the abort timer.

Still open: try_to_cancel() returns -1 while the callback runs and does not
dequeue a timer requeued from it, so an arm that races the expiry can still
take a reference it cannot pair. Your review of v9 1/4 says the same thing.
The next version needs an armed flag taken under the list lock.

The Medium was real too. On a receiver the second timers_cancel()/cancel()
pair re-armed the abort timer, then deactivate_activate_next() moved the
session to DONE without cancelling it, so the expiry skipped the
WAITING_ABORT branch, took the generic else and queued RX_ABORT for a
finished session. v9 has no re-arm and j1939_xtp_rx_rts_session_active()
deactivates for both roles.

Lows, all accepted: the subject was retitled in v8; the Fixes should be
d966635b384b, not 9d71dd0c7009; abort_sent never distinguished anything,
since __j1939_session_cancel() always sets WAITING_ABORT and skips the abort
for broadcasts; and restarting the 500ms deadline on every cancel is a real
behaviour change the v7 message did not describe.

Thanks,
Chaosheng

pw-bot: cr