linux-kernel - Re: [PATCH v2] workqueue: Fix edge cases for calc of pool's cpumask

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Jun 2017 09:34:05 -0500
From:   Michael Bringmann <mwb@...ux.vnet.ibm.com>
To:     Tejun Heo <tj@...nel.org>
Cc:     Lai Jiangshan <jiangshanlai@...il.com>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        Michael Bringmann from Kernel Team <mbringm@...ibm.com>,
        Nathan Fontenot <nfont@...ux.vnet.ibm.com>
Subject: Re: [PATCH v2] workqueue: Fix edge cases for calc of pool's cpumask



On 06/06/2017 09:20 AM, Tejun Heo wrote:
> Hello, Michael.
> 
> It would have been better to continue debugging in the prev thread.
> This still seems incorrect for the same reason as before.
> 
> On Tue, Jun 06, 2017 at 09:09:40AM -0500, Michael Bringmann wrote:
>> On NUMA systems with dynamic processors, the content of the cpumask
>> may change over time.  As new processors are added via DLPAR operations,
>> workqueues are created for them.  Depending upon the order in which CPUs
>> are added/removed, we may run into problems with the content of the
>> cpumask used by the workqueues.  This patch deals with situations where
>> the online cpumask for a node is a proper superset of possible cpumask
>> for the node.  It also deals with edge cases where the order in which
>> CPUs are removed/added from the online cpumask may leave the set for a
>> node empty, and require execution by CPUs on another node.
>>
>> In these and other cases, the patch attempts to ensure that a valid,
>> usable cpumask is used to set up newly created pools for workqueues.
>>
>> Signed-off-by: Tejun Heo <tj@...nel.org> & Michael Bringmann <mwb@...ux.vnet.ibm.com>
> 
> Heh, you can't add sob's for other people.  For partial attributions,
> you can just note in the description.

Sorry for the error.
> 
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index c74bf39..460de61 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -3366,6 +3366,9 @@ static struct worker_pool *get_unbound_pool(const struct workqueue_attrs *attrs)
>>  	copy_workqueue_attrs(pool->attrs, attrs);
>>  	pool->node = target_node;
>>  
>> +	if (!cpumask_weight(pool->attrs->cpumask))
>> +		cpumask_copy(pool->attrs->cpumask, cpumask_of(smp_processor_id()));
> 
> So, this is still wrong.

It only catches if something has gone wrong before.  The alternative in this case
would be,

	BUG(!cpumask_weight(pool->attrs->cpumask));

> 
>>  	/*
>>  	 * no_numa isn't a worker_pool attribute, always clear it.  See
>>  	 * 'struct workqueue_attrs' comments for detail.
>> @@ -3559,13 +3562,13 @@ static struct pool_workqueue *alloc_unbound_pwq(struct workqueue_struct *wq,
>>   * stable.
>>   *
>>   * Return: %true if the resulting @cpumask is different from @attrs->cpumask,
>> - * %false if equal.
>> + * %false if equal.  On %false return, the content of @cpumask is undefined.
>>   */
>>  static bool wq_calc_node_cpumask(const struct workqueue_attrs *attrs, int node,
>>  				 int cpu_going_down, cpumask_t *cpumask)
>>  {
>>  	if (!wq_numa_enabled || attrs->no_numa)
>> -		goto use_dfl;
>> +		return false;
>>  
>>  	/* does @node have any online CPUs @attrs wants? */
>>  	cpumask_and(cpumask, cpumask_of_node(node), attrs->cpumask);
>> @@ -3573,15 +3576,13 @@ static bool wq_calc_node_cpumask(const struct workqueue_attrs *attrs, int node,
>>  		cpumask_clear_cpu(cpu_going_down, cpumask);
>>  
>>  	if (cpumask_empty(cpumask))
>> -		goto use_dfl;
>> +		return false;
>>  
>>  	/* yeap, return possible CPUs in @node that @attrs wants */
>>  	cpumask_and(cpumask, attrs->cpumask, wq_numa_possible_cpumask[node]);
>> -	return !cpumask_equal(cpumask, attrs->cpumask);
>>  
>> -use_dfl:
>> -	cpumask_copy(cpumask, attrs->cpumask);
>> -	return false;
>> +	return !cpumask_empty(cpumask) &&
>> +		!cpumask_equal(cpumask, attrs->cpumask);
> 
> And this part doesn't really change that.
> 
> CPUs going offline or online shouldn't change their relation to
> wq_numa_possible_cpumask.  I wonder whether the arch code is changing
> CPU id <-> NUMA node mapping on CPU on/offlining.  x86 used to do that
> too and got recently modified.  Can you see whether that's the case?

The but that I see does not appear to be related to changing of CPU/Node mapping
-- they are not changing their place when going offline/online.  Rather new CPUs
are being hot-added to the system (i.e. they were not present at boot), and the
node to which they are being added had no CPUs at boot.

> 
> Thanks.
> 

Thanks.

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@...ux.vnet.ibm.com