linux-kernel - Re: [PATCH printk v2 1/7] printk: Move buffer size defines

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]

Date:   Thu, 24 Nov 2022 15:42:53 +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 v2 1/7] printk: Move buffer size defines

On Thu 2022-11-24 13:44:29, John Ogness wrote:
> On 2022-11-24, Petr Mladek <pmladek@...e.com> wrote:
> >> Move the buffer size defines to console.h in preparation of adding a
> >> buffer structure. The new buffer structure will be embedded within
> >> struct console. Therefore console.h was chosen as the new home for
> >> these defines.
> >
> > The buffers are not embedded into struct console in this patchset.
> > Are they going to be added directly or via pointer, please?
> 
> By "embedded" I mean added directly. The buffers need to be available
> immediately and cannot be allocated or assigned dynamically. The console
> struct is generally defined by drivers with:
> 
> static struct console my_console = {
>    ...
> };
> 
> I could think of no way to statically define the buffers but keep their
> sizes hidden.
> 
> > IMHO, it is always better to hide these implementation details
> > in an internal header or source file. It will be possible
> > if struct console contained on a pointer to the buffers.
> 
> The problem is not pointers, it is static definition (without knowing
> the size of the thing that is statically defined). The new thread/atomic
> consoles run in parallel, so they cannot share the single static buffer
> like we do now.

Let me to play a devil advocate first:

Well, allocation is possible long before scheduling is
possible. It is actually available even before early parameters
are proceed where the boot consoles are registered. At least
it is used when setup_log_buf(1) is called in setup_arch() on x86.

The motivation is that only thread/atomic consoles would need
the console-specific buffer. The other consoles might share
the global one.

It would be useful even for atomic consoles. IMHO, most users
use generic kernels that support a variety of hardware. They
would provide static buffers for many console drivers but
only one or two would be used in the end.

Also the atomic consoles would need these buffers for each context.
It might be even more useful to allocate them dynamically.

Or do I miss something, please?

That said:

I do not have any numbers at hands to show how this
is important. Also I do not know if the early allocations
have some limits.

The static buffers might be acceptable for simplicity and
reliability after all.

Feel free to keep them static. I would ack the next version
of this patch after making the CONSOLE_EXT_LOG_MAX dependent
on CONFIG_PRINTK.

Best Regards,
Petr