| 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
| ||
|
Message-ID: <aGJnH0Ka2Y0koGAX@JPC00244420>
Date: Mon, 30 Jun 2025 19:29:51 +0900
From: Shashank Balaji <shashank.mahadasyam@...y.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org,
Alexandre Ghiti <alex@...ti.fr>, linux-riscv@...ts.infradead.org,
Sia Jee Heng <jeeheng.sia@...rfivetech.com>,
James Morse <james.morse@....com>,
Nicholas Piggin <npiggin@...il.com>,
linux-arm-kernel@...ts.infradead.org,
Rahul Bukte <rahul.bukte@...y.com>,
Daniel Palmer <daniel.palmer@...y.com>,
Shinya Takumi <shinya.takumi@...y.com>
Subject: Re: [RFC PATCH] kernel/cpu: in freeze_secondary_cpus() ensure
primary cpu is of domain type
Hi Peter,
On Mon, Jun 30, 2025 at 10:48:08AM +0200, Peter Zijlstra wrote:
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index a59e009e0be4..d9167b0559a5 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -1902,12 +1902,28 @@ int freeze_secondary_cpus(int primary)
> >
> > cpu_maps_update_begin();
> > if (primary == -1) {
> > - primary = cpumask_first(cpu_online_mask);
> > - if (!housekeeping_cpu(primary, HK_TYPE_TIMER))
> > - primary = housekeeping_any_cpu(HK_TYPE_TIMER);
> > + primary = cpumask_first_and_and(cpu_online_mask,
> > + housekeeping_cpumask(HK_TYPE_TIMER),
> > + housekeeping_cpumask(HK_TYPE_DOMAIN));
>
> That's terrible indenting, please align after the opening bracket like:
>
> primary = cpumask_first_and_and(cpu_online_mask,
> housekeeping_cpumask(HK_TYPE_TIMER),
> housekeeping_cpumask(HK_TYPE_DOMAIN));
>
> Also, IIRC HK_TYPE_HRTIMER is deprecated and should be something like
> HK_TYPE_NOISE or somesuch. Frederic?
> <snip>
> > + primary = cpumask_first_and(cpu_online_mask,
> > + housekeeping_cpumask(HK_TYPE_DOMAIN));
>
> Indenting again.
Sorry about the indentation. I've now set my editor to use 8-space tabs.
I'll have it fixed in the next version of the patch.
HK_TYPE_KERNEL_NOISE was added by commit 6010d245ddc9f463bbf0
(sched/isolation: Consolidate housekeeping cpumasks that are always identical).
I'll replace HK_TYPE_TIMER with HK_TYPE_KERNEL_NOISE in the next
version.
>
> > + if (primary >= nr_cpu_ids) {
> > + error = -ENODEV;
> > + pr_err("No suitable primary CPU found. Ensure at least one non-isolated CPU is online\n");
> > + goto abort;
> > + }
> > + } else if (!housekeeping_cpu(primary, HK_TYPE_DOMAIN)) {
> > + error = -ENODEV;
> > + pr_err("Primary CPU %d should not be isolated\n", primary);
> > + goto abort;
> > + }
> > }
> >
> > /*
> > @@ -1943,6 +1959,7 @@ int freeze_secondary_cpus(int primary)
> > else
> > pr_err("Non-boot CPUs are not disabled\n");
> >
> > +abort:
> > /*
> > * Make sure the CPUs won't be enabled by someone else. We need to do
> > * this even in case of failure as all freeze_secondary_cpus() users are
>
>
> Also; doesn't the above boil down to something like:
>
> if (primary == -1) {
> primary = cpumask_first_and_and(cpu_online_mask,
> housekeeping_cpumask(HK_TYPE_TIMER),
> housekeeping_cpumask(HK_TYPE_DOMAIN));
> } if (!cpu_online(primary)) {
> primary = cpumask_first_and(cpu_online_mask,
> housekeeping_cpumask(HK_TYPE_DOMAIN));
> }
>
> if (primary >= nr_cpu_ids || !housekeeping_cpu(primary, HK_TYPE_DOMAIN)) {
> error = -ENODEV;
> pr_err("Primary CPU %d should not be isolated\n", primary);
> goto abort;
> }
>
> Yes, this has less error string variation, but the code is simpler.
If primary >= nr_cpu_ids, primary should not be used in the error
string. But I do think it can be simplified as so, at the cost of the
error string not being completely accurate:
if (primary == -1) {
primary = cpumask_first_and_and(cpu_online_mask,
housekeeping_cpumask(HK_TYPE_TIMER),
housekeeping_cpumask(HK_TYPE_DOMAIN));
} else if (!cpu_online(primary)) {
primary = cpumask_first_and(cpu_online_mask,
housekeeping_cpumask(HK_TYPE_DOMAIN));
}
if (primary >= nr_cpu_ids) {
error = -ENODEV;
pr_err("No suitable primary CPU found. Ensure at least one non-isolated CPU is online\n");
goto abort;
} else if (!housekeeping_cpu(HK_TYPE_DOMAIN)) {
error = -ENODEV;
pr_err("Primary CPU %d should not be isolated\n", primary);
goto abort;
}
The "Ensure at lest one non-isolated CPU is online" is only partially
true in the case primary = -1 was passed and we couldn't find a suitable
cpu because we were looking for an online cpu that's non-isolated _and_
non-nohz_full. But the "non-nohz_full" condition isn't taken care of in
the !cpu_online(primary) branch. Although, I do think it should be
(question 2 below): in both the branches, we should look for a cpu that's online,
non-isolated, and of domain type. That would further simplify the above snippet to,
if (primary == -1 || !cpu_online(primary)) {
primary = cpumask_first_and_and(cpu_online_mask,
housekeeping_cpumask(HK_TYPE_TIMER),
housekeeping_cpumask(HK_TYPE_DOMAIN));
}
if (primary >= nr_cpu_ids) {
error = -ENODEV;
pr_err("No suitable primary CPU found. Ensure at least one non-isolated, non-nohz_full CPU is online\n");
goto abort;
} else if (!housekeeping_cpu(HK_TYPE_DOMAIN) || !housekeeping_cpu(HK_TYPE_TIMER)) {
error = -ENODEV;
pr_err("Primary CPU %d should not be isolated or nohz_full\n", primary);
goto abort;
}
Now the error string is accurate.
>2. This concerns the following snippet of freeze_secondary_cpus():
>
> if (primary == -1) {
> primary = cpumask_first(cpu_online_mask);
> if (!housekeeping_cpu(primary, HK_TYPE_TIMER))
> primary = housekeeping_any_cpu(HK_TYPE_TIMER);
> } else {
> if (!cpu_online(primary))
> primary = cpumask_first(cpu_online_mask);
> }
>
> suspend_disable_secondary_cpus() calls freeze_secondary_cpus() with primary = -1,
> if CONFIG_PM_SLEEP_SMP_NONZERO_CPU, and primary = 0 otherwise. On x86 and arm64,
> for example, it's called with primary = 0. In the primary != -1 branch, why
> isn't primary checked for HK_TYPE_TIMER as is done in the primary == 1 branch?
> On x86 this is fine, since it doesn't advertise ARCH_SUSPEND_NONZERO_CPU,
> cpu 0 will be removed out of the nohz_full mask even if it's specified. And it
> also cannot be offlined. So on x86, the primary cpu will always be online and
> non-nohz_full. But on arm64, for example, cpu 0 _can_ be offlined. So
> cpumask_first(cpu_online_mask) will find the next online cpu, which may not
> be non-nohz_full.
>
> Also, why the requirement for the primary cpu to be of HK_TYPE_TIMER?
The HK_TYPE_TIMER check was added by Nicholas in
commit 9ca12ac04bb7d7cfb28aa5 (kernel/cpu: Allow non-zero CPU to be primary
for suspend / kexec freeze) [1], but it was added only to the primary = -1 branch.
I'm not sure why we don't check for HK_TYPE_TIMER when primary is specified.
[1] https://lore.kernel.org/all/20190411033448.20842-4-npiggin@gmail.com/
Thanks,
Shashank
Powered by blists - more mailing lists