Re: [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates

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

Pre-existing issues:
- [High] Torn reads of ival1, ival2, and count lead to corrupted TX_EXPIRED API notifications.
--

--- Patch [6]: [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates ---
commit 69a20e6fcc24540387b82e9501090dfb55f155cf
Author: Oliver Hartkopp <[email protected]>

can: bcm: extend bcm_tx_lock usage for data and timer updates
    
Stage new CAN frame content for an existing tx op into a kmalloc()'d
buffer and validate it there, mirroring the approach already used in
bcm_rx_setup(). Only copy the validated data into op->frames while
holding op->bcm_tx_lock, so bcm_can_tx() and bcm_tx_timeout_handler()
can no longer observe a partially updated or unvalidated frame.

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

[ ... ]

> @@ -486,8 +492,17 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
>  {
>  	struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
>  	struct bcm_msg_head msg_head;
> +	bool tx_ival1, tx_ival2;
>  
> -	if (op->kt_ival1 && (op->count > 0)) {
> +	/* snapshot kt_ival1/kt_ival2/count under lock to avoid torn
> +	 * ktime_t reads racing with concurrent bcm_tx_setup() updates
> +	 */
> +	spin_lock_bh(&op->bcm_tx_lock);
> +	tx_ival1 = op->kt_ival1 && (op->count > 0);
> +	tx_ival2 = !!op->kt_ival2;
> +	spin_unlock_bh(&op->bcm_tx_lock);
> +
> +	if (tx_ival1) {
>  		bcm_can_tx(op, NULL);
>  		if (!op->count && (op->flags & TX_COUNTEVT)) {
>  

[Severity: High]
This isn't a bug introduced by this patch, but the lockless read of
op->count here can see a newly written value after bcm_can_tx() drops the
lock.

If bcm_tx_setup() runs concurrently via sendmsg() and mutates op->count,
could this condition falsely fail and drop the TX_EXPIRED event?

Additionally, in the lines immediately following this (when setting up
msg_head), op->ival1, op->ival2, and op->count are read locklessly:

			msg_head.count   = op->count;
			msg_head.ival1   = op->ival1;
			msg_head.ival2   = op->ival2;

Since bcm_tx_setup() modifies these multi-word structures under
op->bcm_tx_lock, does this lockless read risk tearing the tv_sec and
tv_usec halves on 32-bit systems, sending corrupted timevals to userspace?

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