linux-kernel - Re: [PATCH 3/4 v3] smp_call_function_many: handle concurrent clearing of mask

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <4D807A430200007800036BDF@vpn.id2.novell.com>
Date:	Wed, 16 Mar 2011 07:52:19 +0000
From:	"Jan Beulich" <JBeulich@...ell.com>
To:	"Milton Miller" <miltonm@....com>
Cc:	<xiaoguangrong@...fujitsu.com>, <mingo@...e.hu>,
	<jaxboe@...ionio.com>, <npiggin@...il.com>,
	"Mike Galbraith" <efault@....de>,
	"Peter Zijlstra" <peterz@...radead.org>,
	"Tony Luck" <tony.luck@...el.com>, <benh@...nel.crashing.org>,
	<akpm@...ux-foundation.org>, <torvalds@...ux-foundation.org>,
	<paulmck@...ux.vnet.ibm.com>, <rusty@...tcorp.com.au>,
	"Anton Blanchard" <anton@...ba.org>,
	"Dimitri Sivanich" <sivanich@....com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4 v3] smp_call_function_many: handle concurrent
	 clearing of mask

>>> On 15.03.11 at 20:27, Milton Miller <miltonm@....com> wrote:
> Mike Galbraith reported finding a lockup ("perma-spin bug") where the
> cpumask passed to smp_call_function_many was cleared by other cpu(s)
> while a cpu was preparing its call_data block, resulting in no cpu to
> clear the last ref and unlock the block.
> 
> Having cpus clear their bit asynchronously could be useful on a mask of
> cpus that might have a translation context, or cpus that need a push to
> complete an rcu window.
> 
> Instead of adding a BUG_ON and requiring yet another cpumask copy, just
> detect the race and handle it.
> 
> Note: arch_send_call_function_ipi_mask must still handle an empty
> cpumask because the data block is globally visible before the that
> arch callback is made.  And (obviously) there are no guarantees to
> which cpus are notified if the mask is changed during the call;
> only cpus that were online and had their mask bit set during the
> whole call are guaranteed to be called.
> 
> Reported-by: Mike Galbraith <efault@....de>
> Reported-by: Jan Beulich <JBeulich@...ell.com>
> Signed-off-by: Milton Miller <miltonm@....com>

Acked-by: Jan Beulich <jbeulich@...ell.com>

> ---
> v3: try to clarify which mask in comment
> v2: rediff for v2 of call_function_many: fix list delete vs add race
> 
> The arch code not expecting the race to empty the mask is the cause
> of https://bugzilla.kernel.org/show_bug.cgi?id=23042 that Andrew pointed
> out.
> 
> 
> Index: common/kernel/smp.c
> ===================================================================
> --- common.orig/kernel/smp.c	2011-03-15 05:22:26.000000000 -0500
> +++ common/kernel/smp.c	2011-03-15 06:22:26.000000000 -0500
> @@ -450,7 +450,7 @@ void smp_call_function_many(const struct
>  {
>  	struct call_function_data *data;
>  	unsigned long flags;
> -	int cpu, next_cpu, this_cpu = smp_processor_id();
> +	int refs, cpu, next_cpu, this_cpu = smp_processor_id();
>  
>  	/*
>  	 * Can deadlock when called with interrupts disabled.
> @@ -461,7 +461,7 @@ void smp_call_function_many(const struct
>  	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
>  		     && !oops_in_progress && !early_boot_irqs_disabled);
>  
> -	/* So, what's a CPU they want? Ignoring this one. */
> +	/* Try to fastpath.  So, what's a CPU they want? Ignoring this one. */
>  	cpu = cpumask_first_and(mask, cpu_online_mask);
>  	if (cpu == this_cpu)
>  		cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
> @@ -519,6 +519,13 @@ void smp_call_function_many(const struct
>  	/* We rely on the "and" being processed before the store */
>  	cpumask_and(data->cpumask, mask, cpu_online_mask);
>  	cpumask_clear_cpu(this_cpu, data->cpumask);
> +	refs = cpumask_weight(data->cpumask);
> +
> +	/* Some callers race with other cpus changing the passed mask */
> +	if (unlikely(!refs)) {
> +		csd_unlock(&data->csd);
> +		return;
> +	}
>  
>  	raw_spin_lock_irqsave(&call_function.lock, flags);
>  	/*
> @@ -532,7 +539,7 @@ void smp_call_function_many(const struct
>  	 * to the cpumask before this write to refs, which indicates
>  	 * data is on the list and is ready to be processed.
>  	 */
> -	atomic_set(&data->refs, cpumask_weight(data->cpumask));
> +	atomic_set(&data->refs, refs);
>  	raw_spin_unlock_irqrestore(&call_function.lock, flags);
>  
>  	/*



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/