| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CY8PR11MB71347B5215509D3B58258DCE894B2@CY8PR11MB7134.namprd11.prod.outlook.com>
Date: Tue, 29 Oct 2024 03:32:00 +0000
From: "Zhuo, Qiuxu" <qiuxu.zhuo@...el.com>
To: Borislav Petkov <bp@...en8.de>
CC: "Luck, Tony" <tony.luck@...el.com>, "tglx@...utronix.de"
<tglx@...utronix.de>, "dave.hansen@...ux.intel.com"
<dave.hansen@...ux.intel.com>, "mingo@...hat.com" <mingo@...hat.com>,
"hpa@...or.com" <hpa@...or.com>, "x86@...nel.org" <x86@...nel.org>,
"linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3 03/10] x86/mce: Make several functions return bool and
rename a function
Hi Boris,
Thanks for your time reviewing the series.
> From: Borislav Petkov <bp@...en8.de>
> [...]
> On Fri, Oct 25, 2024 at 10:45:55AM +0800, Qiuxu Zhuo wrote:
> > @@ -1748,7 +1748,7 @@ static void mce_timer_delete_all(void)
> > * Can be called from interrupt context, but not from machine check/NMI
> > * context.
> > */
> > -int mce_notify_irq(void)
> > +bool mce_notify_user(void)
>
> So why are you not fixing the comment above it too then?
>
> Have you audited all the code to make sure this function is not called from
> IRQ context anymore?
Currently this function is called from either interrupt context (timer) or
process context (notifier chain).
> Did you do git archeology to find out why it was called "_irq" at all in the first
Thanks for reminding me this.
Commit
9ff36ee9668f ("x86, mce: rename mce_notify_user to mce_notify_irq")
renamed mce_notify_user() to mce_notify_irq() to indicate that this function
should only be called from interrupt context and not used in machine check
or NMI context.
> place and why is it ok to change the name and adjust the comment above it
> now?
At first glance, the function name mce_notify_irq() it looks like "MCE notifies IRQ ...",
which is confusing and doesn't clearly reflect what it does. After the "git archeology"
above and offline communication from Andi (the commit author), it was clarified
that the suffix '_irq' was only used to indicate that the function can only be used in
interrupt context.
But I think the comments above the function clearly indicates which types of context
it can be used in, so it doesn't need the suffix '_irq' in the function name. Renaming it
back to mce_notify_user() can better reflect its function of notifying the user(s) about
the new machine check events.
> In any case, this change needs to be a separate patch along with all the
> explanations why is it ok to rename it.
OK.
I'll sperate this patch in the next version.
How about the following diff?
From f33f7340a40d3db1f4acf3c9ded574d1b9801fa7 Mon Sep 17 00:00:00 2001
From: Qiuxu Zhuo <qiuxu.zhuo@...el.com>
Date: Tue, 29 Oct 2024 09:07:47 +0800
Subject: [PATCH 1/1] x86/mce: Rename mce_notify_irq() back to
mce_notify_user()
Commit
9ff36ee9668f ("x86, mce: rename mce_notify_user to mce_notify_irq")
renamed mce_notify_user() to mce_notify_irq() to indicate that this
function should only be called from interrupt context and not used in
machine check or NMI context. However, the function name mce_notify_irq()
is confusing and doesn't clearly reflect what it does.
The comments above the function clearly indicates which types of context
it can be used in, so it doesn't need the suffix '_irq' in the function
name. Rename it back to mce_notify_user() to better reflect its function
of notifying the user(s) about the new machine check events and adjust the
comment to indicate that it can also be called from process context.
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@...el.com>
---
arch/x86/include/asm/mce.h | 2 +-
arch/x86/kernel/cpu/mce/core.c | 10 +++++-----
arch/x86/kernel/cpu/mce/inject.c | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 7a01bb5b19d3..dd9effd7c8b5 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -264,7 +264,7 @@ enum mcp_flags {
void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
-bool mce_notify_irq(void);
+bool mce_notify_user(void);
DECLARE_PER_CPU(struct mce, injectm);
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 725c1d6fb1e5..5f42ab2ea944 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -584,7 +584,7 @@ static int mce_early_notifier(struct notifier_block *nb, unsigned long val,
set_bit(0, &mce_need_notify);
- mce_notify_irq();
+ mce_notify_user();
return NOTIFY_DONE;
}
@@ -1704,7 +1704,7 @@ static void mce_timer_fn(struct timer_list *t)
* Alert userspace if needed. If we logged an MCE, reduce the polling
* interval, otherwise increase the polling interval.
*/
- if (mce_notify_irq())
+ if (mce_notify_user())
iv = max(iv / 2, (unsigned long) HZ/100);
else
iv = min(iv * 2, round_jiffies_relative(check_interval * HZ));
@@ -1745,10 +1745,10 @@ static void mce_timer_delete_all(void)
/*
* Notify the user(s) about new machine check events.
- * Can be called from interrupt context, but not from machine check/NMI
+ * Can be called from process/interrupt context, but not from machine check/NMI
* context.
*/
-bool mce_notify_irq(void)
+bool mce_notify_user(void)
{
/* Not more than two messages every minute */
static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
@@ -1763,7 +1763,7 @@ bool mce_notify_irq(void)
}
return false;
}
-EXPORT_SYMBOL_GPL(mce_notify_irq);
+EXPORT_SYMBOL_GPL(mce_notify_user);
static void __mcheck_cpu_mce_banks_init(void)
{
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 49ed3428785d..f5a7dae385c6 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -229,7 +229,7 @@ static int raise_local(void)
} else if (m->status) {
pr_info("Starting machine check poll CPU %d\n", cpu);
raise_poll(m);
- mce_notify_irq();
+ mce_notify_user();
pr_info("Machine check poll done on CPU %d\n", cpu);
} else
m->finished = 0;
--
2.17.1
Powered by blists - more mailing lists