diff mbox series

[printk,v1] printk: fix illegal pbufs access for !CONFIG_PRINTK

Message ID 20230920155238.670439-1-john.ogness@linutronix.de
State New
Headers
Series [printk,v1] printk: fix illegal pbufs access for !CONFIG_PRINTK |

Commit Message

John Ogness Sept. 20, 2023, 3:52 p.m. UTC 
  When CONFIG_PRINTK is not set, PRINTK_MESSAGE_MAX is 0. This
leads to a zero-sized array @outbuf in @printk_shared_pbufs. In
console_flush_all() a pointer to the first element of the array
is assigned with:

   char *outbuf = &printk_shared_pbufs.outbuf[0];

For !CONFIG_PRINTK this leads to a compiler warning:

   warning: array subscript 0 is outside array bounds of
   'char[0]' [-Warray-bounds]

This is not really dangerous because printk_get_next_message()
always returns false for !CONFIG_PRINTK, which leads to @outbuf
never being used. However, it makes no sense to even compile
these functions for !CONFIG_PRINTK.

Extend the existing '#ifdef CONFIG_PRINTK' block to contain
the formatting and emitting functions since these have no
purpose in !CONFIG_PRINTK. This also allows removing several
more !CONFIG_PRINTK dummies as well as moving
@suppress_panic_printk into a CONFIG_PRINTK block.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202309201724.M9BMAQIh-lkp@intel.com/
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 44 +++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)


base-commit: 9757acd0a700ba4a0d16dde4ba820eb052aba1a7

Comments

Sergey Senozhatsky Sept. 21, 2023, 2:18 a.m. UTC | #1 
On (23/09/20 17:58), John Ogness wrote:
> When CONFIG_PRINTK is not set, PRINTK_MESSAGE_MAX is 0. This
> leads to a zero-sized array @outbuf in @printk_shared_pbufs. In
> console_flush_all() a pointer to the first element of the array
> is assigned with:
> 
>    char *outbuf = &printk_shared_pbufs.outbuf[0];
> 
> For !CONFIG_PRINTK this leads to a compiler warning:
> 
>    warning: array subscript 0 is outside array bounds of
>    'char[0]' [-Warray-bounds]
> 
> This is not really dangerous because printk_get_next_message()
> always returns false for !CONFIG_PRINTK, which leads to @outbuf
> never being used. However, it makes no sense to even compile
> these functions for !CONFIG_PRINTK.

I wonder if anyone really use !PRINTK kernels. Can we get rid
of CONFIG_PRINTK?

> Extend the existing '#ifdef CONFIG_PRINTK' block to contain
> the formatting and emitting functions since these have no
> purpose in !CONFIG_PRINTK. This also allows removing several
> more !CONFIG_PRINTK dummies as well as moving
> @suppress_panic_printk into a CONFIG_PRINTK block.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202309201724.M9BMAQIh-lkp@intel.com/
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

FWIW,
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
John Ogness Sept. 21, 2023, 7:13 a.m. UTC | #2 
On 2023-09-21, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> I wonder if anyone really use !PRINTK kernels. Can we get rid
> of CONFIG_PRINTK?

It is used. It is one of the big annoyances during the last several
years of the rework. I get bug reports relatively quickly after breaking
!CONFIG_PRINTK. The reports come mostly from the kbuild robots, but also
from real people.

If someone has limited space/memory requirements and does not care about
dmesg, they can save a considerable amount of kernel size and memory by
turning all that off. The problem right now is that !CONFIG_PRINTNK is
horribly hacked together with dummy implementations and useless real
functions that pretend to do stuff.

After the rework we can work on splitting out the code based on
functionality. If done right, it will be trivial to "implement"
!CONFIG_PRINTK in such a way that changes to real code don't explode
every time on !CONFIG_PRINTK.

John
Sergey Senozhatsky Sept. 21, 2023, 9:44 a.m. UTC | #3 
On (23/09/21 09:19), John Ogness wrote:
> On 2023-09-21, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> > I wonder if anyone really use !PRINTK kernels. Can we get rid
> > of CONFIG_PRINTK?
> 
> It is used. It is one of the big annoyances during the last several
> years of the rework. I get bug reports relatively quickly after breaking
> !CONFIG_PRINTK. The reports come mostly from the kbuild robots, but also
> from real people.

Right, that has happened to almost everyone who has ever submitted
patches to printk, even dramatically simpler than yours and Petr's
patches (e.g. my printk patches).

> If someone has limited space/memory requirements and does not care about
> dmesg, they can save a considerable amount of kernel size and memory by
> turning all that off. The problem right now is that !CONFIG_PRINTNK is
> horribly hacked together with dummy implementations and useless real
> functions that pretend to do stuff.

I was somehow thinking that prb is the biggest memory consumer on such
systems, but now that I looked at it, even on my very trimmed .config
the difference between PRINTK and !PRINTK is pretty huge:

./scripts/bloat-o-meter vmlinux.o.printk vmlinux.o.noprintk
add/remove: 79/643 grow/shrink: 235/3169 up/down: 30532/-1488150 (-1457618)
...
Total: Before=31118934, After=29661316, chg -4.68%

And !PRINTK doesn't even completely eliminate printk-s footprint.
All those temp buffers that are used for sprintf/printk are still
there. For example:

void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
{
        /* Use static buffer, for the caller is holding oom_lock. */
        static char buf[PAGE_SIZE];
        struct seq_buf s;

	...

        memory_stat_format(memcg, &s);
        seq_buf_do_printk(&s, KERN_INFO);
}

in !PRINTK builds seq_buf_do_printk() doesn't do anything useful, yet
the buffer (and the code that fills that buffer) is (are) still there.

> After the rework we can work on splitting out the code based on
> functionality. If done right, it will be trivial to "implement"
> !CONFIG_PRINTK in such a way that changes to real code don't explode
> every time on !CONFIG_PRINTK.

Sounds good.
And sorry for the noise.
diff mbox series

Patch

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 778359b21761..7f40d9122caa 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -102,12 +102,6 @@  DEFINE_STATIC_SRCU(console_srcu);
  */
 int __read_mostly suppress_printk;
 
-/*
- * During panic, heavy printk by other CPUs can delay the
- * panic and risk deadlock on console resources.
- */
-static int __read_mostly suppress_panic_printk;
-
 #ifdef CONFIG_LOCKDEP
 static struct lockdep_map console_lock_dep_map = {
 	.name = "console_lock"
@@ -445,6 +439,12 @@  static int console_msg_format = MSG_FORMAT_DEFAULT;
 static DEFINE_MUTEX(syslog_lock);
 
 #ifdef CONFIG_PRINTK
+/*
+ * During panic, heavy printk by other CPUs can delay the
+ * panic and risk deadlock on console resources.
+ */
+static int __read_mostly suppress_panic_printk;
+
 DECLARE_WAIT_QUEUE_HEAD(log_wait);
 /* All 3 protected by @syslog_lock. */
 /* the next printk record to read by syslog(READ) or /proc/kmsg */
@@ -2346,22 +2346,6 @@  static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
 
 static u64 syslog_seq;
 
-static size_t record_print_text(const struct printk_record *r,
-				bool syslog, bool time)
-{
-	return 0;
-}
-static ssize_t info_print_ext_header(char *buf, size_t size,
-				     struct printk_info *info)
-{
-	return 0;
-}
-static ssize_t msg_print_ext_body(char *buf, size_t size,
-				  char *text, size_t text_len,
-				  struct dev_printk_info *dev_info) { return 0; }
-static void console_lock_spinning_enable(void) { }
-static int console_lock_spinning_disable_and_check(int cookie) { return 0; }
-static bool suppress_message_printing(int level) { return false; }
 static bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
 static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; }
 
@@ -2715,6 +2699,8 @@  static void __console_unlock(void)
 	up_console_sem();
 }
 
+#ifdef CONFIG_PRINTK
+
 /*
  * Prepend the message in @pmsg->pbufs->outbuf with a "dropped message". This
  * is achieved by shifting the existing message over and inserting the dropped
@@ -2729,7 +2715,6 @@  static void __console_unlock(void)
  *
  * If @pmsg->pbufs->outbuf is modified, @pmsg->outbuf_len is updated.
  */
-#ifdef CONFIG_PRINTK
 void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
 {
 	struct printk_buffers *pbufs = pmsg->pbufs;
@@ -2761,9 +2746,6 @@  void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
 	memcpy(outbuf, scratchbuf, len);
 	pmsg->outbuf_len += len;
 }
-#else
-#define console_prepend_dropped(pmsg, dropped)
-#endif /* CONFIG_PRINTK */
 
 /*
  * Read and format the specified record (or a later record if the specified
@@ -2921,6 +2903,16 @@  static bool console_emit_next_record(struct console *con, bool *handover, int co
 	return true;
 }
 
+#else
+
+static bool console_emit_next_record(struct console *con, bool *handover, int cookie)
+{
+	*handover = false;
+	return false;
+}
+
+#endif /* CONFIG_PRINTK */
+
 /*
  * Print out all remaining records to all consoles.
  *