Re: [PATCH 2/2] lib/vsprintf: reduce space taken by no_hash_pointers warning

[Marco Elver]

On Mon, 8 Mar 2021 at 11:16, Petr Mladek <pmladek@xxxxxxxx> wrote:
> On Fri 2021-03-05 20:42:06, Marco Elver wrote:
> > Move the no_hash_pointers warning string into __initconst section, so
> > that it is discarded after init. Remove common start/end characters.
> > Also remove repeated lines from the array, since the compiler can't
> > remove duplicate strings for us since the array must appear in
> > __initconst as defined.
> >
> > Note, a similar message appears in kernel/trace/trace.c, but compiling
> > the feature is guarded by CONFIG_TRACING. It is not immediately obvious
> > if a space-concious kernel would prefer CONFIG_TRACING=n. Therefore, it
> > makes sense to keep the message for no_hash_pointers as __initconst, and
> > not move the NOTICE-printing to a common function.
> >
> > Link: https://lkml.kernel.org/r/CAMuHMdULKZCJevVJcp7TxzLdWLjsQPhE8hqxhnztNi9bjT_cEw@xxxxxxxxxxxxxx
> > Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> > ---
> >  lib/vsprintf.c | 30 +++++++++++++++++-------------
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 4a14889ccb35..1095689c9c97 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -2094,26 +2094,30 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
> >  bool no_hash_pointers __ro_after_init;
> >  EXPORT_SYMBOL_GPL(no_hash_pointers);
> >
> > +static const char no_hash_pointers_warning[8][55] __initconst = {
> > +     "******************************************************",
> > +     "   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   ",
> > +     " This system shows unhashed kernel memory addresses   ",
> > +     " via the console, logs, and other interfaces. This    ",
> > +     " might reduce the security of your system.            ",
> > +     " If you see this message and you are not debugging    ",
> > +     " the kernel, report this immediately to your system   ",
> > +     " administrator!                                       ",
> > +};
> > +
> >  static int __init no_hash_pointers_enable(char *str)
> >  {
> > +     /* Indices into no_hash_pointers_warning; -1 is an empty line. */
> > +     const int lines[] = { 0, 1, -1, 2, 3, 4, -1, 5, 6, 7, -1, 1, 0 };
> > +     int i;
> > +
> >       if (no_hash_pointers)
> >               return 0;
> >
> >       no_hash_pointers = true;
> >
> > -     pr_warn("**********************************************************\n");
> > -     pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> > -     pr_warn("**                                                      **\n");
> > -     pr_warn("** This system shows unhashed kernel memory addresses   **\n");
> > -     pr_warn("** via the console, logs, and other interfaces. This    **\n");
> > -     pr_warn("** might reduce the security of your system.            **\n");
> > -     pr_warn("**                                                      **\n");
> > -     pr_warn("** If you see this message and you are not debugging    **\n");
> > -     pr_warn("** the kernel, report this immediately to your system   **\n");
> > -     pr_warn("** administrator!                                       **\n");
> > -     pr_warn("**                                                      **\n");
> > -     pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> > -     pr_warn("**********************************************************\n");
> > +     for (i = 0; i < ARRAY_SIZE(lines); i++)
> > +             pr_warn("**%54s**\n", i == -1 ? "" : no_hash_pointers_warning[lines[i]]);
>
> Is this worth it, please? Could anyone provide some numbers how
> the kernel size increases between releases?

I just posted this patch as an option to solve the problem as I had
posted in the other thread, but otherwise agree that it's questionable
if it's worth it. (I hope that Geert will post the numbers for the
arch+config he noticed that led to his report.)

> The number of code lines is basically just growing. The same is true
> for the amount of printed messages.
>
> This patch is saving some lines of text that might be effectively
> compressed. But it adds some code and array with indexes. Does it
> make any significant imrovement in the compressed kernel image?
>
> Geert was primary concerned about the runtime memory consuption.
> It will be solved by the  __initconst. The rest affects only
> the size of the compressed image on disk.

If we do __initconst change we need to manually remove the duplicate
lines because we're asking the compiler to create a large array (and
there's no more auto-dedup). If we do not remove the duplicate lines,
the __initconst-only approach would create a larger image and result
in subtly increased memory consumption during init. The additional
code together with manual dedup should offset that. (I can split this
patch as Andy suggests, but first need confirmation what people
actually want.)

I have no idea what the right trade-off is, and appeal to Geert to
suggest what would be acceptable to him.

Thanks,
-- Marco