Messages in this thread | | | From | Seunghun Han <> | Date | Sat, 3 Mar 2018 05:29:54 +0900 | Subject | Re: [PATCH V2] x86: mce: fix kernel panic when check_interval is changed |
| |
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@alien8.de>: > 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.
| |