linux-kernel - Re: [PATCH printk v1 03/13] printk: use percpu flag instead of cpu

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: <YiI2x6K5IhsADEmK@alley>
Date:   Fri, 4 Mar 2022 16:56:55 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v1 03/13] printk: use percpu flag instead of
 cpu_online()

On Wed 2022-03-02 15:27:50, John Ogness wrote:
> I have taken some time to investigate the percpu implementation so that
> I could provide clear answers here.
> 
> On 2022-02-15, Petr Mladek <pmladek@...e.com> wrote:
> > I am not 100% sure. But it seems that static per-CPU variables might
> > actually be used since the boot.
> 
> You are correct, but until per-cpu areas are setup, they all point to
> CPU0. Normally that is not a problem since usually code is using
> this_cpu_ptr() (which will always be the CPU0 in early boot), rather
> than specifying foreign CPUs with per_cpu_ptr().
> 
> > Most likely, only dynamically allocated per-cpu variables have to wait
> > until the per-cpu areas are initialized.
> 
> It is also important to wait if data will be stored that is no longer
> valid after per-cpu areas are setup. setup_per_cpu_areas() copies the
> static CPU0 per-cpu value to all the newly setup per-cpu areas.
> 
> This is actually the cause for the mystery [0] of failing irq_work when
> printk_deferred was added with commit
> 15341b1dd409749fa5625e4b632013b6ba81609b ("char/random: silence a
> lockdep splat with printk()".

Just for record, the right commit ID in the mainline is
1b710b1b10eff9d466. It used printk_deferred() in _warn_unseeded_randomness():

--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1687,8 +1687,9 @@ static void _warn_unseeded_randomness(const char *func_name, void *caller,
 	print_once = true;
 #endif
 	if (__ratelimit(&unseeded_warning))
-		pr_notice("random: %s called from %pS with crng_init=%d\n",
-			  func_name, caller, crng_init);
+		printk_deferred(KERN_NOTICE "random: %s called from %pS "
+				"with crng_init=%d\n", func_name, caller,
+				crng_init);
 }
 
 /*



> By avoiding queueing irq_work before setup_per_cpu_areas(), we correctly
> avoided this problem. (I considered sending a patch so that
> irq_work_claim() will fail if a global @percpu_complete is not yet
> set.

That is a great TODO :-)

> But for now, our set_percpu_data_ready() solution is at least good
> enough for the printk subsystem.)

Yes but it is most likely not needed for CON_ANYTIME consoles,
see below.

> > We should probably revisit the code and remove the fallback to
> > normal static variables.
> 
> Definitely. Now it is clear that @printk_count and @printk_count_nmi do
> not need early variants.

Note the ordering:

asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
{
[...]
	early_security_init();
[...]
	setup_per_cpu_areas();
[...]
	parse_early_param();
	after_dashes = parse_args("Booting kernel",


My guess is that _warn_unseeded_randomness() was called from a code
before early_security_init(). It was bad because per-CPU variables
were not ready yet.

The early consoles are enabled by parse_early_param(). It happens
after setup_per_cpu_areas(). It means that all console drivers
should be on the safe side.

Best Regards,
Petr