[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <f1079411-0188-4b0b-9cc9-7e0c0130d69a@pengutronix.de>
Date: Tue, 1 Apr 2025 10:42:43 +0200
From: Ahmad Fatoum <a.fatoum@...gutronix.de>
To: "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Cc: "o.rempel@...gutronix.de" <o.rempel@...gutronix.de>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"wbg@...nel.org" <wbg@...nel.org>
Subject: Re: [PATCH] counter: interrupt-cnt: Protect enable/disable OPs with
mutex
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@...gutronix.de>
> */
>
> +#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 |
Powered by blists - more mailing lists