Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
On (20/05/05 00:21), Pavel Tatashin wrote:
> > > I changed it to make code cleaner: for such basic operation there are
> > > too many conditions if we will keep it inside the kmsg_dump().
> > > However, if being able to set always_kmsg_dump dynamically during
> > > runtime is deemed important, I can change it back to be checked in
> > > kmsg_dump.
> >
> > If you agree that we do not have to modify this variable dynamically,
> > I will also change the permission here:
> > module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO |
> > S_IWUSR);
>
> Hi Sergey,
Hi Pavel,
> After thinking about this. I will move this logic back to kmsg_dump(),
> to keep the current behavior where kmsg_dump can be modified during
> runtime.
Agreed. I think that sysfs knobs and user-visible API need to preserve
their behaviour. There is a deprecation protocol, but usually it takes
many years of WARN_ON() and pr_err("this knob will be removed") before
we can change anything. E.g. sysctl has been deprecated for about a
decade IIRC before it was actually removed.
-ss
Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
> > I changed it to make code cleaner: for such basic operation there are > > too many conditions if we will keep it inside the kmsg_dump(). > > However, if being able to set always_kmsg_dump dynamically during > > runtime is deemed important, I can change it back to be checked in > > kmsg_dump. > > If you agree that we do not have to modify this variable dynamically, > I will also change the permission here: > module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | > S_IWUSR); Hi Sergey, After thinking about this. I will move this logic back to kmsg_dump(), to keep the current behavior where kmsg_dump can be modified during runtime. Thank you, Pasha
Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
On Mon, May 4, 2020 at 10:52 PM Pavel Tatashin
wrote:
>
> > > @@ -3157,12 +3162,9 @@ 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)
> > > + if (reason > dumper->max_reason)
> > > continue;
> >
> > Why always_kmsg_dump check moved from the dumper loop entry point to the
> > dumper registration code? What if the user change always_ksmsg_dump
> > dynamically via sysfs?
>
> Hi Sergey,
>
> I changed it to make code cleaner: for such basic operation there are
> too many conditions if we will keep it inside the kmsg_dump().
> However, if being able to set always_kmsg_dump dynamically during
> runtime is deemed important, I can change it back to be checked in
> kmsg_dump.
If you agree that we do not have to modify this variable dynamically,
I will also change the permission here:
module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
>
> Thank you,
> Pasha
>
> >
> > -ss
Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
> > @@ -3157,12 +3162,9 @@ 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)
> > + if (reason > dumper->max_reason)
> > continue;
>
> Why always_kmsg_dump check moved from the dumper loop entry point to the
> dumper registration code? What if the user change always_ksmsg_dump
> dynamically via sysfs?
Hi Sergey,
I changed it to make code cleaner: for such basic operation there are
too many conditions if we will keep it inside the kmsg_dump().
However, if being able to set always_kmsg_dump dynamically during
runtime is deemed important, I can change it back to be checked in
kmsg_dump.
Thank you,
Pasha
>
> -ss
Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
On (20/05/02 10:35), Pavel Tatashin wrote:
[..]
> +static bool always_kmsg_dump;
> +module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO |
> S_IWUSR);
>
> /**
> * kmsg_dump_register - register a kernel log dumper.
> @@ -3106,6 +3108,12 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
> spin_lock_irqsave(_list_lock, flags);
> /* Don't allow registering multiple times */
> if (!dumper->registered) {
> + if (!dumper->max_reason) {
> + if (always_kmsg_dump)
> + dumper->max_reason = KMSG_DUMP_MAX;
> + else
> + dumper->max_reason = KMSG_DUMP_OOPS;
> + }
> dumper->registered = 1;
> list_add_tail_rcu(>list, _list);
> err = 0;
[..]
> @@ -3157,12 +3162,9 @@ 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)
> + if (reason > dumper->max_reason)
> continue;
Why always_kmsg_dump check moved from the dumper loop entry point to the
dumper registration code? What if the user change always_ksmsg_dump
dynamically via sysfs?
-ss
Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
Thank Kees, I think it is a little cleaner this way.
Thank you,
Pasha
On Mon, May 4, 2020 at 2:12 PM Kees Cook wrote:
>
> On Mon, May 04, 2020 at 01:15:00PM -0400, Steven Rostedt wrote:
> > On Sat, 2 May 2020 10:35:53 -0400
> > Pavel Tatashin wrote:
> >
> > > 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
> > > ---
> > > include/linux/kmsg_dump.h | 1 +
> > > kernel/printk/printk.c| 16 +---
> > > 2 files changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> > > index 2e7a1e032c71..c0d703b7ce38 100644
> > > --- a/include/linux/kmsg_dump.h
> > > +++ b/include/linux/kmsg_dump.h
> > > @@ -28,6 +28,7 @@ enum kmsg_dump_reason {
> > > KMSG_DUMP_RESTART,
> > > KMSG_DUMP_HALT,
> > > KMSG_DUMP_POWEROFF,
> > > + KMSG_DUMP_MAX = KMSG_DUMP_POWEROFF
> >
> > Hmm, I didn't realize that enums were allowed to have duplicates. That can
> > usually screw up logic. I would recommend making that a define afterward.
> >
> > #define KMSG_DUMP_MAX KMSG_DUMP_POWEROFF
> >
> > As is done in other locations of the kernel.
>
> I've seen it also be the last item in an enum, then comparisons can just
> do "< KMSG_DUMP_MAX" instead of "<= KMSG_DUMP_MAX".
>
> --
> Kees Cook
Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
On Mon, May 04, 2020 at 01:15:00PM -0400, Steven Rostedt wrote:
> On Sat, 2 May 2020 10:35:53 -0400
> Pavel Tatashin wrote:
>
> > 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
> > ---
> > include/linux/kmsg_dump.h | 1 +
> > kernel/printk/printk.c| 16 +---
> > 2 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> > index 2e7a1e032c71..c0d703b7ce38 100644
> > --- a/include/linux/kmsg_dump.h
> > +++ b/include/linux/kmsg_dump.h
> > @@ -28,6 +28,7 @@ enum kmsg_dump_reason {
> > KMSG_DUMP_RESTART,
> > KMSG_DUMP_HALT,
> > KMSG_DUMP_POWEROFF,
> > + KMSG_DUMP_MAX = KMSG_DUMP_POWEROFF
>
> Hmm, I didn't realize that enums were allowed to have duplicates. That can
> usually screw up logic. I would recommend making that a define afterward.
>
> #define KMSG_DUMP_MAX KMSG_DUMP_POWEROFF
>
> As is done in other locations of the kernel.
I've seen it also be the last item in an enum, then comparisons can just
do "< KMSG_DUMP_MAX" instead of "<= KMSG_DUMP_MAX".
--
Kees Cook
Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
> Hmm, I didn't realize that enums were allowed to have duplicates. That can > usually screw up logic. I would recommend making that a define afterward. > > #define KMSG_DUMP_MAX KMSG_DUMP_POWEROFF > > As is done in other locations of the kernel. > Hi Steve, Sure, I will change it to define. Thank you, Pasha
Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
On Sat, 2 May 2020 10:35:53 -0400
Pavel Tatashin wrote:
> 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
> ---
> include/linux/kmsg_dump.h | 1 +
> kernel/printk/printk.c| 16 +---
> 2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> index 2e7a1e032c71..c0d703b7ce38 100644
> --- a/include/linux/kmsg_dump.h
> +++ b/include/linux/kmsg_dump.h
> @@ -28,6 +28,7 @@ enum kmsg_dump_reason {
> KMSG_DUMP_RESTART,
> KMSG_DUMP_HALT,
> KMSG_DUMP_POWEROFF,
> + KMSG_DUMP_MAX = KMSG_DUMP_POWEROFF
Hmm, I didn't realize that enums were allowed to have duplicates. That can
usually screw up logic. I would recommend making that a define afterward.
#define KMSG_DUMP_MAX KMSG_DUMP_POWEROFF
As is done in other locations of the kernel.
The rest looks fine to me.
-- Steve
> };
>
> /**
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 9a9b6156270b..04c1e9a9b139 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3085,6 +3085,8 @@ EXPORT_SYMBOL(printk_timed_ratelimit);
>
> static DEFINE_SPINLOCK(dump_list_lock);
> static LIST_HEAD(dump_list);
> +static bool always_kmsg_dump;
> +module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO |
> S_IWUSR);
>
> /**
> * kmsg_dump_register - register a kernel log dumper.
> @@ -3106,6 +3108,12 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
> spin_lock_irqsave(_list_lock, flags);
> /* Don't allow registering multiple times */
> if (!dumper->registered) {
> + if (!dumper->max_reason) {
> + if (always_kmsg_dump)
> + dumper->max_reason = KMSG_DUMP_MAX;
> + else
> + dumper->max_reason = KMSG_DUMP_OOPS;
> + }
> dumper->registered = 1;
> list_add_tail_rcu(>list, _list);
> err = 0;
> @@ -3141,9 +3149,6 @@ int kmsg_dump_unregister(struct kmsg_dumper *dumper)
> }
> EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
>
> -static bool always_kmsg_dump;
> -module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO |
> S_IWUSR);
> -
> /**
> * kmsg_dump - dump kernel log to kernel message dumpers.
> * @reason: the reason (oops, panic etc) for dumping
> @@ -3157,12 +3162,9 @@ 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)
> + if (reason > dumper->max_reason)
> continue;
>
> /* initialize iterator with data about the stored records */
[PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper
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
---
include/linux/kmsg_dump.h | 1 +
kernel/printk/printk.c| 16 +---
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index 2e7a1e032c71..c0d703b7ce38 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -28,6 +28,7 @@ enum kmsg_dump_reason {
KMSG_DUMP_RESTART,
KMSG_DUMP_HALT,
KMSG_DUMP_POWEROFF,
+ KMSG_DUMP_MAX = KMSG_DUMP_POWEROFF
};
/**
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9a9b6156270b..04c1e9a9b139 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3085,6 +3085,8 @@ EXPORT_SYMBOL(printk_timed_ratelimit);
static DEFINE_SPINLOCK(dump_list_lock);
static LIST_HEAD(dump_list);
+static bool always_kmsg_dump;
+module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO |
S_IWUSR);
/**
* kmsg_dump_register - register a kernel log dumper.
@@ -3106,6 +3108,12 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
spin_lock_irqsave(_list_lock, flags);
/* Don't allow registering multiple times */
if (!dumper->registered) {
+ if (!dumper->max_reason) {
+ if (always_kmsg_dump)
+ dumper->max_reason = KMSG_DUMP_MAX;
+ else
+ dumper->max_reason = KMSG_DUMP_OOPS;
+ }
dumper->registered = 1;
list_add_tail_rcu(>list, _list);
err = 0;
@@ -3141,9 +3149,6 @@ int kmsg_dump_unregister(struct kmsg_dumper *dumper)
}
EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
-static bool always_kmsg_dump;
-module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO |
S_IWUSR);
-
/**
* kmsg_dump - dump kernel log to kernel message dumpers.
* @reason: the reason (oops, panic etc) for dumping
@@ -3157,12 +3162,9 @@ 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)
+ if (reason > dumper->max_reason)
continue;
/* initialize iterator with data about the stored records */
--
2.25.1
