linux-kernel - Re: [PATCH -mm][resend] Disable CPU hotplug during suspend

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: <200607282115.45407.rjw@sisk.pl>
Date:	Fri, 28 Jul 2006 21:15:45 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Nathan Lynch <ntl@...ox.com>
Cc:	Andrew Morton <akpm@...l.org>, LKML <linux-kernel@...r.kernel.org>,
	Pavel Machek <pavel@....cz>
Subject: Re: [PATCH -mm][resend] Disable CPU hotplug during suspend

Hi Nathan,

On Friday 28 July 2006 20:20, Nathan Lynch wrote:
> Hi Rafael-
> 
> A couple of minor comments:
> 
> 
> > +int cpu_down(unsigned int cpu)
> > +{
> > +	int err = 0;
> > +
> > +	mutex_lock(&cpu_add_remove_lock);
> > +	if (cpu_hotplug_disabled)
> > +		err = -EPERM;
> > +	else
> > +		err = __cpu_down(cpu);
> > +
> >  	mutex_unlock(&cpu_add_remove_lock);
> >  	return err;
> >  }
> > @@ -191,6 +203,11 @@ int __devinit cpu_up(unsigned int cpu)
> >  	void *hcpu = (void *)(long)cpu;
> >  
> >  	mutex_lock(&cpu_add_remove_lock);
> > +	if (cpu_hotplug_disabled) {
> > +		ret = -EPERM;
> > +		goto out;
> > +	}
> 
> I think -EBUSY would be more appropriate than -EPERM, perhaps?

Sure, why not.
 
> > +#ifdef CONFIG_SUSPEND_SMP
> > +static cpumask_t frozen_cpus;
> > +
> > +int disable_nonboot_cpus(void)
> > +{
> > +	int cpu, error = 0;
> > +
> > +	/* We take all of the non-boot CPUs down in one shot to avoid races
> > +	 * with the userspace trying to use the CPU hotplug at the same time
> > +	 */
> > +	mutex_lock(&cpu_add_remove_lock);
> > +	cpus_clear(frozen_cpus);
> > +	printk("Disabling non-boot CPUs ...\n");
> > +	for_each_online_cpu(cpu) {
> > +		if (cpu == 0)
> > +			continue;
> 
> Assuming cpu 0 is online is not okay in generic code.

Absolutely.  Thanks for pointing this out.

> This should be something like:
> 
> 	int cpu, first_cpu, error = 0;
> 
> 	/* We take all of the non-boot CPUs down in one shot to avoid races
> 	 * with the userspace trying to use the CPU hotplug at the same time
> 	 */
> 	mutex_lock(&cpu_add_remove_lock);
> 	cpus_clear(frozen_cpus);
> 	first_cpu = first_cpu(cpu_online_mask);
> 	printk("Disabling non-boot CPUs ...\n");
> 	for_each_online_cpu(cpu) {
> 		if (cpu == first_cpu)
> 			continue;

I'm not quite sure if we can finish with CPU0 offline.  Perhaps it's better to
check if CPU0 is online and bring it up if not and then continue or return
an error if that fails?

Rafael
-
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/