* [RFC] pr_warn_once() issue in x86 MSR extable code
@ 2022-06-17 11:08 Stephane Eranian
2022-06-17 14:52 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Stephane Eranian @ 2022-06-17 11:08 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: LKML, Eric Dumazet, jpoimboe, Steven Rostedt
Hi,
Some changes to the way invalid MSR accesses are reported by the kernel is
causing some problems with messages printed on the console.
We have seen several cases of ex_handler_msr() printing invalid MSR
accesses once but
the callstack multiple times causing confusion on the console.
The last time the exception MSR code was modified (5.16) by PeterZ was:
d52a7344bdfa x86/msr: Remove .fixup usage:
if (!safe && wrmsr && pr_warn_once("unchecked MSR access error: ..."))
show_stack_regs(regs);
Note that this code pattern was also present, though in a different
form, before this commit.
The problem here is that another earlier commit (5.13):
a358f40600b3 once: implement DO_ONCE_LITE for non-fast-path "do once"
functionality
Modifies all the pr_*_once() calls to always return true claiming that
no caller is ever
checking the return value of the functions.
This is why we are seeing the callstack printed without the associated
printk() msg.
I believe that having the pr_*_once() functions return true the first
time they are called
is useful especially when extra information, such as callstack, must
be printed to help
track the origin of the problem.
The exception handling code seems to be the only place where the
return value is checked
for pr_warn_once(). A minimal change would be to create another
version of that function
that calls DO_ONCE() instead of DO_ONCE_LITE(), e.g., pr_warn_once_return().
I can post a patch to that effect if we all agree on the approach.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC] pr_warn_once() issue in x86 MSR extable code 2022-06-17 11:08 [RFC] pr_warn_once() issue in x86 MSR extable code Stephane Eranian @ 2022-06-17 14:52 ` Peter Zijlstra 2022-06-22 17:51 ` Stephane Eranian 2022-07-21 8:52 ` [tip: x86/core] " tip-bot2 for Peter Zijlstra 0 siblings, 2 replies; 6+ messages in thread From: Peter Zijlstra @ 2022-06-17 14:52 UTC (permalink / raw) To: Stephane Eranian; +Cc: LKML, Eric Dumazet, jpoimboe, Steven Rostedt On Fri, Jun 17, 2022 at 02:08:52PM +0300, Stephane Eranian wrote: > Hi, > > Some changes to the way invalid MSR accesses are reported by the kernel is > causing some problems with messages printed on the console. > > We have seen several cases of ex_handler_msr() printing invalid MSR > accesses once but > the callstack multiple times causing confusion on the console. > > The last time the exception MSR code was modified (5.16) by PeterZ was: > > d52a7344bdfa x86/msr: Remove .fixup usage: > > if (!safe && wrmsr && pr_warn_once("unchecked MSR access error: ...")) > show_stack_regs(regs); > > Note that this code pattern was also present, though in a different > form, before this commit. > > The problem here is that another earlier commit (5.13): > > a358f40600b3 once: implement DO_ONCE_LITE for non-fast-path "do once" > functionality > > Modifies all the pr_*_once() calls to always return true claiming that > no caller is ever > checking the return value of the functions. > > This is why we are seeing the callstack printed without the associated > printk() msg. > > I believe that having the pr_*_once() functions return true the first > time they are called > is useful especially when extra information, such as callstack, must > be printed to help > track the origin of the problem. > > The exception handling code seems to be the only place where the > return value is checked > for pr_warn_once(). A minimal change would be to create another > version of that function > that calls DO_ONCE() instead of DO_ONCE_LITE(), e.g., pr_warn_once_return(). > > I can post a patch to that effect if we all agree on the approach. > > Thanks. How about something like this? diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c index dba2197c05c3..331310c29349 100644 --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -94,16 +94,18 @@ static bool ex_handler_copy(const struct exception_table_entry *fixup, static bool ex_handler_msr(const struct exception_table_entry *fixup, struct pt_regs *regs, bool wrmsr, bool safe, int reg) { - if (!safe && wrmsr && - pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", - (unsigned int)regs->cx, (unsigned int)regs->dx, - (unsigned int)regs->ax, regs->ip, (void *)regs->ip)) + if (__ONCE_LITE_IF(!safe && wrmsr)) { + pr_warn("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", + (unsigned int)regs->cx, (unsigned int)regs->dx, + (unsigned int)regs->ax, regs->ip, (void *)regs->ip); show_stack_regs(regs); + } - if (!safe && !wrmsr && - pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", - (unsigned int)regs->cx, regs->ip, (void *)regs->ip)) + if (__ONCE_LITE_IF(!safe && !wrmsr)) { + pr_warn("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", + (unsigned int)regs->cx, regs->ip, (void *)regs->ip); show_stack_regs(regs); + } if (!wrmsr) { /* Pretend that the read succeeded and returned 0. */ diff --git a/include/linux/once_lite.h b/include/linux/once_lite.h index 861e606b820f..63c3bbcef694 100644 --- a/include/linux/once_lite.h +++ b/include/linux/once_lite.h @@ -9,15 +9,27 @@ */ #define DO_ONCE_LITE(func, ...) \ DO_ONCE_LITE_IF(true, func, ##__VA_ARGS__) -#define DO_ONCE_LITE_IF(condition, func, ...) \ + +#define __ONCE_LITE_IF(condition) \ ({ \ static bool __section(".data.once") __already_done; \ - bool __ret_do_once = !!(condition); \ + bool __ret_cond = !!(condition); \ + bool __ret_once = false; \ \ if (unlikely(__ret_do_once && !__already_done)) { \ __already_done = true; \ - func(__VA_ARGS__); \ + __ret_once = true; \ } \ + unlikely(__ret_once); \ + }) + +#define DO_ONCE_LITE_IF(condition, func, ...) \ + ({ \ + bool __ret_do_once = !!(condition); \ + \ + if (__ONCE_LITE_IF(__ret_do_once)) \ + func(__VA_ARGS__); \ + \ unlikely(__ret_do_once); \ }) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] pr_warn_once() issue in x86 MSR extable code 2022-06-17 14:52 ` Peter Zijlstra @ 2022-06-22 17:51 ` Stephane Eranian 2022-07-20 12:48 ` Stephane Eranian 2022-07-21 8:52 ` [tip: x86/core] " tip-bot2 for Peter Zijlstra 1 sibling, 1 reply; 6+ messages in thread From: Stephane Eranian @ 2022-06-22 17:51 UTC (permalink / raw) To: Peter Zijlstra; +Cc: LKML, Eric Dumazet, jpoimboe, Steven Rostedt Hi Peter, Thanks for taking a quick look at this. I am currently OOO and I cannot test this proposed patch. I am okay with your suggestion. Thanks. On Fri, Jun 17, 2022 at 4:52 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Jun 17, 2022 at 02:08:52PM +0300, Stephane Eranian wrote: > > Hi, > > > > Some changes to the way invalid MSR accesses are reported by the kernel is > > causing some problems with messages printed on the console. > > > > We have seen several cases of ex_handler_msr() printing invalid MSR > > accesses once but > > the callstack multiple times causing confusion on the console. > > > > The last time the exception MSR code was modified (5.16) by PeterZ was: > > > > d52a7344bdfa x86/msr: Remove .fixup usage: > > > > if (!safe && wrmsr && pr_warn_once("unchecked MSR access error: ...")) > > show_stack_regs(regs); > > > > Note that this code pattern was also present, though in a different > > form, before this commit. > > > > The problem here is that another earlier commit (5.13): > > > > a358f40600b3 once: implement DO_ONCE_LITE for non-fast-path "do once" > > functionality > > > > Modifies all the pr_*_once() calls to always return true claiming that > > no caller is ever > > checking the return value of the functions. > > > > This is why we are seeing the callstack printed without the associated > > printk() msg. > > > > I believe that having the pr_*_once() functions return true the first > > time they are called > > is useful especially when extra information, such as callstack, must > > be printed to help > > track the origin of the problem. > > > > The exception handling code seems to be the only place where the > > return value is checked > > for pr_warn_once(). A minimal change would be to create another > > version of that function > > that calls DO_ONCE() instead of DO_ONCE_LITE(), e.g., pr_warn_once_return(). > > > > I can post a patch to that effect if we all agree on the approach. > > > > Thanks. > > How about something like this? > > diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c > index dba2197c05c3..331310c29349 100644 > --- a/arch/x86/mm/extable.c > +++ b/arch/x86/mm/extable.c > @@ -94,16 +94,18 @@ static bool ex_handler_copy(const struct exception_table_entry *fixup, > static bool ex_handler_msr(const struct exception_table_entry *fixup, > struct pt_regs *regs, bool wrmsr, bool safe, int reg) > { > - if (!safe && wrmsr && > - pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", > - (unsigned int)regs->cx, (unsigned int)regs->dx, > - (unsigned int)regs->ax, regs->ip, (void *)regs->ip)) > + if (__ONCE_LITE_IF(!safe && wrmsr)) { > + pr_warn("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", > + (unsigned int)regs->cx, (unsigned int)regs->dx, > + (unsigned int)regs->ax, regs->ip, (void *)regs->ip); > show_stack_regs(regs); > + } > > - if (!safe && !wrmsr && > - pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", > - (unsigned int)regs->cx, regs->ip, (void *)regs->ip)) > + if (__ONCE_LITE_IF(!safe && !wrmsr)) { > + pr_warn("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", > + (unsigned int)regs->cx, regs->ip, (void *)regs->ip); > show_stack_regs(regs); > + } > > if (!wrmsr) { > /* Pretend that the read succeeded and returned 0. */ > diff --git a/include/linux/once_lite.h b/include/linux/once_lite.h > index 861e606b820f..63c3bbcef694 100644 > --- a/include/linux/once_lite.h > +++ b/include/linux/once_lite.h > @@ -9,15 +9,27 @@ > */ > #define DO_ONCE_LITE(func, ...) \ > DO_ONCE_LITE_IF(true, func, ##__VA_ARGS__) > -#define DO_ONCE_LITE_IF(condition, func, ...) \ > + > +#define __ONCE_LITE_IF(condition) \ > ({ \ > static bool __section(".data.once") __already_done; \ > - bool __ret_do_once = !!(condition); \ > + bool __ret_cond = !!(condition); \ > + bool __ret_once = false; \ > \ > if (unlikely(__ret_do_once && !__already_done)) { \ > __already_done = true; \ > - func(__VA_ARGS__); \ > + __ret_once = true; \ > } \ > + unlikely(__ret_once); \ > + }) > + > +#define DO_ONCE_LITE_IF(condition, func, ...) \ > + ({ \ > + bool __ret_do_once = !!(condition); \ > + \ > + if (__ONCE_LITE_IF(__ret_do_once)) \ > + func(__VA_ARGS__); \ > + \ > unlikely(__ret_do_once); \ > }) > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] pr_warn_once() issue in x86 MSR extable code 2022-06-22 17:51 ` Stephane Eranian @ 2022-07-20 12:48 ` Stephane Eranian 2022-07-20 13:44 ` [PATCH] x86/extable: Fix ex_handler_msr() print condition Peter Zijlstra 0 siblings, 1 reply; 6+ messages in thread From: Stephane Eranian @ 2022-07-20 12:48 UTC (permalink / raw) To: Peter Zijlstra; +Cc: LKML, Eric Dumazet, jpoimboe, Steven Rostedt On Wed, Jun 22, 2022 at 8:51 PM Stephane Eranian <eranian@google.com> wrote: > > Hi Peter, > > Thanks for taking a quick look at this. > I am currently OOO and I cannot test this proposed patch. > I am okay with your suggestion. > > Thanks. > > On Fri, Jun 17, 2022 at 4:52 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Fri, Jun 17, 2022 at 02:08:52PM +0300, Stephane Eranian wrote: > > > Hi, > > > > > > Some changes to the way invalid MSR accesses are reported by the kernel is > > > causing some problems with messages printed on the console. > > > > > > We have seen several cases of ex_handler_msr() printing invalid MSR > > > accesses once but > > > the callstack multiple times causing confusion on the console. > > > > > > The last time the exception MSR code was modified (5.16) by PeterZ was: > > > > > > d52a7344bdfa x86/msr: Remove .fixup usage: > > > > > > if (!safe && wrmsr && pr_warn_once("unchecked MSR access error: ...")) > > > show_stack_regs(regs); > > > > > > Note that this code pattern was also present, though in a different > > > form, before this commit. > > > > > > The problem here is that another earlier commit (5.13): > > > > > > a358f40600b3 once: implement DO_ONCE_LITE for non-fast-path "do once" > > > functionality > > > > > > Modifies all the pr_*_once() calls to always return true claiming that > > > no caller is ever > > > checking the return value of the functions. > > > > > > This is why we are seeing the callstack printed without the associated > > > printk() msg. > > > > > > I believe that having the pr_*_once() functions return true the first > > > time they are called > > > is useful especially when extra information, such as callstack, must > > > be printed to help > > > track the origin of the problem. > > > > > > The exception handling code seems to be the only place where the > > > return value is checked > > > for pr_warn_once(). A minimal change would be to create another > > > version of that function > > > that calls DO_ONCE() instead of DO_ONCE_LITE(), e.g., pr_warn_once_return(). > > > > > > I can post a patch to that effect if we all agree on the approach. > > > > > > Thanks. > > > > How about something like this? > > > > diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c > > index dba2197c05c3..331310c29349 100644 > > --- a/arch/x86/mm/extable.c > > +++ b/arch/x86/mm/extable.c > > @@ -94,16 +94,18 @@ static bool ex_handler_copy(const struct exception_table_entry *fixup, > > static bool ex_handler_msr(const struct exception_table_entry *fixup, > > struct pt_regs *regs, bool wrmsr, bool safe, int reg) > > { > > - if (!safe && wrmsr && > > - pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", > > - (unsigned int)regs->cx, (unsigned int)regs->dx, > > - (unsigned int)regs->ax, regs->ip, (void *)regs->ip)) > > + if (__ONCE_LITE_IF(!safe && wrmsr)) { > > + pr_warn("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", > > + (unsigned int)regs->cx, (unsigned int)regs->dx, > > + (unsigned int)regs->ax, regs->ip, (void *)regs->ip); > > show_stack_regs(regs); > > + } > > > > - if (!safe && !wrmsr && > > - pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", > > - (unsigned int)regs->cx, regs->ip, (void *)regs->ip)) > > + if (__ONCE_LITE_IF(!safe && !wrmsr)) { > > + pr_warn("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", > > + (unsigned int)regs->cx, regs->ip, (void *)regs->ip); > > show_stack_regs(regs); > > + } > > > > if (!wrmsr) { > > /* Pretend that the read succeeded and returned 0. */ > > diff --git a/include/linux/once_lite.h b/include/linux/once_lite.h > > index 861e606b820f..63c3bbcef694 100644 > > --- a/include/linux/once_lite.h > > +++ b/include/linux/once_lite.h > > @@ -9,15 +9,27 @@ > > */ > > #define DO_ONCE_LITE(func, ...) \ > > DO_ONCE_LITE_IF(true, func, ##__VA_ARGS__) > > -#define DO_ONCE_LITE_IF(condition, func, ...) \ > > + > > +#define __ONCE_LITE_IF(condition) \ > > ({ \ > > static bool __section(".data.once") __already_done; \ > > - bool __ret_do_once = !!(condition); \ > > + bool __ret_cond = !!(condition); \ > > + bool __ret_once = false; \ > > \ > > if (unlikely(__ret_do_once && !__already_done)) { \ You need to replace __ret_do_once with __ret_cond above and then it compiles and works. I have tested with a kernel module that reads and writes to an illegal MSR: unchecked MSR access error: RDMSR from 0x1234567 at rIP: 0xffffffffc00ec138 (rdpmc_intel+0x28/0x21d0 [rdpmc_test]) Call Trace: <TASK> rdpmc_bench_store+0x53/0x80 [rdpmc_test] kobj_attr_store+0xf/0x20 sysfs_kf_write+0x34/0x50 kernfs_fop_write_iter+0xfa/0x180 vfs_write+0x334/0x3d0 ksys_write+0x71/0xe0 __x64_sys_write+0x1b/0x20 do_syscall_64+0x44/0xa0 ? exc_page_fault+0x6e/0x110 entry_SYSCALL_64_after_hwframe+0x63/0xcd Tested-by: Stephane Eranian <eranian@google.com> > > > __already_done = true; \ > > - func(__VA_ARGS__); \ > > + __ret_once = true; \ > > } \ > > + unlikely(__ret_once); \ > > + }) > > + > > +#define DO_ONCE_LITE_IF(condition, func, ...) \ > > + ({ \ > > + bool __ret_do_once = !!(condition); \ > > + \ > > + if (__ONCE_LITE_IF(__ret_do_once)) \ > > + func(__VA_ARGS__); \ > > + \ > > unlikely(__ret_do_once); \ > > }) > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] x86/extable: Fix ex_handler_msr() print condition 2022-07-20 12:48 ` Stephane Eranian @ 2022-07-20 13:44 ` Peter Zijlstra 0 siblings, 0 replies; 6+ messages in thread From: Peter Zijlstra @ 2022-07-20 13:44 UTC (permalink / raw) To: Stephane Eranian Cc: LKML, Eric Dumazet, jpoimboe, Steven Rostedt, tannerlove, maheshb On Wed, Jul 20, 2022 at 03:48:56PM +0300, Stephane Eranian wrote: > Tested-by: Stephane Eranian <eranian@google.com> Thanks, I'll queue the below. --- Subject: x86/extable: Fix ex_handler_msr() print condition From: Peter Zijlstra <peterz@infradead.org> Date: Fri, 17 Jun 2022 16:52:06 +0200 On Fri, Jun 17, 2022 at 02:08:52PM +0300, Stephane Eranian wrote: > Some changes to the way invalid MSR accesses are reported by the > kernel is causing some problems with messages printed on the > console. > > We have seen several cases of ex_handler_msr() printing invalid MSR > accesses once but the callstack multiple times causing confusion on > the console. > The problem here is that another earlier commit (5.13): > > a358f40600b3 ("once: implement DO_ONCE_LITE for non-fast-path "do once" functionality") > > Modifies all the pr_*_once() calls to always return true claiming > that no caller is ever checking the return value of the functions. > > This is why we are seeing the callstack printed without the > associated printk() msg. Extract the ONCE_IF(cond) part into __ONCE_LTE_IF() and use that to implement DO_ONCE_LITE_IF() and fix the extable code. Fixes: a358f40600b3 ("once: implement DO_ONCE_LITE for non-fast-path "do once" functionality") Reported-by: Stephane Eranian <eranian@google.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Tested-by: Stephane Eranian <eranian@google.com> Link: https://lkml.kernel.org/r/YqyVFsbviKjVGGZ9@worktop.programming.kicks-ass.net --- arch/x86/mm/extable.c | 16 +++++++++------- include/linux/once_lite.h | 20 ++++++++++++++++---- 2 files changed, 25 insertions(+), 11 deletions(-) --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -94,16 +94,18 @@ static bool ex_handler_copy(const struct static bool ex_handler_msr(const struct exception_table_entry *fixup, struct pt_regs *regs, bool wrmsr, bool safe, int reg) { - if (!safe && wrmsr && - pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", - (unsigned int)regs->cx, (unsigned int)regs->dx, - (unsigned int)regs->ax, regs->ip, (void *)regs->ip)) + if (__ONCE_LITE_IF(!safe && wrmsr)) { + pr_warn("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", + (unsigned int)regs->cx, (unsigned int)regs->dx, + (unsigned int)regs->ax, regs->ip, (void *)regs->ip); show_stack_regs(regs); + } - if (!safe && !wrmsr && - pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", - (unsigned int)regs->cx, regs->ip, (void *)regs->ip)) + if (__ONCE_LITE_IF(!safe && !wrmsr)) { + pr_warn("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", + (unsigned int)regs->cx, regs->ip, (void *)regs->ip); show_stack_regs(regs); + } if (!wrmsr) { /* Pretend that the read succeeded and returned 0. */ --- a/include/linux/once_lite.h +++ b/include/linux/once_lite.h @@ -9,15 +9,27 @@ */ #define DO_ONCE_LITE(func, ...) \ DO_ONCE_LITE_IF(true, func, ##__VA_ARGS__) -#define DO_ONCE_LITE_IF(condition, func, ...) \ + +#define __ONCE_LITE_IF(condition) \ ({ \ static bool __section(".data.once") __already_done; \ - bool __ret_do_once = !!(condition); \ + bool __ret_cond = !!(condition); \ + bool __ret_once = false; \ \ - if (unlikely(__ret_do_once && !__already_done)) { \ + if (unlikely(__ret_cond && !__already_done)) { \ __already_done = true; \ - func(__VA_ARGS__); \ + __ret_once = true; \ } \ + unlikely(__ret_once); \ + }) + +#define DO_ONCE_LITE_IF(condition, func, ...) \ + ({ \ + bool __ret_do_once = !!(condition); \ + \ + if (__ONCE_LITE_IF(__ret_do_once)) \ + func(__VA_ARGS__); \ + \ unlikely(__ret_do_once); \ }) ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: x86/core] x86/extable: Fix ex_handler_msr() print condition 2022-06-17 14:52 ` Peter Zijlstra 2022-06-22 17:51 ` Stephane Eranian @ 2022-07-21 8:52 ` tip-bot2 for Peter Zijlstra 1 sibling, 0 replies; 6+ messages in thread From: tip-bot2 for Peter Zijlstra @ 2022-07-21 8:52 UTC (permalink / raw) To: linux-tip-commits Cc: Stephane Eranian, Peter Zijlstra (Intel), x86, linux-kernel The following commit has been merged into the x86/core branch of tip: Commit-ID: a1a5482a2c6e38a3ebed32e571625c56a8cc41a6 Gitweb: https://git.kernel.org/tip/a1a5482a2c6e38a3ebed32e571625c56a8cc41a6 Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Fri, 17 Jun 2022 16:52:06 +02:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Thu, 21 Jul 2022 10:39:42 +02:00 x86/extable: Fix ex_handler_msr() print condition On Fri, Jun 17, 2022 at 02:08:52PM +0300, Stephane Eranian wrote: > Some changes to the way invalid MSR accesses are reported by the > kernel is causing some problems with messages printed on the > console. > > We have seen several cases of ex_handler_msr() printing invalid MSR > accesses once but the callstack multiple times causing confusion on > the console. > The problem here is that another earlier commit (5.13): > > a358f40600b3 ("once: implement DO_ONCE_LITE for non-fast-path "do once" functionality") > > Modifies all the pr_*_once() calls to always return true claiming > that no caller is ever checking the return value of the functions. > > This is why we are seeing the callstack printed without the > associated printk() msg. Extract the ONCE_IF(cond) part into __ONCE_LTE_IF() and use that to implement DO_ONCE_LITE_IF() and fix the extable code. Fixes: a358f40600b3 ("once: implement DO_ONCE_LITE for non-fast-path "do once" functionality") Reported-by: Stephane Eranian <eranian@google.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Tested-by: Stephane Eranian <eranian@google.com> Link: https://lkml.kernel.org/r/YqyVFsbviKjVGGZ9@worktop.programming.kicks-ass.net --- arch/x86/mm/extable.c | 16 +++++++++------- include/linux/once_lite.h | 20 ++++++++++++++++---- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c index dba2197..331310c 100644 --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -94,16 +94,18 @@ static bool ex_handler_copy(const struct exception_table_entry *fixup, static bool ex_handler_msr(const struct exception_table_entry *fixup, struct pt_regs *regs, bool wrmsr, bool safe, int reg) { - if (!safe && wrmsr && - pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", - (unsigned int)regs->cx, (unsigned int)regs->dx, - (unsigned int)regs->ax, regs->ip, (void *)regs->ip)) + if (__ONCE_LITE_IF(!safe && wrmsr)) { + pr_warn("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", + (unsigned int)regs->cx, (unsigned int)regs->dx, + (unsigned int)regs->ax, regs->ip, (void *)regs->ip); show_stack_regs(regs); + } - if (!safe && !wrmsr && - pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", - (unsigned int)regs->cx, regs->ip, (void *)regs->ip)) + if (__ONCE_LITE_IF(!safe && !wrmsr)) { + pr_warn("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", + (unsigned int)regs->cx, regs->ip, (void *)regs->ip); show_stack_regs(regs); + } if (!wrmsr) { /* Pretend that the read succeeded and returned 0. */ diff --git a/include/linux/once_lite.h b/include/linux/once_lite.h index 861e606..b7bce49 100644 --- a/include/linux/once_lite.h +++ b/include/linux/once_lite.h @@ -9,15 +9,27 @@ */ #define DO_ONCE_LITE(func, ...) \ DO_ONCE_LITE_IF(true, func, ##__VA_ARGS__) -#define DO_ONCE_LITE_IF(condition, func, ...) \ + +#define __ONCE_LITE_IF(condition) \ ({ \ static bool __section(".data.once") __already_done; \ - bool __ret_do_once = !!(condition); \ + bool __ret_cond = !!(condition); \ + bool __ret_once = false; \ \ - if (unlikely(__ret_do_once && !__already_done)) { \ + if (unlikely(__ret_cond && !__already_done)) { \ __already_done = true; \ - func(__VA_ARGS__); \ + __ret_once = true; \ } \ + unlikely(__ret_once); \ + }) + +#define DO_ONCE_LITE_IF(condition, func, ...) \ + ({ \ + bool __ret_do_once = !!(condition); \ + \ + if (__ONCE_LITE_IF(__ret_do_once)) \ + func(__VA_ARGS__); \ + \ unlikely(__ret_do_once); \ }) ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-21 8:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-17 11:08 [RFC] pr_warn_once() issue in x86 MSR extable code Stephane Eranian 2022-06-17 14:52 ` Peter Zijlstra 2022-06-22 17:51 ` Stephane Eranian 2022-07-20 12:48 ` Stephane Eranian 2022-07-20 13:44 ` [PATCH] x86/extable: Fix ex_handler_msr() print condition Peter Zijlstra 2022-07-21 8:52 ` [tip: x86/core] " tip-bot2 for Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).