| Message ID | 20250331163642.2382651-1-alexander.sverdlin@siemens.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | counter: interrupt-cnt: Protect enable/disable OPs with mutex | expand |
Hello Alexander, On 31.03.25 18:36, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > 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? Thanks, Ahmad > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > --- > drivers/counter/interrupt-cnt.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c > index 949598d51575a..d83848d0fe2af 100644 > --- a/drivers/counter/interrupt-cnt.c > +++ b/drivers/counter/interrupt-cnt.c > @@ -3,12 +3,14 @@ > * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de> > */ > > +#include <linux/cleanup.h> > #include <linux/counter.h> > #include <linux/gpio/consumer.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > #include <linux/mod_devicetable.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/platform_device.h> > #include <linux/types.h> > > @@ -19,6 +21,7 @@ struct interrupt_cnt_priv { > struct gpio_desc *gpio; > int irq; > bool enabled; > + struct mutex lock; > struct counter_signal signals; > struct counter_synapse synapses; > struct counter_count cnts; > @@ -41,6 +44,8 @@ static int interrupt_cnt_enable_read(struct counter_device *counter, > { > struct interrupt_cnt_priv *priv = counter_priv(counter); > > + guard(mutex)(&priv->lock); > + > *enable = priv->enabled; > > return 0; > @@ -51,6 +56,8 @@ static int interrupt_cnt_enable_write(struct counter_device *counter, > { > struct interrupt_cnt_priv *priv = counter_priv(counter); > > + guard(mutex)(&priv->lock); > + > if (priv->enabled == enable) > return 0; > > @@ -227,6 +234,8 @@ static int interrupt_cnt_probe(struct platform_device *pdev) > if (ret) > return ret; > > + mutex_init(&priv->lock); > + > ret = devm_counter_add(dev, counter); > if (ret < 0) > return dev_err_probe(dev, ret, "Failed to add counter\n");
Hi Ahmad, 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. 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@pengutronix.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
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@pengutronix.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 >
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c index 949598d51575a..d83848d0fe2af 100644 --- a/drivers/counter/interrupt-cnt.c +++ b/drivers/counter/interrupt-cnt.c @@ -3,12 +3,14 @@ * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de> */ +#include <linux/cleanup.h> #include <linux/counter.h> #include <linux/gpio/consumer.h> #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/mod_devicetable.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/platform_device.h> #include <linux/types.h> @@ -19,6 +21,7 @@ struct interrupt_cnt_priv { struct gpio_desc *gpio; int irq; bool enabled; + struct mutex lock; struct counter_signal signals; struct counter_synapse synapses; struct counter_count cnts; @@ -41,6 +44,8 @@ static int interrupt_cnt_enable_read(struct counter_device *counter, { struct interrupt_cnt_priv *priv = counter_priv(counter); + guard(mutex)(&priv->lock); + *enable = priv->enabled; return 0; @@ -51,6 +56,8 @@ static int interrupt_cnt_enable_write(struct counter_device *counter, { struct interrupt_cnt_priv *priv = counter_priv(counter); + guard(mutex)(&priv->lock); + if (priv->enabled == enable) return 0; @@ -227,6 +234,8 @@ static int interrupt_cnt_probe(struct platform_device *pdev) if (ret) return ret; + mutex_init(&priv->lock); + ret = devm_counter_add(dev, counter); if (ret < 0) return dev_err_probe(dev, ret, "Failed to add counter\n");