linux-kernel - Re: [PATCH v4 2/2] clocksource: Use migrate_disable() to avoid calling get_random

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] [day] [month] [year] [list]

Message-ID: <a747efb7-2b1a-4570-b379-d8a3bce606a7@paulmck-laptop>
Date: Fri, 31 Jan 2025 10:52:32 -0800
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Waiman Long <longman@...hat.com>
Cc: John Stultz <jstultz@...gle.com>, Thomas Gleixner <tglx@...utronix.de>,
	Stephen Boyd <sboyd@...nel.org>, Feng Tang <feng.tang@...el.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Clark Williams <clrkwllms@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>, linux-kernel@...r.kernel.org,
	linux-rt-devel@...ts.linux.dev
Subject: Re: [PATCH v4 2/2] clocksource: Use migrate_disable() to avoid
 calling get_random_u32() in atomic context

On Fri, Jan 31, 2025 at 12:33:23PM -0500, Waiman Long wrote:
> The following bug report happened with a PREEMPT_RT kernel.
> 
> [   30.957705] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
> [   30.957711] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 2012, name: kwatchdog
> [   30.962673] preempt_count: 1, expected: 0
> [   30.962676] RCU nest depth: 0, expected: 0
> 
> It is due to the fact that clocksource_verify_choose_cpus() is invoked
> with preemption disabled.  This function invokes get_random_u32()
> to obtain random numbers for choosing CPUs.  The batched_entropy_32
> local lock and/or the base_crng.lock spinlock in driver/char/random.c
> will be acquired during the call. In PREEMPT_RT kernel, they are both
> sleeping locks and so cannot be acquired in atomic context.
> 
> Fix this problem by using migrate_disable() to allow
> smp_processor_id() to be reliably used without introducing atomic
> context. The preempt_disable() function is then called after
> clocksource_verify_choose_cpus() but before the clocksource measurement
> is being run to avoid introducing unexpected latency.
> 
> Fixes: 7560c02bdffb ("clocksource: Check per-CPU clock synchronization when marked unstable")
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Signed-off-by: Waiman Long <longman@...hat.com>

Reviewed-by: Paul E. McKenney <paulmck@...nel.org>

> ---
>  kernel/time/clocksource.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index 77d9566d3aa6..2a7802ec480c 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -373,10 +373,10 @@ void clocksource_verify_percpu(struct clocksource *cs)
>  	cpumask_clear(&cpus_ahead);
>  	cpumask_clear(&cpus_behind);
>  	cpus_read_lock();
> -	preempt_disable();
> +	migrate_disable();
>  	clocksource_verify_choose_cpus();
>  	if (cpumask_empty(&cpus_chosen)) {
> -		preempt_enable();
> +		migrate_enable();
>  		cpus_read_unlock();
>  		pr_warn("Not enough CPUs to check clocksource '%s'.\n", cs->name);
>  		return;
> @@ -384,6 +384,7 @@ void clocksource_verify_percpu(struct clocksource *cs)
>  	testcpu = smp_processor_id();
>  	pr_info("Checking clocksource %s synchronization from CPU %d to CPUs %*pbl.\n",
>  		cs->name, testcpu, cpumask_pr_args(&cpus_chosen));
> +	preempt_disable();
>  	for_each_cpu(cpu, &cpus_chosen) {
>  		if (cpu == testcpu)
>  			continue;
> @@ -403,6 +404,7 @@ void clocksource_verify_percpu(struct clocksource *cs)
>  			cs_nsec_min = cs_nsec;
>  	}
>  	preempt_enable();
> +	migrate_enable();
>  	cpus_read_unlock();
>  	if (!cpumask_empty(&cpus_ahead))
>  		pr_warn("        CPUs %*pbl ahead of CPU %d for clocksource %s.\n",
> -- 
> 2.48.1
>