| 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: <CAKfTPtAB1cv_MxOveenxa5Y5zjsNC-RqinSr9Z2vrC7S92NOUw@mail.gmail.com>
Date: Mon, 27 Jan 2025 14:13:47 +0100
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Yafang Shao <laoar.shao@...il.com>
Cc: dan.carpenter@...aro.org, seanjc@...gle.com, mkoutny@...e.com,
peterz@...radead.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched: Don't define sched_clock_irqtime as static key
On Sun, 26 Jan 2025 at 07:50, Yafang Shao <laoar.shao@...il.com> wrote:
>
> The sched_clock_irqtime was defined as a static key in commit 8722903cbb8f
> ('sched: Define sched_clock_irqtime as static key'). However, this change
> introduces a 'sleeping in atomic context' warning, as shown below:
>
> arch/x86/kernel/tsc.c:1214 mark_tsc_unstable()
> warn: sleeping in atomic context
>
> As analyzed by Dan, the affected code path is as follows:
>
> vcpu_load() <- disables preempt
> -> kvm_arch_vcpu_load()
> -> mark_tsc_unstable() <- sleeps
>
> virt/kvm/kvm_main.c
> 166 void vcpu_load(struct kvm_vcpu *vcpu)
> 167 {
> 168 int cpu = get_cpu();
> ^^^^^^^^^^
> This get_cpu() disables preemption.
>
> 169
> 170 __this_cpu_write(kvm_running_vcpu, vcpu);
> 171 preempt_notifier_register(&vcpu->preempt_notifier);
> 172 kvm_arch_vcpu_load(vcpu, cpu);
> 173 put_cpu();
> 174 }
>
> arch/x86/kvm/x86.c
> 4979 if (unlikely(vcpu->cpu != cpu) || kvm_check_tsc_unstable()) {
> 4980 s64 tsc_delta = !vcpu->arch.last_host_tsc ? 0 :
> 4981 rdtsc() - vcpu->arch.last_host_tsc;
> 4982 if (tsc_delta < 0)
> 4983 mark_tsc_unstable("KVM discovered backwards TSC");
>
> arch/x86/kernel/tsc.c
> 1206 void mark_tsc_unstable(char *reason)
> 1207 {
> 1208 if (tsc_unstable)
> 1209 return;
> 1210
> 1211 tsc_unstable = 1;
> 1212 if (using_native_sched_clock())
> 1213 clear_sched_clock_stable();
> --> 1214 disable_sched_clock_irqtime();
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> kernel/jump_label.c
> 245 void static_key_disable(struct static_key *key)
> 246 {
> 247 cpus_read_lock();
> ^^^^^^^^^^^^^^^^
> This lock has a might_sleep() in it which triggers the static checker
> warning.
>
> 248 static_key_disable_cpuslocked(key);
> 249 cpus_read_unlock();
> 250 }
>
> Let revert this change for now as {disable,enable}_sched_clock_irqtime
> are used in many places, as pointed out by Sean, including the following:
>
> The code path in clocksource_watchdog():
>
> clocksource_watchdog()
> |
> -> spin_lock(&watchdog_lock);
> |
> -> __clocksource_unstable()
> |
> -> clocksource.mark_unstable() == tsc_cs_mark_unstable()
> |
> -> disable_sched_clock_irqtime()
>
> And the code path in sched_clock_register():
>
> /* Cannot register a sched_clock with interrupts on */
> local_irq_save(flags);
>
> ...
>
> /* Enable IRQ time accounting if we have a fast enough sched_clock() */
> if (irqtime > 0 || (irqtime == -1 && rate >= 1000000))
> enable_sched_clock_irqtime();
>
> local_irq_restore(flags);
>
> Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> Closes: https://lore.kernel.org/kvm/37a79ba3-9ce0-479c-a5b0-2bd75d573ed3@stanley.mountain/
> Debugged-by: Dan Carpenter <dan.carpenter@...aro.org>
> Debugged-by: Sean Christopherson <seanjc@...gle.com>
> Debugged-by: Michal Koutný <mkoutny@...e.com>
> Fixes: 8722903cbb8f ("sched: Define sched_clock_irqtime as static key")
> Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> Cc: Dan Carpenter <dan.carpenter@...aro.org>
> Cc: Sean Christopherson <seanjc@...gle.com>
> Cc: Michal Koutný <mkoutny@...e.com>
> Cc: Peter Zijlstra" <peterz@...radead.org>
> Cc: Vincent Guittot" <vincent.guittot@...aro.org>
I overlooked that this was used under atomic context
Reviewed-by: Vincent Guittot <vincent.guittot@...aro.org>
> ---
> kernel/sched/cputime.c | 8 ++++----
> kernel/sched/sched.h | 4 ++--
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 5d9143dd0879..c7904ce2345a 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -9,8 +9,6 @@
>
> #ifdef CONFIG_IRQ_TIME_ACCOUNTING
>
> -DEFINE_STATIC_KEY_FALSE(sched_clock_irqtime);
> -
> /*
> * There are no locks covering percpu hardirq/softirq time.
> * They are only modified in vtime_account, on corresponding CPU
> @@ -24,14 +22,16 @@ DEFINE_STATIC_KEY_FALSE(sched_clock_irqtime);
> */
> DEFINE_PER_CPU(struct irqtime, cpu_irqtime);
>
> +static int sched_clock_irqtime;
> +
> void enable_sched_clock_irqtime(void)
> {
> - static_branch_enable(&sched_clock_irqtime);
> + sched_clock_irqtime = 1;
> }
>
> void disable_sched_clock_irqtime(void)
> {
> - static_branch_disable(&sched_clock_irqtime);
> + sched_clock_irqtime = 0;
> }
>
> static void irqtime_account_delta(struct irqtime *irqtime, u64 delta,
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 38e0e323dda2..ab16d3d0e51c 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3259,11 +3259,11 @@ struct irqtime {
> };
>
> DECLARE_PER_CPU(struct irqtime, cpu_irqtime);
> -DECLARE_STATIC_KEY_FALSE(sched_clock_irqtime);
> +extern int sched_clock_irqtime;
>
> static inline int irqtime_enabled(void)
> {
> - return static_branch_likely(&sched_clock_irqtime);
> + return sched_clock_irqtime;
> }
>
> /*
> --
> 2.43.5
>
Powered by blists - more mailing lists