| 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: <Zg2qFinSkAOmRHcM@slm.duckdns.org>
Date: Wed, 3 Apr 2024 09:12:22 -1000
From: Tejun Heo <tj@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Leonardo Bras <leobras@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Lai Jiangshan <jiangshanlai@...il.com>,
linux-kernel@...r.kernel.org, Junyao Zhao <junzhao@...hat.com>,
Chris von Recklinghausen <crecklin@...hat.com>,
Frederic Weisbecker <frederic@...nel.org>
Subject: Re: [PATCH v2 1/1] wq: Avoid using isolated cpus' timers on
queue_delayed_work
(cc'ing Frederic and quoting whole body)
Hello, Oleg.
On Tue, Apr 02, 2024 at 12:58:47PM +0200, Oleg Nesterov wrote:
> This patch was applied as aae17ebb53cd3da but as Chris reports with this
> commit the kernel can crash at boot time because __queue_delayed_work()
> doesn't check that housekeeping_any_cpu() returns a valid cpu < nr_cpu_ids.
>
> Just boot the kernel with nohz_full=mask which includes the boot cpu, say
> nohz_full=0-6 on a machine with 8 CPUs. __queue_delayed_work() will use
> add_timer_on(timer, NR_CPUS /* returned by housekeeping_any_cpu */) until
> start_secondary() brings CPU 7 up.
>
> The problem is simple, but I do not know what should we do, I know nothing
> about CPU isolation.
>
> We can fix __queue_delayed_work(), this is simple, but other callers of
> housekeeping_any_cpu() seem to assume it must always return a valid CPU
> too. So perhaps we should change housekeeping_any_cpu()
Yeah, patching this up from wq side is easy but housekeeping_any_cpu()
always being able to pick a housekeeping CPU would be better.
> - return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
> + cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
> + if (cpu < nr_cpu_ids)
> + return cpu;
>
> ?
>
> But I'm afraid this can hide other problems. May be
>
> - return cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
> + cpu = cpumask_any_and(housekeeping.cpumasks[type], cpu_online_mask);
> + if (cpu < nr_cpu_ids)
> + return cpu;
> +
> + WARN_ON(system_state > SYSTEM_BOOTING);
>
> ?
>
> -------------------------------------------------------------------------------
> OTOH, Documentation/timers/no_hz.rst says
>
> Therefore, the
> boot CPU is prohibited from entering adaptive-ticks mode. Specifying a
> "nohz_full=" mask that includes the boot CPU will result in a boot-time
> error message, and the boot CPU will be removed from the mask.
>
> and this doesn't match the reality.
Don't some archs allow the boot CPU to go down too tho? If so, this doesn't
really solve the problem, right?
> So it seems that we should fix housekeeping_setup() ? see the patch below.
>
> In any case the usage of cpu_present_mask doesn't look right to me.
>
> Oleg.
>
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -129,7 +154,7 @@ static int __init housekeeping_setup(char *str, unsigned long flags)
> cpumask_andnot(housekeeping_staging,
> cpu_possible_mask, non_housekeeping_mask);
>
> - if (!cpumask_intersects(cpu_present_mask, housekeeping_staging)) {
> + if (!cpumask_test_cpu(smp_processor_id(), housekeeping_staging)) {
> __cpumask_set_cpu(smp_processor_id(), housekeeping_staging);
> __cpumask_clear_cpu(smp_processor_id(), non_housekeeping_mask);
> if (!housekeeping.flags) {
>
>
> On 01/29, Leonardo Bras wrote:
> >
> > When __queue_delayed_work() is called, it chooses a cpu for handling the
> > timer interrupt. As of today, it will pick either the cpu passed as
> > parameter or the last cpu used for this.
> >
> > This is not good if a system does use CPU isolation, because it can take
> > away some valuable cpu time to:
> > 1 - deal with the timer interrupt,
> > 2 - schedule-out the desired task,
> > 3 - queue work on a random workqueue, and
> > 4 - schedule the desired task back to the cpu.
> >
> > So to fix this, during __queue_delayed_work(), if cpu isolation is in
> > place, pick a random non-isolated cpu to handle the timer interrupt.
> >
> > As an optimization, if the current cpu is not isolated, use it instead
> > of looking for another candidate.
> >
> > Signed-off-by: Leonardo Bras <leobras@...hat.com>
> > ---
> > Changes since v1:
> > - Make sure the CPU is isolated for any value of "cpu"
> >
> > Changes since RFC:
> > - Do not use the same cpu from the timer for queueing the work.
> > - If the current cpu is not isolated, use it's timer instead of
> > looking for another candidate.
> >
> > kernel/workqueue.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 76e60faed8923..8dd7c01b326a4 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -1958,10 +1958,18 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
> > dwork->cpu = cpu;
> > timer->expires = jiffies + delay;
> >
> > - if (unlikely(cpu != WORK_CPU_UNBOUND))
> > + if (housekeeping_enabled(HK_TYPE_TIMER)) {
> > + /* If the current cpu is a housekeeping cpu, use it. */
> > + cpu = smp_processor_id();
> > + if (!housekeeping_test_cpu(cpu, HK_TYPE_TIMER))
> > + cpu = housekeeping_any_cpu(HK_TYPE_TIMER);
> > add_timer_on(timer, cpu);
> > - else
> > - add_timer(timer);
> > + } else {
> > + if (likely(cpu == WORK_CPU_UNBOUND))
> > + add_timer(timer);
> > + else
> > + add_timer_on(timer, cpu);
> > + }
> > }
> >
> > /**
> > --
> > 2.43.0
> >
>
--
tejun
Powered by blists - more mailing lists