linux-kernel - Re: [PATCH] comedi: comedi_test: Prevent timers rescheduling during deletion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 14 Feb 2024 09:46:54 +0000
From: Ian Abbott <abbotti@....co.uk>
To: linux-kernel@...r.kernel.org
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 H Hartley Sweeten <hsweeten@...ionengravers.com>, stable@...r.kernel.org
Subject: Re: [PATCH] comedi: comedi_test: Prevent timers rescheduling during
 deletion

On 13/02/2024 18:10, Ian Abbott wrote:
> The comedi_test devices have a couple of timers (ai_timer and ao_timer)
> that can be started to simulate hardware interrupts.  Their expiry
> functions normally reschedule the timer.  The driver code calls either
> del_timer_sync() or del_timer() to delete the timers from the queue, but
> does not currently prevent the timers from rescheduling themselves so
> synchronized deletion may be ineffective.
> 
> Add a couple of boolean members (one for each timer: ai_timer_enable and
> ao_timer_enable) to the device private data structure to indicate
> whether the timers are allowed to reschedule themselves.  Set the member
> to true when adding the timer to the queue, and to false when deleting
> the timer from the queue in the waveform_ai_cancel() and
> waveform_ao_cancel() functions.
> 
> The del_timer_sync() function is also called from the waveform_detach()
> function, but the timer enable members will already be set to false when
> that function is called, so no change is needed there.
> 
> Fixes: 403fe7f34e33 ("staging: comedi: comedi_test: fix timer race conditions")
> Cc: <stable@...r.kernel.org> # 4.4+
> Signed-off-by: Ian Abbott <abbotti@....co.uk>
> ---
>   drivers/comedi/drivers/comedi_test.c | 37 +++++++++++++++++++++++++---
>   1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/comedi/drivers/comedi_test.c b/drivers/comedi/drivers/comedi_test.c
> index 30ea8b53ebf8..7fefe0de0bcc 100644
> --- a/drivers/comedi/drivers/comedi_test.c
> +++ b/drivers/comedi/drivers/comedi_test.c
> @@ -87,6 +87,8 @@ struct waveform_private {
>   	struct comedi_device *dev;	/* parent comedi device */
>   	u64 ao_last_scan_time;		/* time of previous AO scan in usec */
>   	unsigned int ao_scan_period;	/* AO scan period in usec */
> +	bool ai_timer_enable:1;		/* should AI timer be running? */
> +	bool ao_timer_enable:1;		/* should AO timer be running? */
>   	unsigned short ao_loopbacks[N_CHANS];
>   };
>   
> @@ -232,12 +234,18 @@ static void waveform_ai_timer(struct timer_list *t)
>   	if (cmd->stop_src == TRIG_COUNT && async->scans_done >= cmd->stop_arg) {
>   		async->events |= COMEDI_CB_EOA;
>   	} else {
> +		unsigned long flags;
> +
>   		if (devpriv->ai_convert_time > now)
>   			time_increment = devpriv->ai_convert_time - now;
>   		else
>   			time_increment = 1;
> -		mod_timer(&devpriv->ai_timer,
> -			  jiffies + usecs_to_jiffies(time_increment));
> +		spin_lock_irqsave(&dev->spinlock, flags);
> +		if (devpriv->ai_timer_enable) {
> +			mod_timer(&devpriv->ai_timer,
> +				  jiffies + usecs_to_jiffies(time_increment));
> +		}
> +		spin_unlock_irqrestore(&dev->spinlock, flags);

Actually, I should have used a plain old spin_lock() in the above (and 
spin_lock_bh() elsewhere in the code) instead of spin_lock_irqsave(), 
since there are no hard interrupts involved.

I'll send a v2 patch shortly.

-- 
-=( Ian Abbott <abbotti@....co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-