linux-kernel - Re: [PATCH] LoongArch: Do not create sysfs control file for io master CPUs

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]

Message-ID: <b4803250-e66c-0c0e-15c9-35d8fdfb09ea@loongson.cn>
Date:   Sat, 8 Oct 2022 18:44:31 +0800
From:   Tiezhu Yang <yangtiezhu@...ngson.cn>
To:     WANG Xuerui <kernel@...0n.name>,
        Huacai Chen <chenhuacai@...nel.org>
Cc:     loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] LoongArch: Do not create sysfs control file for io master
 CPUs



On 10/08/2022 05:51 PM, Tiezhu Yang wrote:
>
>
> On 10/08/2022 05:27 PM, WANG Xuerui wrote:
>> On 2022/10/8 16:59, Tiezhu Yang wrote:
>>> Now io master CPUs are not hotpluggable on LoongArch, in the current
>>> code,
>>> only /sys/devices/system/cpu/cpu0/online is not created, let us set the
>>> hotpluggable field of all the io master CPUs as 0, then prevent to
>>> create
>>> sysfs control file for the other io master CPUs which confuses some user
>>> space tools. This is similar with commit 9cce844abf07 ("MIPS: CPU#0 is
>>> not
>>> hotpluggable").
>>>
>>> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
>>> ---
>>>   arch/loongarch/kernel/smp.c      |  8 --------
>>>   arch/loongarch/kernel/topology.c | 12 +++++++++++-
>>>   2 files changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
>>> index b5fab30..ef89292 100644
>>> --- a/arch/loongarch/kernel/smp.c
>>> +++ b/arch/loongarch/kernel/smp.c
>>> @@ -240,19 +240,11 @@ void loongson3_smp_finish(void)
>>>     #ifdef CONFIG_HOTPLUG_CPU
>>>   -static bool io_master(int cpu)
>>> -{
>>> -    return test_bit(cpu, &loongson_sysconf.cores_io_master);
>>> -}
>>> -
>>>   int loongson3_cpu_disable(void)
>>>   {
>>>       unsigned long flags;
>>>       unsigned int cpu = smp_processor_id();
>>>   -    if (io_master(cpu))
>>> -        return -EBUSY;
>>> -
>>
>> Could this get invoked from somewhere other than the sysfs entries that
>> "confuse user-space tools", e.g. from somewhere else in kernel land? If
>> so (or if we can't rule out the possibility) keeping the guard here
>> might prove more prudent.
>>

takedown_cpu()  kernel/cpu.c
take_cpu_down()  kernel/cpu.c
__cpu_disable()  arch/loongarch/include/asm/smp.h
loongson3_cpu_disable()  arch/loongarch/kernel/smp.c

So I think you are right, keeping the guard here might prove more 
prudent, then it is better move io_master() to a header file that
can be used in smp.c and topology.c.

Let us wait for more review comments, thank you.

>
> If c->hotpluggable is 0, it will not generate a control file in sysfs
> for this CPU, for example:
>
> [root@...ux loongson]# cat /sys/devices/system/cpu/cpu0/online
> cat: /sys/devices/system/cpu/cpu0/online: No such file or directory
> [root@...ux loongson]# echo 0 > /sys/devices/system/cpu/cpu0/online
> bash: /sys/devices/system/cpu/cpu0/online: Permission denied
>
> So no need to check it here, just remove the code.
>
> This was done in commit cbab54d9c2b2 ("MIPS: No need to check CPU 0 in
> {loongson3,bmips,octeon}_cpu_disable()").
>
>>>   #ifdef CONFIG_NUMA
>>>       numa_remove_cpu(cpu);
>>>   #endif
>>> diff --git a/arch/loongarch/kernel/topology.c
>>> b/arch/loongarch/kernel/topology.c
>>> index ab1a75c..7e7a77f 100644
>>> --- a/arch/loongarch/kernel/topology.c
>>> +++ b/arch/loongarch/kernel/topology.c
>>> @@ -5,6 +5,7 @@
>>>   #include <linux/node.h>
>>>   #include <linux/nodemask.h>
>>>   #include <linux/percpu.h>
>>> +#include <asm/bootinfo.h>
>>>     static DEFINE_PER_CPU(struct cpu, cpu_devices);
>>>   @@ -33,6 +34,11 @@ void arch_unregister_cpu(int cpu)
>>>   EXPORT_SYMBOL(arch_unregister_cpu);
>>>   #endif
>>>   +static bool io_master(int cpu)
>>> +{
>>> +    return test_bit(cpu, &loongson_sysconf.cores_io_master);
>>> +}
>>> +
>>>   static int __init topology_init(void)
>>>   {
>>>       int i, ret;
>>> @@ -40,7 +46,11 @@ static int __init topology_init(void)
>>>       for_each_present_cpu(i) {
>>>           struct cpu *c = &per_cpu(cpu_devices, i);
>>>   -        c->hotpluggable = !!i;
>>> +        if (io_master(i))
>>> +            c->hotpluggable = 0;
>>> +        else
>>> +            c->hotpluggable = 1;
>>> +
>>
>> This is just "c->hotpluggable = !io_master(i);".
>>
>
> Yes, I am OK either way, if it is necessary to send v2,
> please let me know.
>
>>>           ret = register_cpu(c, i);
>>>           if (ret < 0)
>>>               pr_warn("topology_init: register_cpu %d failed (%d)\n",
>>> i, ret);
>> Other changes should be okay as they are in line with the previous MIPS
>> change you referenced, but let's see what Huacai thinks.
>>
>
> Thanks,
> Tiezhu