linux-kernel - Re: [PATCH v2] padata: validate cpumask without removed CPU during offline

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: <b341a330-0e85-e386-1f74-89219c4f511f@oracle.com>
Date:   Thu, 22 Aug 2019 18:53:06 -0400
From:   Daniel Jordan <daniel.m.jordan@...cle.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] padata: validate cpumask without removed CPU during
 offline



On 8/22/19 6:10 PM, Daniel Jordan wrote:
> On 8/21/19 11:50 PM, Herbert Xu wrote:
>> On Fri, Aug 09, 2019 at 05:06:03PM -0400, Daniel Jordan wrote:
>>> diff --git a/kernel/padata.c b/kernel/padata.c
>>> index d056276a96ce..01460ea1d160 100644
>>> --- a/kernel/padata.c
>>> +++ b/kernel/padata.c
>>> @@ -702,10 +702,7 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
>>>       struct parallel_data *pd = NULL;
>>>       if (cpumask_test_cpu(cpu, cpu_online_mask)) {
>>> -
>>> -        if (!padata_validate_cpumask(pinst, pinst->cpumask.pcpu) ||
>>> -            !padata_validate_cpumask(pinst, pinst->cpumask.cbcpu))
>>> -            __padata_stop(pinst);
>>> +        __padata_stop(pinst);
>>>           pd = padata_alloc_pd(pinst, pinst->cpumask.pcpu,
>>>                        pinst->cpumask.cbcpu);
>>> @@ -716,6 +713,9 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
>>>           cpumask_clear_cpu(cpu, pd->cpumask.cbcpu);
>>>           cpumask_clear_cpu(cpu, pd->cpumask.pcpu);
>>> +        if (padata_validate_cpumask(pinst, pd->cpumask.pcpu) &&
>>> +            padata_validate_cpumask(pinst, pd->cpumask.cbcpu))
>>> +            __padata_start(pinst);
>>>       }
>>
>> I looked back at the original code and in fact the original
>> assumption is to call this after cpu_online_mask has been modified.
>>
>> So I suspect we need to change the state at which this is called
>> by CPU hotplug.
> 
> Yes the state idea is good, it's cleaner to have the CPU out of the online mask ahead of time.
> 
> I think we'll need two states.  We want a CPU being offlined to already be removed from the online cpumask so and'ing the user-supplied and online masks reflects conditions after the hotplug operation is finished.  For the same reason we want a CPU being onlined to already be in the online mask, and we can use the existing hotplug state for that, though we'd need a new padata-specific state for the offline case.

The new state would be something before CPUHP_BRINGUP_CPU so the cpu isn't in the online mask yet.

> 
>> IOW the commit that broke this is 30e92153b4e6.
> 
> I don't think 30e92153b4e6 is the one since the commit before that only allows __padata_remove_cpu to do its work if @cpu is in the online mask, so the call happens before cpu_online_mask has been modified.  Same story for the very first padata commit, so it seems like that should actually be Fixes.
> 
>> This would also allow us to get rid of the two cpumask_clear_cpu
>> calls on pd->cpumask which is just bogus as you should only ever
>> modify the pd->cpumask prior to the padata_repalce call (because
>> the readers are not serialised with respect to this).
> 
> Yeah, makes sense.
> 
> Daniel