Re: [PATCH v4 2/6] printk: honor the max_reason field in kmsg_dumper

2020-05-22 Thread Kees Cook 
On Fri, May 22, 2020 at 06:51:20PM +0200, Petr Mladek wrote:
> On Fri 2020-05-15 11:44:30, Kees Cook wrote:
> > From: Pavel Tatashin 
> > 
> > kmsg_dump() allows to dump kmesg buffer for various system events: oops,
> > panic, reboot, etc. It provides an interface to register a callback call
> > for clients, and in that callback interface there is a field "max_reason"
> > which gets ignored unless always_kmsg_dump is passed as kernel parameter.
> 
> Strictly speaking, this is not fully true. "max_reason" field is not
> ignored when set to KMSG_DUMP_PANIC even when always_kmsg_dump was not set.
> 
> It should be something like:
> 
> "which gets ignored for reason higher than KMSG_DUMP_OOPS unless
> always_kmsg_dump is passed as kernel parameter".
> 
> Heh, I wonder if anyone will be able to parse this ;-)

Ah yeah, good point. I've reworded things like this:


kmsg_dump() allows to dump kmesg buffer for various system events: oops,
panic, reboot, etc. It provides an interface to register a callback
call for clients, and in that callback interface there is a field
"max_reason", but it was getting ignored when set to any "reason"
higher than KMSG_DUMP_OOPS unless "always_kmsg_dump" was passed as
kernel parameter.

Allow clients to actually control their "max_reason", and keep the
current behavior when "max_reason" is not set.

> Otherwise, it looks good to me. With the updated commit message:
> 
> Reviewed-by: Petr Mladek 

Thanks!

-- 
Kees Cook

Re: [PATCH v4 2/6] printk: honor the max_reason field in kmsg_dumper

2020-05-22 Thread Petr Mladek 
On Fri 2020-05-15 11:44:30, Kees Cook wrote:
> From: Pavel Tatashin 
> 
> kmsg_dump() allows to dump kmesg buffer for various system events: oops,
> panic, reboot, etc. It provides an interface to register a callback call
> for clients, and in that callback interface there is a field "max_reason"
> which gets ignored unless always_kmsg_dump is passed as kernel parameter.

Strictly speaking, this is not fully true. "max_reason" field is not
ignored when set to KMSG_DUMP_PANIC even when always_kmsg_dump was not set.

It should be something like:

"which gets ignored for reason higher than KMSG_DUMP_OOPS unless
always_kmsg_dump is passed as kernel parameter".

Heh, I wonder if anyone will be able to parse this ;-)


Otherwise, it looks good to me. With the updated commit message:

Reviewed-by: Petr Mladek 

Best Regards,
Petr

[PATCH v4 2/6] printk: honor the max_reason field in kmsg_dumper

2020-05-15 Thread Kees Cook 
From: Pavel Tatashin 

kmsg_dump() allows to dump kmesg buffer for various system events: oops,
panic, reboot, etc. It provides an interface to register a callback call
for clients, and in that callback interface there is a field "max_reason"
which gets ignored unless always_kmsg_dump is passed as kernel parameter.

Allow clients to decide max_reason, and keep the current behavior when
max_reason is not set.

Signed-off-by: Pavel Tatashin 
Link: https://lore.kernel.org/lkml/20200506211523.15077-2-keesc...@chromium.org/
Signed-off-by: Kees Cook 
---
 include/linux/kmsg_dump.h |  1 +
 kernel/printk/printk.c| 15 +++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index 3f82b5cb2d82..9826014771ab 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -26,6 +26,7 @@ enum kmsg_dump_reason {
KMSG_DUMP_OOPS,
KMSG_DUMP_EMERG,
KMSG_DUMP_SHUTDOWN,
+   KMSG_DUMP_MAX
 };
 
 /**
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9a9b6156270b..a121c2255737 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3157,12 +3157,19 @@ void kmsg_dump(enum kmsg_dump_reason reason)
struct kmsg_dumper *dumper;
unsigned long flags;
 
-   if ((reason > KMSG_DUMP_OOPS) && !always_kmsg_dump)
-   return;
-
rcu_read_lock();
list_for_each_entry_rcu(dumper, _list, list) {
-   if (dumper->max_reason && reason > dumper->max_reason)
+   enum kmsg_dump_reason max_reason = dumper->max_reason;
+
+   /*
+* If client has not provided a specific max_reason, default
+* to KMSG_DUMP_OOPS, unless always_kmsg_dump was set.
+*/
+   if (max_reason == KMSG_DUMP_UNDEF) {
+   max_reason = always_kmsg_dump ? KMSG_DUMP_MAX :
+   KMSG_DUMP_OOPS;
+   }
+   if (reason > max_reason)
continue;
 
/* initialize iterator with data about the stored records */
-- 
2.20.1