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, &dump_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, &dump_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(&dump_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(&dumper->list, &dump_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, &dump_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(&dump_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(&dumper->list, &dump_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, &dump_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(&dump_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(&dumper->list, &dump_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, &dump_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