| Message ID | 20220517111551.4077061-1-liu.yun@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/kfence: print disabling or re-enabling message | expand |
On Tue, May 17, 2022 at 07:15PM +0800, Jackie Liu wrote: > From: Jackie Liu <liuyun01@kylinos.cn> > > By printing information, we can friendly prompt the status change > information of kfence by dmesg. > > Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> Personally, I've never found this useful. If I want to get the current accurate state of KFENCE enablement, I just look at /sys/kernel/debug/kfence/stats. Nevertheless, some comments below. > --- > mm/kfence/core.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > index 11a954763be9..beb552089b67 100644 > --- a/mm/kfence/core.c > +++ b/mm/kfence/core.c > @@ -67,8 +67,11 @@ static int param_set_sample_interval(const char *val, const struct kernel_param > if (ret < 0) > return ret; > > - if (!num) /* Using 0 to indicate KFENCE is disabled. */ > + if (!num) { > + /* Using 0 to indicate KFENCE is disabled. */ > WRITE_ONCE(kfence_enabled, false); > + pr_info("KFENCE is disabled.\n"); This will also print on boot if kfence.sample_interval=0 is passed. This is ugly. We also have a pr_fmt, and writing "KFENCE" again is ugly, too. And adding '.' at the end of these short log lines is not something done much in the kernel, and also ugly. So what you want is this fixup: diff --git a/mm/kfence/core.c b/mm/kfence/core.c index beb552089b67..de5bcf2609fe 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -67,10 +67,11 @@ static int param_set_sample_interval(const char *val, const struct kernel_param if (ret < 0) return ret; + /* Using 0 to indicate KFENCE is disabled. */ if (!num) { - /* Using 0 to indicate KFENCE is disabled. */ + if (READ_ONCE(kfence_enabled)) + pr_info("disabled\n"); WRITE_ONCE(kfence_enabled, false); - pr_info("KFENCE is disabled.\n"); } *((unsigned long *)kp->arg) = num; @@ -877,7 +878,7 @@ static int kfence_enable_late(void) WRITE_ONCE(kfence_enabled, true); queue_delayed_work(system_unbound_wq, &kfence_timer, 0); - pr_info("KFENCE is re-enabled.\n"); + pr_info("re-enabled\n"); return 0; }
Hi Marco, Thanks for your reply. May 17, 2022 11:42 AM, "Marco Elver" <elver@google.com> 写到: > On Tue, May 17, 2022 at 07:15PM +0800, Jackie Liu wrote: > >> From: Jackie Liu <liuyun01@kylinos.cn> >> >> By printing information, we can friendly prompt the status change >> information of kfence by dmesg. >> >> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> > > Personally, I've never found this useful. If I want to get the current > accurate state of KFENCE enablement, I just look at > /sys/kernel/debug/kfence/stats. Yes, I can get the status through this file, but there is no other place to indicate that the status has changed. By logging in kmsg, it can not only reflect the status change through dmesg, but also be recorded by programs such as syslog. This is very useful for me. > > Nevertheless, some comments below. > >> --- >> mm/kfence/core.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/mm/kfence/core.c b/mm/kfence/core.c >> index 11a954763be9..beb552089b67 100644 >> --- a/mm/kfence/core.c >> +++ b/mm/kfence/core.c >> @@ -67,8 +67,11 @@ static int param_set_sample_interval(const char *val, const struct kernel_param >> if (ret < 0) >> return ret; >> >> - if (!num) /* Using 0 to indicate KFENCE is disabled. */ >> + if (!num) { >> + /* Using 0 to indicate KFENCE is disabled. */ >> WRITE_ONCE(kfence_enabled, false); >> + pr_info("KFENCE is disabled.\n"); > > This will also print on boot if kfence.sample_interval=0 is passed. This > is ugly. > > We also have a pr_fmt, and writing "KFENCE" again is ugly, too. And > adding '.' at the end of these short log lines is not something done > much in the kernel, and also ugly. > > So what you want is this fixup: > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > index beb552089b67..de5bcf2609fe 100644 > --- a/mm/kfence/core.c > +++ b/mm/kfence/core.c > @@ -67,10 +67,11 @@ static int param_set_sample_interval(const char *val, const struct kernel_param > if (ret < 0) > return ret; > > + /* Using 0 to indicate KFENCE is disabled. */ > if (!num) { > - /* Using 0 to indicate KFENCE is disabled. */ > + if (READ_ONCE(kfence_enabled)) > + pr_info("disabled\n"); > WRITE_ONCE(kfence_enabled, false); > - pr_info("KFENCE is disabled.\n"); > } > > *((unsigned long *)kp->arg) = num; > @@ -877,7 +878,7 @@ static int kfence_enable_late(void) > > WRITE_ONCE(kfence_enabled, true); > queue_delayed_work(system_unbound_wq, &kfence_timer, 0); > - pr_info("KFENCE is re-enabled.\n"); > + pr_info("re-enabled\n"); > return 0; > } Thanks for you fixup. -- Jackie Liu
diff --git a/mm/kfence/core.c b/mm/kfence/core.c index 11a954763be9..beb552089b67 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -67,8 +67,11 @@ static int param_set_sample_interval(const char *val, const struct kernel_param if (ret < 0) return ret; - if (!num) /* Using 0 to indicate KFENCE is disabled. */ + if (!num) { + /* Using 0 to indicate KFENCE is disabled. */ WRITE_ONCE(kfence_enabled, false); + pr_info("KFENCE is disabled.\n"); + } *((unsigned long *)kp->arg) = num; @@ -874,6 +877,7 @@ static int kfence_enable_late(void) WRITE_ONCE(kfence_enabled, true); queue_delayed_work(system_unbound_wq, &kfence_timer, 0); + pr_info("KFENCE is re-enabled.\n"); return 0; }