Re: [PATCH V2] x86: mce: fix kernel panic when check_interval is changed

From: Seunghun Han
Date: Fri Mar 02 2018 - 15:30:13 EST


Hi, Borislav.

Thank you for your good advice.
According to your advice, I will make and send PATCH v3.

Best regards.

Seunghun.

2018-03-02 21:14 GMT+09:00 Borislav Petkov <bp@xxxxxxxxx>:
> On Thu, Mar 01, 2018 at 05:31:31AM +0900, Seunghun Han wrote:
>> Changes since v1: add mce_sysfs_mutex according to review result.
>
> Ok, we're getting there.
>
>> arch/x86/kernel/cpu/mcheck/mce.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>> index 706584681a4c..bee0795a3b8c 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> @@ -55,6 +55,7 @@
>> #include "mce-internal.h"
>>
>> static DEFINE_MUTEX(mce_log_mutex);
>> +static DEFINE_MUTEX(mce_sysfs_mutex);
>>
>> #define CREATE_TRACE_POINTS
>> #include <trace/events/mce.h>
>> @@ -2045,8 +2046,11 @@ static void mce_enable_ce(void *all)
>> return;
>> cmci_reenable();
>> cmci_recheck();
>> - if (all)
>> + if (all) {
>> + mutex_lock(&mce_sysfs_mutex);
>> __mcheck_cpu_init_timer();
>> + mutex_unlock(&mce_sysfs_mutex);
>> + }
>
> Instead of chasing all the places which need to grab the mutex, I was
> thinking of grabbing it in all those functions which modify MCA behavior
> non-trivially from sysfs. And those are:
>
> store_int_with_restart()
> set_ignore_ce()
> set_cmci_disabled()
>
> And you've done that already for store_int_with_restart() and I think
> doing the same in the other two will address the issue nicely and
> in the simplest way possible.
>
> All those sysfs operations should be synchronized anyway. Please reflect
> that in your commit message too.
>
> The aspect about DOS is not really important because you need to be root
> to write those so if you have root, there are gazillion ways to shoot
> yourself in the foot already.
>
>> static struct bus_type mce_subsys = {
>> @@ -2132,8 +2136,14 @@ static ssize_t store_int_with_restart(struct device *s,
>> struct device_attribute *attr,
>> const char *buf, size_t size)
>> {
>> + unsigned long old_check_interval = check_interval;
>> ssize_t ret = device_store_int(s, attr, buf, size);
>> +
>> + if (check_interval == old_check_interval)
>> + return ret;
>
> <---- newline here.
>
>> + mutex_lock(&mce_sysfs_mutex);
>> mce_restart();
>> + mutex_unlock(&mce_sysfs_mutex);
>
> <---- newline here.
>
>> return ret;
>> }
>>
>> --
>> 2.16.2
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.