Re: [PATCH v7 05/10] can: bcm: add missing rcu list annotations and operations

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unlocked 64-bit `ktime_t` assignments allow torn reads on 32-bit systems, causing incorrect timer expiries.
--

--- Patch [5]: [PATCH v7 05/10] can: bcm: add missing rcu list annotations and operations ---
commit 356a7e056868b1df56b247c6708d4c5bdd80c8f0
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add missing rcu list annotations and operations

This commit adds missing list_add_rcu() calls in bcm_tx_setup() and
bcm_rx_setup() to ensure bcm_op structures are properly initialized
before being traversed by bcm_proc_show() under rcu_read_lock(). It
also fixes the mirroring removal side in bcm_release() by properly
unlinking operations with list_del_rcu() before freeing them via
bcm_remove_op().

Link: https://lore.kernel.org/linux-can/20260610094654.A1FFE1F00893@smtp.kernel.org/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 340b7fc89744e..fca7c714cc52f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1163,6 +1163,10 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		op->flags |= TX_ANNOUNCE;
>  	}
>  
> +	/* add this bcm_op to the list of the tx_ops? */
> +	if (add_op_to_list)
> +		list_add_rcu(&op->list, &bo->tx_ops);
> +

[Severity: High]
This is a pre-existing issue, but while reviewing this setup code I noticed
a potential torn read problem on 32-bit systems.

Earlier in bcm_tx_setup(), the 64-bit ktime_t values are written under
bcm_tx_lock:

	spin_lock_bh(&op->bcm_tx_lock);
	op->ival1 = msg_head->ival1;
	op->ival2 = msg_head->ival2;
	op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
	op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
	spin_unlock_bh(&op->bcm_tx_lock);

However, if the hrtimer fires on another CPU concurrently,
bcm_tx_timeout_handler() reads these 64-bit values without acquiring
bcm_tx_lock:

	if (op->kt_ival1 && (op->count > 0)) {
		bcm_can_tx(op, NULL);
		// ...
	} else if (op->kt_ival2) {

And bcm_tx_set_expiry() also reads them unlocked to set the timer expiry:

	if (op->kt_ival1 && op->count)
		ival = op->kt_ival1;
	else if (op->kt_ival2)
		ival = op->kt_ival2;

Could an unlocked concurrent read of op->kt_ival1 or op->kt_ival2 cause a
torn read on 32-bit systems, resulting in a corrupted or negative timer
expiry?

If this corrupted value is passed to hrtimer_set_expires(), might it schedule
the timer in the past and lead to an immediate expiration and subsequent
softirq storm for repeating cyclic timers?

>  	if (op->flags & TX_ANNOUNCE)
>  		bcm_can_tx(op, NULL);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v7-0-c78896b342e4@hartkopp.net?part=5