Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with mutex

[Ahmad Fatoum]

Hello Alexander,

On 4/1/25 10:38, Sverdlin, Alexander wrote:
> On Tue, 2025-04-01 at 06:50 +0200, Ahmad Fatoum wrote:
>>> Enable/disable seems to be racy on SMP, consider the following scenario:
>>>
>>> CPU0					CPU1
>>>
>>> interrupt_cnt_enable_write(true)
>>> {
>>>  	if (priv->enabled == enable)
>>>  		return 0;
>>>
>>>  	if (enable) {
>>>  		priv->enabled = true;
>>>  					interrupt_cnt_enable_write(false)
>>>  					{
>>>  						if (priv->enabled == enable)
>>>  							return 0;
>>>
>>>  						if (enable) {
>>>  							priv->enabled = true;
>>>  							enable_irq(priv->irq);
>>>  						} else {
>>>  							disable_irq(priv->irq)
>>>  							priv->enabled = false;
>>>  						}
>>>  		enable_irq(priv->irq);
>>>  	} else {
>>>  		disable_irq(priv->irq);
>>>  		priv->enabled = false;
>>>  	}
>>>
>>> The above would result in priv->enabled == false, but IRQ left enabled.
>>> Protect both write (above race) and read (to propagate the value on SMP)
>>> callbacks with a mutex.
>>
>> Doesn't sysfs/kernfs already ensure that the ops may not be called concurrently
>> on the same open file?
> 
> as I understand the code, the operations on the same FD will be serialized, i.e.
> if you open an entry and access it concurrently within the same process.

Thanks for checking! I agree now that we indeed need synchronization here.

Cheers,
Ahmad

> 
> If you apply the following patch on top of the proposed patch:
> 
> --- a/drivers/counter/interrupt-cnt.c
> +++ b/drivers/counter/interrupt-cnt.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@xxxxxxxxxxxxxx>
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/cleanup.h>
>  #include <linux/counter.h>
>  #include <linux/gpio/consumer.h>
> @@ -56,6 +57,9 @@ static int interrupt_cnt_enable_write(struct counter_device *counter,
>  {
>  	struct interrupt_cnt_priv *priv = counter_priv(counter);
>  
> +	WARN_ON(!mutex_trylock(&priv->lock));
> +	mutex_unlock(&priv->lock);
> +
>  	guard(mutex)(&priv->lock);
>  
>  	if (priv->enabled == enable)
> @@ -69,6 +73,8 @@ static int interrupt_cnt_enable_write(struct counter_device *counter,
>  		priv->enabled = false;
>  	}
>  
> +	msleep(1000);
> +
>  	return 0;
>  }
>  
> 
> And would run `while true; do echo 0 > /sys/.../enable; echo 1 > /sys/.../enable; done &`
> twice, you'd see the following quickly:
> 
> WARNING: CPU: 1 PID: 754 at /drivers/counter/interrupt-cnt.c:60 interrupt_cnt_enable_write+0xa0/0xb0 [interrupt_cnt]
> CPU: 1 UID: 0 PID: 754 Comm: sh
> pc : interrupt_cnt_enable_write+0xa0/0xb0 [interrupt_cnt]
> lr : interrupt_cnt_enable_write+0x34/0xb0 [interrupt_cnt]
> Call trace:
>  interrupt_cnt_enable_write+0xa0/0xb0 [interrupt_cnt] (P)
>  counter_comp_u8_store+0xcc/0x118 [counter]
>  dev_attr_store+0x20/0x40
>  sysfs_kf_write+0x84/0xa8
>  kernfs_fop_write_iter+0x128/0x1e0
>  vfs_write+0x248/0x388
>  ksys_write+0x78/0x118
>  __arm64_sys_write+0x24/0x38
>  invoke_syscall+0x50/0x120
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |