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

On 14.07.26 12:34, [email protected] wrote:
> 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.
> --
> 

Well yes :-/

V14 candidate

> --- 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?
>