Hi Marco, Thanks for your reply.
May 17, 2022 11:42 AM, "Marco Elver" <elver@xxxxxxxxxx> 写到:
> On Tue, May 17, 2022 at 07:15PM +0800, Jackie Liu wrote:
>
>> From: Jackie Liu <liuyun01@xxxxxxxxxx>
>>
>> By printing information, we can friendly prompt the status change
>> information of kfence by dmesg.
>>
>> Signed-off-by: Jackie Liu <liuyun01@xxxxxxxxxx>
>
> 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
