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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ