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

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

Pre-existing issues:
- [High] Unsynchronized reads of 64-bit ktime_t and struct bcm_timeval values cause torn reads on 32-bit systems, leading to corrupted hrtimer expirations.
--

--- Patch [5]: [PATCH v11 05/11] can: bcm: add missing rcu list annotations and operations ---
commit c261663cca88ad03b64923cce2abd1bd069a9797
Author: Oliver Hartkopp <[email protected]>

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 proper initialization when bcm_proc_show()
traverses the operations under rcu_read_lock(). It also fixes a list
removal issue in bcm_release() by unlinking each operation with
list_del_rcu() before removing.

Closes: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 38c3f690c1cd0..940cd9df1cca3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1173,6 +1173,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);
> +
>  	if (op->flags & TX_ANNOUNCE)
>  		bcm_can_tx(op, NULL);
>  

[Severity: High]
This is a pre-existing issue, but can the 64-bit ktime_t timer interval
reads tear on 32-bit systems?

In bcm_tx_setup(), the timer interval values are updated under the lock:

net/can/bcm.c:bcm_tx_setup() {
    ...
    if (op->flags & SETTIMER) {
        spin_lock_bh(&op->bcm_tx_lock);
        ...
        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, when the timer fires, bcm_tx_timeout_handler() and its callee
bcm_tx_set_expiry() appear to read these 64-bit values locklessly:

net/can/bcm.c:bcm_tx_set_expiry() {
    ...
    if (op->kt_ival1 && op->count)
        ival = op->kt_ival1;
    else if (op->kt_ival2)
        ival = op->kt_ival2;
    ...
}

If the softirq handler observes a torn read on a 32-bit system, could it
result in corrupted timer expiration values and lead to an immediate
re-arming loop or stalled transmission?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5