linux-kernel - Re: [PATCH v2 01/14] smp: Create a new function to shutdown nonboot 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]

Date:   Tue, 21 Jan 2020 17:47:52 +0000
From:   Qais Yousef <qais.yousef@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Jiri Kosina <jkosina@...e.cz>,
        Nicholas Piggin <npiggin@...il.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Ingo Molnar <mingo@...nel.org>,
        Eiichi Tsukata <devel@...ukata.com>,
        Zhenzhong Duan <zhenzhong.duan@...cle.com>,
        Nadav Amit <namit@...are.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-ia64@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 01/14] smp: Create a new function to shutdown nonboot
 cpus

On 01/21/20 17:03, Russell King - ARM Linux admin wrote:
> On Mon, Nov 25, 2019 at 11:27:41AM +0000, Qais Yousef wrote:
> > +void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
> > +{
> > +	unsigned int cpu;
> > +
> > +	if (!cpu_online(primary_cpu)) {
> > +		pr_info("Attempting to shutdodwn nonboot cpus while boot cpu is offline!\n");
> > +		cpu_online(primary_cpu);

Eh, that should be cpu_up(primary_cpu)!

Which I have to say I'm not if is the right thing to do.
migrate_to_reboot_cpu() picks the first online cpu if reboot_cpu (assumed 0) is
offline

migrate_to_reboot_cpu():
 225         /* Make certain the cpu I'm about to reboot on is online */
 226         if (!cpu_online(cpu))
 227                 cpu = cpumask_first(cpu_online_mask);

> > +	}
> > +
> > +	for_each_present_cpu(cpu) {
> > +		if (cpu == primary_cpu)
> > +			continue;
> > +		if (cpu_online(cpu))
> > +			cpu_down(cpu);
> > +	}
> 
> How does this avoid racing with userspace attempting to restart CPUs
> that have already been taken down by this function?

This is meant to be called from machine_shutdown() only.

But you've got a point.

The previous logic that used disable_nonboot_cpus(), which in turn called
freeze_secondary_cpus() didn't hold hotplug lock. So I assumed the higher level
logic of machine_shutdown() ensures that hotplug lock is held to synchronize
with potential other hotplug operations.

But I can see now that it doesn't.

With this series that migrates users to use device_{online,offline}, holding
the lock_device_hotplug() should protect against such races.

Worth noting that this an existing problem in the code and not something
I introduced, of course it makes sense to fix it properly as part of this
series.

I'm not sure how the other archs deal with this TBH.

Thanks for having a look!

Cheers

--
Qais Yousef