[PATCH 1/2] lib/vsprintf: do not show no_hash_pointers message multiple times

[PATCH 1/2] lib/vsprintf: do not show no_hash_pointers message multiple times

[Marco Elver]

Do not show no_hash_pointers message multiple times if the option was
passed more than once (e.g. via generated command line).

Signed-off-by: Marco Elver <elver@google.com>
---
 lib/vsprintf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 41ddc353ebb8..4a14889ccb35 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2096,6 +2096,9 @@ EXPORT_SYMBOL_GPL(no_hash_pointers);
 
 static int __init no_hash_pointers_enable(char *str)
 {
+	if (no_hash_pointers)
+		return 0;
+
 	no_hash_pointers = true;
 
 	pr_warn("**********************************************************\n");

base-commit: fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8
-- 
2.30.1.766.gb4fecdf3b7-goog

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

[Marco Elver]

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@mail.gmail.com
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Marco Elver <elver@google.com>
---
 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]]);
 
 	return 0;
 }
-- 
2.30.1.766.gb4fecdf3b7-goog

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

[Timur Tabi]

On Fri, Mar 5, 2021 at 1:46 PM Marco Elver <elver@google.com> wrote:
> +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 };

You can save a few more bytes by making this an array of s8.

I agree with the __initconst.  The rest seems overkill to me, but I
won't reject it.

Acked-by: Timur Tabi <timur@kernel.org>

Re: [PATCH 1/2] lib/vsprintf: do not show no_hash_pointers message multiple times

[Petr Mladek]

On Fri 2021-03-05 20:42:05, Marco Elver wrote:
> Do not show no_hash_pointers message multiple times if the option was
> passed more than once (e.g. via generated command line).
>
> Signed-off-by: Marco Elver <elver@google.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

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

[Petr Mladek]

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@mail.gmail.com
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
>  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?

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.

Best Regards,
Petr

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

[Andy Shevchenko]

On Fri, Mar 05, 2021 at 08:42:06PM +0100, 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.

This seems to have 2-in-1 patch. Care to split?
Feel free to add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
to the __initconst part, but the rest.


> Link: https://lkml.kernel.org/r/CAMuHMdULKZCJevVJcp7TxzLdWLjsQPhE8hqxhnztNi9bjT_cEw@mail.gmail.com
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
>  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]]);
>  
>  	return 0;
>  }
> -- 
> 2.30.1.766.gb4fecdf3b7-goog
> 

-- 
With Best Regards,
Andy Shevchenko

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@suse.com> 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@mail.gmail.com
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> >  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

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

[Geert Uytterhoeven]

Hi Petr,

On Mon, Mar 8, 2021 at 11:16 AM Petr Mladek <pmladek@suse.com> 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@mail.gmail.com
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> >  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

Yeah, the code indeed starts to look a bit cumbersome...

> the kernel size increases between releases?

I'd say 20 KiB per release, on average.

> The number of code lines is basically just growing. The same is true
> for the amount of printed messages.

Yeah, we keep on adding more messages.
But do we really need to print a message of 13 lines?
If you consider this critical for security, perhaps it should use pr_crit(),
or pr_alert()? But please don't print more than a single line.

<sarcastic>
Perhaps it should print a URL to a message instead, like the
"software license" option in Android systems and apps?
</sarcastic>

> 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.

I'm actually concerned about both.  Platforms (and boot loaders) may
have limitations for kernel image size, too.
Static memory consumption is also more easily measured, so I tend
to run bloat-o-meter, and dive into anything that adds more than 1 KiB.
And yes, this message is a low-hanging fruit...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Petr Mladek]

On Mon 2021-03-08 13:22:40, Geert Uytterhoeven wrote:
> Hi Petr,
> 
> On Mon, Mar 8, 2021 at 11:16 AM Petr Mladek <pmladek@suse.com> 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@mail.gmail.com
> > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > ---
> > >  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
> 
> Yeah, the code indeed starts to look a bit cumbersome...
> 
> > the kernel size increases between releases?
> 
> I'd say 20 KiB per release, on average.
> 
> > The number of code lines is basically just growing. The same is true
> > for the amount of printed messages.
> 
> Yeah, we keep on adding more messages.
> But do we really need to print a message of 13 lines?
> If you consider this critical for security, perhaps it should use pr_crit(),
> or pr_alert()? But please don't print more than a single line.
> 
> <sarcastic>
> Perhaps it should print a URL to a message instead, like the
> "software license" option in Android systems and apps?
> </sarcastic>
> 
> > 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.
> 
> I'm actually concerned about both.  Platforms (and boot loaders) may
> have limitations for kernel image size, too.
> Static memory consumption is also more easily measured, so I tend
> to run bloat-o-meter, and dive into anything that adds more than 1 KiB.
> And yes, this message is a low-hanging fruit...

OK, I wondered how big trick does the  __initconst on its own.

1. I compiled kernel without this patchset:

$# ll /boot/vmlinux-5.12.0-rc2-default+.bz2
-rwxr-xr-x 1 root root 18911364 Mar  8 15:58 /boot/vmlinux-5.12.0-rc2-default+.bz2

2. With this patchset:

$# ll /boot/vmlinux-5.12.0-rc2-default+.bz2
-rwxr-xr-x 1 root root 18910767 Mar  8 16:16 /boot/vmlinux-5.12.0-rc2-default+.bz2
$# echo $((18910767 - 18911364))
-597

3. With the patch below:

$# ll /boot/vmlinux-5.12.0-rc2-default+.bz2
-rwxr-xr-x 1 root root 18910906 Mar  8 16:58 /boot/vmlinux-5.12.0-rc2-default+.bz2
$# echo $((18910906 - 18911364))
-458

This patchset saves 139B more than a simple array.


Well, I am a bit confused. I have tried to keep the strings as a
static variable outside the function:

static const char *no_hash_pointers_warning[] __initconst = {
	...

and I got the following build error:

  CC      lib/vsprintf.o
lib/vsprintf.c:2097:20: error: no_hash_pointers_warning causes a section type conflict with __setup_str_no_hash_pointers_enable
 static const char *no_hash_pointers_warning[] __initconst = {
                    ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./include/linux/printk.h:6:0,
                 from ./include/linux/kernel.h:16,
                 from ./include/linux/clk.h:13,
                 from lib/vsprintf.c:22:
./include/linux/init.h:315:20: note: ‘__setup_str_no_hash_pointers_enable’ was declared here
  static const char __setup_str_##unique_id[] __initconst  \
                    ^
./include/linux/init.h:330:2: note: in expansion of macro ‘__setup_param’
  __setup_param(str, fn, fn, 1)
  ^~~~~~~~~~~~~
lib/vsprintf.c:2127:1: note: in expansion of macro ‘early_param’
 early_param("no_hash_pointers", no_hash_pointers_enable);
 ^~~~~~~~~~~


I solved this be defining the array inside the function that is marked
__init. But I am not sure if it is the correct solution. And I wonder
why the original patch did not have this problem.

Also I am curious why the array reduced the size of the binary so
significantly in compare with the const strings used as pr_warn()
arguments. It might depend on the compression method or???


Anyway, here is the patch that works for me and reduced the size of
the binary considerably:

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 4a14889ccb35..af01edae0d86 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2096,24 +2096,30 @@ EXPORT_SYMBOL_GPL(no_hash_pointers);
 
 static int __init no_hash_pointers_enable(char *str)
 {
+	int i;
+	const char *no_hash_pointers_warning[] = {
+		"**********************************************************",
+		"**   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!                                       **",
+		"**                                                      **",
+		"**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **",
+		"**********************************************************",
+	};
+
 	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(no_hash_pointers_warning); i++)
+		pr_warn("%s\n", no_hash_pointers_warning[i]);
 
 	return 0;
 }


Honestly, I do not want to spend much more time on this. I made the
test out of curiosity.

Feel free to provide the patch using the array, ideally with some
numbers how it helps. But please _avoid_ the indirection via

    const int lines[] = { 0, 1, -1, 2, 3, 4, -1, 5, 6, 7, -1, 1, 0 };

and also _avoid_ all the hardcoded constants, like:

      no_hash_pointers_warning[8][55]

and

      pr_warn("**%54s**\n"

They are error prone and hard to maintain. Such tricks are not
worth it from my POV.

Best Regards,
Petr

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

[Marco Elver]

On Mon, 8 Mar 2021 at 18:23, Petr Mladek <pmladek@suse.com> wrote:
[...]
> > I'm actually concerned about both.  Platforms (and boot loaders) may
> > have limitations for kernel image size, too.
> > Static memory consumption is also more easily measured, so I tend
> > to run bloat-o-meter, and dive into anything that adds more than 1 KiB.
> > And yes, this message is a low-hanging fruit...
>
> OK, I wondered how big trick does the  __initconst on its own.
>
> 1. I compiled kernel without this patchset:
>
> $# ll /boot/vmlinux-5.12.0-rc2-default+.bz2
> -rwxr-xr-x 1 root root 18911364 Mar  8 15:58 /boot/vmlinux-5.12.0-rc2-default+.bz2
>
> 2. With this patchset:
>
> $# ll /boot/vmlinux-5.12.0-rc2-default+.bz2
> -rwxr-xr-x 1 root root 18910767 Mar  8 16:16 /boot/vmlinux-5.12.0-rc2-default+.bz2
> $# echo $((18910767 - 18911364))
> -597
>
> 3. With the patch below:
>
> $# ll /boot/vmlinux-5.12.0-rc2-default+.bz2
> -rwxr-xr-x 1 root root 18910906 Mar  8 16:58 /boot/vmlinux-5.12.0-rc2-default+.bz2
> $# echo $((18910906 - 18911364))
> -458
>
> This patchset saves 139B more than a simple array.
>
>
> Well, I am a bit confused. I have tried to keep the strings as a
> static variable outside the function:
>
> static const char *no_hash_pointers_warning[] __initconst = {
>         ...
>
> and I got the following build error:
>
>   CC      lib/vsprintf.o
> lib/vsprintf.c:2097:20: error: no_hash_pointers_warning causes a section type conflict with __setup_str_no_hash_pointers_enable
>  static const char *no_hash_pointers_warning[] __initconst = {

This does not place the strings themselves into the initconst section,
but only the array of pointers to them. So, with 13 lines, we're
merely saving 13*sizeof(char*) after init, which does not resolve
Geert's problem of runtime overhead.

To dealloc the string text itself (remove the section), each line must
be placed into a 'char[N] __initconst' (or 'char [M][N] __initconst'
if we split the lines).

>                     ^~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ./include/linux/printk.h:6:0,
>                  from ./include/linux/kernel.h:16,
>                  from ./include/linux/clk.h:13,
>                  from lib/vsprintf.c:22:
> ./include/linux/init.h:315:20: note: ‘__setup_str_no_hash_pointers_enable’ was declared here
>   static const char __setup_str_##unique_id[] __initconst  \
>                     ^
> ./include/linux/init.h:330:2: note: in expansion of macro ‘__setup_param’
>   __setup_param(str, fn, fn, 1)
>   ^~~~~~~~~~~~~
> lib/vsprintf.c:2127:1: note: in expansion of macro ‘early_param’
>  early_param("no_hash_pointers", no_hash_pointers_enable);
>  ^~~~~~~~~~~
>
>
> I solved this be defining the array inside the function that is marked
> __init. But I am not sure if it is the correct solution. And I wonder
> why the original patch did not have this problem.
>
> Also I am curious why the array reduced the size of the binary so
> significantly in compare with the const strings used as pr_warn()
> arguments. It might depend on the compression method or???
>
>
> Anyway, here is the patch that works for me and reduced the size of
> the binary considerably:
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 4a14889ccb35..af01edae0d86 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2096,24 +2096,30 @@ EXPORT_SYMBOL_GPL(no_hash_pointers);
>
>  static int __init no_hash_pointers_enable(char *str)
>  {
> +       int i;
> +       const char *no_hash_pointers_warning[] = {
> +               "**********************************************************",
> +               "**   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!                                       **",
> +               "**                                                      **",
> +               "**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **",
> +               "**********************************************************",
> +       };
> +

This has no __initconst optimization (no runtime savings), and the
compiler places these strings into the data section and the above
array is just an array of pointers to them.

>         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");

While we're here: This paragraph can be shortened by saying what
kernel/trace/trace.c says ("..., report this immediately to your
vendor!") which avoids the "administrator! <lots of wasted spaces>".

> -       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(no_hash_pointers_warning); i++)
> +               pr_warn("%s\n", no_hash_pointers_warning[i]);

My guess is that the savings came from repeated calls to pr_warn() and
reduction in code-size and compression working better.

>         return 0;
>  }
>
>
> Honestly, I do not want to spend much more time on this. I made the
> test out of curiosity.
>
> Feel free to provide the patch using the array, ideally with some
> numbers how it helps. But please _avoid_ the indirection via
>
>     const int lines[] = { 0, 1, -1, 2, 3, 4, -1, 5, 6, 7, -1, 1, 0 };

We can probably do without this, but we'll have duplicated lines
stored in the initconst section.

> and also _avoid_ all the hardcoded constants, like:
>
>       no_hash_pointers_warning[8][55]

We'll need this if we want __initconst. But perhaps we do not have to
split it by lines, so we can get away with a char[].

> and
>
>       pr_warn("**%54s**\n"
>
> They are error prone and hard to maintain. Such tricks are not
> worth it from my POV.

I can send the version with a single 'static char[] __initconst', but
that version doesn't dedup lines and requires more init memory. I
don't know if we can make everybody happy here, we have to sacrifice
something: readability or space.

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

[Andy Shevchenko]

On Mon, Mar 08, 2021 at 07:23:34PM +0100, Marco Elver wrote:
> On Mon, 8 Mar 2021 at 18:23, Petr Mladek <pmladek@suse.com> wrote:

> > -       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");
> 
> While we're here: This paragraph can be shortened by saying what
> kernel/trace/trace.c says ("..., report this immediately to your
> vendor!") which avoids the "administrator! <lots of wasted spaces>".

Aren't we discussed that and the point was that kernel configuration option is
in administrator's realm?

> > -       pr_warn("** administrator!                                       **\n");
> > -       pr_warn("**                                                      **\n");
> > -       pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> > -       pr_warn("**********************************************************\n");

-- 
With Best Regards,
Andy Shevchenko

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

[David Laight]

...
> On Mon, Mar 08, 2021 at 07:23:34PM +0100, Marco Elver wrote:
> > On Mon, 8 Mar 2021 at 18:23, Petr Mladek <pmladek@suse.com> wrote:
> 
> > > -       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");
> >
> > While we're here: This paragraph can be shortened by saying what
> > kernel/trace/trace.c says ("..., report this immediately to your
> > vendor!") which avoids the "administrator! <lots of wasted spaces>".
> 
> Aren't we discussed that and the point was that kernel configuration option is
> in administrator's realm?
> 
> > > -       pr_warn("** administrator!                                       **\n");
> > > -       pr_warn("**                                                      **\n");
> > > -       pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> > > -       pr_warn("**********************************************************\n");

If you actually want anyone to notice it you need to splat it out
when root logs in - not just hidden in the middle of the boot messages.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Timur Tabi]

On Mon, Mar 8, 2021 at 4:51 AM Marco Elver <elver@google.com> wrote:
> 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.

Maybe we can have only the message itself wrapped in an #ifdef CONFIG_
of some kind.  For example:

+#ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
       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");
...
+#endif

       return 0;
}

In other words, if space is really constrained, then don't include the
message.  Or maybe just include part of the message.

Re: [PATCH 1/2] lib/vsprintf: do not show no_hash_pointers message multiple times

[Marco Elver]

On Mon, 8 Mar 2021 at 11:01, Petr Mladek <pmladek@suse.com> wrote:
> On Fri 2021-03-05 20:42:05, Marco Elver wrote:
> > Do not show no_hash_pointers message multiple times if the option was
> > passed more than once (e.g. via generated command line).
> >
> > Signed-off-by: Marco Elver <elver@google.com>
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Could you pick up this patch only?
I think there's still controversy around how to best proceed with
reducing space and we should drop patch 2/2 for now.

If you'd like me to re-send this standalone I can do so as well.

Thanks,
-- Marco

Re: [PATCH 1/2] lib/vsprintf: do not show no_hash_pointers message multiple times

[Petr Mladek]

On Wed 2021-03-17 20:34:43, Marco Elver wrote:
> On Mon, 8 Mar 2021 at 11:01, Petr Mladek <pmladek@suse.com> wrote:
> > On Fri 2021-03-05 20:42:05, Marco Elver wrote:
> > > Do not show no_hash_pointers message multiple times if the option was
> > > passed more than once (e.g. via generated command line).
> > >
> > > Signed-off-by: Marco Elver <elver@google.com>
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> Could you pick up this patch only?
> I think there's still controversy around how to best proceed with
> reducing space and we should drop patch 2/2 for now.
> 
> If you'd like me to re-send this standalone I can do so as well.

I have just committed the patch into printk/linux.git, branch
for-5.13.

Best Regards,
Petr