Hi,
I found three patches from two topics in my outbox.
* 0001-lib-percpu_counter-percpu_counter_add_batch-overflow.patch
* 0002-include-linux-percpu_counter.h-Race-in-uniprocessor-.patch
The percpu_counter code does take interrupt into account properly,
this could result in corrupted counters.
0003-kernel-irq-manage.c-disable_irq-might-sleep.patch
The disable_irq() documentation does not take threaded interrupt
handlers into account.
I've not added cc stable, the races are not really likely:
- #002 is probably the most likely case: UP systems that use
percpu_counters from interrupt should observe corruptions.
- #001 is fairly theoretical
- #003 is a docu update and thus out of scope for stable.
@Andrew: Could you add them to -mm and -next?
Especially #003 should be in -next for a few months, to check what the
might_sleep() encounters.
--
Manfred
If an interrupt happens between __this_cpu_read(*fbc->counters) and
this_cpu_add(*fbc->counters, amount), and that interrupt modifies
the per_cpu_counter, then the this_cpu_add() after the interrupt
returns may under/overflow.
Signed-off-by: Manfred Spraul <[email protected]>
Cc: "Sun, Jiebin" <[email protected]>
---
lib/percpu_counter.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 42f729c8e56c..dba56c5c1837 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -73,28 +73,33 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
EXPORT_SYMBOL(percpu_counter_set);
/*
- * This function is both preempt and irq safe. The former is due to explicit
- * preemption disable. The latter is guaranteed by the fact that the slow path
- * is explicitly protected by an irq-safe spinlock whereas the fast patch uses
- * this_cpu_add which is irq-safe by definition. Hence there is no need muck
- * with irq state before calling this one
+ * local_irq_save() is needed to make the function irq safe:
+ * - The slow path would be ok as protected by an irq-safe spinlock.
+ * - this_cpu_add would be ok as it is irq-safe by definition.
+ * But:
+ * The decision slow path/fast path and the actual update must be atomic, too.
+ * Otherwise a call in process context could check the current values and
+ * decide that the fast path can be used. If now an interrupt occurs before
+ * the this_cpu_add(), and the interrupt updates this_cpu(*fbc->counters),
+ * then the this_cpu_add() that is executed after the interrupt has completed
+ * can produce values larger than "batch" or even overflows.
*/
void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
{
s64 count;
+ unsigned long flags;
- preempt_disable();
+ local_irq_save(flags);
count = __this_cpu_read(*fbc->counters) + amount;
if (abs(count) >= batch) {
- unsigned long flags;
- raw_spin_lock_irqsave(&fbc->lock, flags);
+ raw_spin_lock(&fbc->lock);
fbc->count += count;
__this_cpu_sub(*fbc->counters, count - amount);
- raw_spin_unlock_irqrestore(&fbc->lock, flags);
+ raw_spin_unlock(&fbc->lock);
} else {
this_cpu_add(*fbc->counters, amount);
}
- preempt_enable();
+ local_irq_restore(flags);
}
EXPORT_SYMBOL(percpu_counter_add_batch);
--
2.38.1
With the introduction of threaded interrupt handlers, it is virtually
never safe to call disable_irq() from non-premptible context.
Thus: Update the documentation, add a might_sleep() to catch any
offenders.
Fixes: 3aa551c9b4c4 ("genirq: add threaded interrupt handler support")
Signed-off-by: Manfred Spraul <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: "Sverdlin, Alexander" <[email protected]>
---
kernel/irq/manage.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 5b7cf28df290..8ce75495e04f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -723,10 +723,13 @@ EXPORT_SYMBOL(disable_irq_nosync);
* to complete before returning. If you use this function while
* holding a resource the IRQ handler may need you will deadlock.
*
- * This function may be called - with care - from IRQ context.
+ * Can only be called from preemptible code as it might sleep when
+ * an interrupt thread is associated to @irq.
+ *
*/
void disable_irq(unsigned int irq)
{
+ might_sleep();
if (!__disable_irq_nosync(irq))
synchronize_irq(irq);
}
--
2.38.1
Hi Manfred!
On Fri, 2022-12-16 at 16:04 +0100, Manfred Spraul wrote:
> With the introduction of threaded interrupt handlers, it is virtually
> never safe to call disable_irq() from non-premptible context.
>
> Thus: Update the documentation, add a might_sleep() to catch any
> offenders.
>
> Fixes: 3aa551c9b4c4 ("genirq: add threaded interrupt handler
> support")
>
> Signed-off-by: Manfred Spraul <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: "Sverdlin, Alexander" <[email protected]>
> ---
> kernel/irq/manage.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 5b7cf28df290..8ce75495e04f 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -723,10 +723,13 @@ EXPORT_SYMBOL(disable_irq_nosync);
> * to complete before returning. If you use this function while
> * holding a resource the IRQ handler may need you will
> deadlock.
> *
> - * This function may be called - with care - from IRQ context.
> + * Can only be called from preemptible code as it might sleep
> when
> + * an interrupt thread is associated to @irq.
> + *
> */
> void disable_irq(unsigned int irq)
> {
> + might_sleep();
I'm not sure about this, latest wait_event() inside synchronize_irq()
has it already.
> if (!__disable_irq_nosync(irq))
> synchronize_irq(irq);
> }
--
Alexander Sverdlin
Siemens AG
http://www.siemens.com
Hello Manfred,
On Fri, 2022-12-23 at 11:54 +0100, Manfred Spraul wrote:
> > I'm not sure about this, latest wait_event() inside
> > synchronize_irq()
> > has it already.
> >
> > > if (!__disable_irq_nosync(irq))
> > > synchronize_irq(irq);
> > > }
>
> That is the whole point: might_sleep() should be always called. We
> are
> clarifying an API definition. Everyone who uses disable_irq() from
> non-sleeping context should get a warning, 100% of the time.
>
> Not just within synchronize_irq() if there is an active threaded irq
> handler.
As I read it, it will warn almost always, even without threaded handler
configured, only in some error cases it will not warn. I'm just
thinking that disable_irq() might be in a hot path and this is being
checked anyway two calls deeper. But I don't have a strong opinion on
that and it looks like it has been taken into mm tree already.
--
Alexander Sverdlin
Siemens AG
http://www.siemens.com
Hi Alexander,
On 12/20/22 07:58, Sverdlin, Alexander wrote:
> Hi Manfred!
>
> On Fri, 2022-12-16 at 16:04 +0100, Manfred Spraul wrote:
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 5b7cf28df290..8ce75495e04f 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -723,10 +723,13 @@ EXPORT_SYMBOL(disable_irq_nosync);
>> * to complete before returning. If you use this function while
>> * holding a resource the IRQ handler may need you will
>> deadlock.
>> *
>> - * This function may be called - with care - from IRQ context.
>> + * Can only be called from preemptible code as it might sleep
>> when
>> + * an interrupt thread is associated to @irq.
>> + *
>> */
>> void disable_irq(unsigned int irq)
>> {
>> + might_sleep();
> I'm not sure about this, latest wait_event() inside synchronize_irq()
> has it already.
>
>> if (!__disable_irq_nosync(irq))
>> synchronize_irq(irq);
>> }
That is the whole point: might_sleep() should be always called. We are
clarifying an API definition. Everyone who uses disable_irq() from
non-sleeping context should get a warning, 100% of the time.
Not just within synchronize_irq() if there is an active threaded irq
handler.
--
Manfred
On Fri, 23 Dec 2022 11:22:30 +0000 "Sverdlin, Alexander" <[email protected]> wrote:
> But I don't have a strong opinion on
> that and it looks like it has been taken into mm tree already.
It can be taken out again ;) Hence the "mm-unstable" name.
The following commit has been merged into the irq/core branch of tip:
Commit-ID: 17549b0f184d870f2cfa4e5cfa79f4c4905ed757
Gitweb: https://git.kernel.org/tip/17549b0f184d870f2cfa4e5cfa79f4c4905ed757
Author: Manfred Spraul <[email protected]>
AuthorDate: Fri, 16 Dec 2022 16:04:41 +01:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 11 Jan 2023 19:35:13 +01:00
genirq: Add might_sleep() to disable_irq()
With the introduction of threaded interrupt handlers, it is virtually
never safe to call disable_irq() from non-premptible context.
Thus: Update the documentation, add an explicit might_sleep() to catch any
offenders. This is more obvious and straight forward than the implicit
might_sleep() check deeper down in the disable_irq() call chain.
Fixes: 3aa551c9b4c4 ("genirq: add threaded interrupt handler support")
Signed-off-by: Manfred Spraul <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
kernel/irq/manage.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 5b7cf28..8ce7549 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -723,10 +723,13 @@ EXPORT_SYMBOL(disable_irq_nosync);
* to complete before returning. If you use this function while
* holding a resource the IRQ handler may need you will deadlock.
*
- * This function may be called - with care - from IRQ context.
+ * Can only be called from preemptible code as it might sleep when
+ * an interrupt thread is associated to @irq.
+ *
*/
void disable_irq(unsigned int irq)
{
+ might_sleep();
if (!__disable_irq_nosync(irq))
synchronize_irq(irq);
}