[PATCH] Avoid indefinite wait in smp_call_function_many() if cpumask is modified

[PATCH] Avoid indefinite wait in smp_call_function_many() if cpumask is modified

[Catalin Marinas]

On ARM processors (and not only) with software broadcasting of the TLB
maintenance operations, smp_call_function_many() is given a
mm_cpumask(mm) as argument. This cpumask may be modified (bits cleared)
during the smp_call_function_many() execution as a result of other
events like ASID roll-over.

smp_call_function_many() checks the mask for CPUs to call but there is a
small window between the last check and the mask copying to
data->cpumask when the given mask may be modified. If the mask is reset
to the current CPU only, csd_lock_wait() at the end of the function
would wait indefinitely. Similar scenario could happen if a CPU goes
offline in this window.

This patch adds an additional check for data->refs in
smp_call_function_many() to avoid waiting indefinitely if there are no
CPUs to call.

Reported-by: Saeed Bishara <saeed.bishara@gmail.com>
Tested-by: Saeed Bishara <saeed.bishara@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---

An alternative to this patch would be to find the
smp_call_function_many() calling sites and do a cpumask_copy() so that
the mask passed is guaranteed to remain unmodified. But I prefer the
current patch as it is much simpler.

 kernel/smp.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 9910744..a79454f 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -499,6 +499,10 @@ void smp_call_function_many(const struct cpumask *mask,
 	smp_wmb();
 
 	atomic_set(&data->refs, cpumask_weight(data->cpumask));
+	if (unlikely(!atomic_read(&data->refs))) {
+		csd_unlock(&data->csd);
+		return;
+	}
 
 	raw_spin_lock_irqsave(&call_function.lock, flags);
 	/*

Re: [PATCH] Avoid indefinite wait in smp_call_function_many() if cpumask is modified

[Russell King - ARM Linux]

On Tue, Mar 15, 2011 at 06:20:56PM +0000, Catalin Marinas wrote:
> On ARM processors (and not only) with software broadcasting of the TLB
> maintenance operations, smp_call_function_many() is given a
> mm_cpumask(mm) as argument. This cpumask may be modified (bits cleared)
> during the smp_call_function_many() execution as a result of other
> events like ASID roll-over.

We shouldn't be modifying a mask which has been passed into one of these
functions.  Having masks change beneath a function which is actively
using it and can wait is an obvious recipe for problems and races.

Re: [PATCH] Avoid indefinite wait in smp_call_function_many() if cpumask is modified

[Catalin Marinas]

On Tuesday, 15 March 2011, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Mar 15, 2011 at 06:20:56PM +0000, Catalin Marinas wrote:
>> On ARM processors (and not only) with software broadcasting of the TLB
>> maintenance operations, smp_call_function_many() is given a
>> mm_cpumask(mm) as argument. This cpumask may be modified (bits cleared)
>> during the smp_call_function_many() execution as a result of other
>> events like ASID roll-over.
>
> We shouldn't be modifying a mask which has been passed into one of these
> functions.  Having masks change beneath a function which is actively
> using it and can wait is an obvious recipe for problems and races.

It's not that bad since the mask is copied inside the function.
Anyway, it looks like such patch has been around for more than a
month, so I'm not pushing it anymore.

Catalin

-- 
Catalin