linux-kernel - [PATCH v2 6/6] x86/mce: Dynamically register default MCE handler

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: <20200103150722.20313-7-jschoenh@amazon.de>
Date:   Fri,  3 Jan 2020 16:07:22 +0100
From:   Jan H. Schönherr <jschoenh@...zon.de>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Jan H. Schönherr <jschoenh@...zon.de>,
        Yazen Ghannam <yazen.ghannam@....com>,
        linux-kernel@...r.kernel.org, linux-edac@...r.kernel.org,
        Tony Luck <tony.luck@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org
Subject: [PATCH v2 6/6] x86/mce: Dynamically register default MCE handler

The default MCE handler takes action, when no external MCE handler is
registered. Instead of checking for external handlers within the default
MCE handler, only register the default MCE handler when there are no
external handlers in the first place.

Signed-off-by: Jan H. Schönherr <jschoenh@...zon.de>
---
New in v2. This is something that became possible due to patch 4.
I'm not entirely happy with it.

One the one hand, I'm wondering whether there's a nicer way to handle
(de-)registration races.

On the other hand, I'm starting to question the whole logic to "only print
the MCE if nothing else in the kernel has a handler registered". Is that
really how it should be? For example, there are handlers that filter for a
specific subset of MCEs. If one of those is registered, we're losing all
information for MCEs that don't match.

A possible solution to the latter would be to have a "handled" or "printed"
flag within "struct mce" and print the MCE based on that in the default
handler. What do you think?
---
 arch/x86/kernel/cpu/mce/core.c | 90 ++++++++++++++++++++--------------
 1 file changed, 52 insertions(+), 38 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index d8fe5b048ee7..3b6e37c5252f 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -156,36 +156,6 @@ void mce_log(struct mce *m)
 }
 EXPORT_SYMBOL_GPL(mce_log);
 
-/*
- * We run the default notifier as long as we have no external
- * notifiers registered on the chain.
- */
-static atomic_t num_notifiers;
-
-static void mce_register_decode_chain_internal(struct notifier_block *nb)
-{
-	if (WARN_ON(nb->priority > MCE_PRIO_MCELOG && nb->priority < MCE_PRIO_EDAC))
-		return;
-
-	blocking_notifier_chain_register(&x86_mce_decoder_chain, nb);
-}
-
-void mce_register_decode_chain(struct notifier_block *nb)
-{
-	atomic_inc(&num_notifiers);
-
-	mce_register_decode_chain_internal(nb);
-}
-EXPORT_SYMBOL_GPL(mce_register_decode_chain);
-
-void mce_unregister_decode_chain(struct notifier_block *nb)
-{
-	atomic_dec(&num_notifiers);
-
-	blocking_notifier_chain_unregister(&x86_mce_decoder_chain, nb);
-}
-EXPORT_SYMBOL_GPL(mce_unregister_decode_chain);
-
 static inline u32 ctl_reg(int bank)
 {
 	return MSR_IA32_MCx_CTL(bank);
@@ -606,18 +576,19 @@ static struct notifier_block mce_uc_nb = {
 	.priority	= MCE_PRIO_UC,
 };
 
+/*
+ * We run the default notifier as long as we have no external
+ * notifiers registered on the chain.
+ */
+static atomic_t num_notifiers;
+
 static int mce_default_notifier(struct notifier_block *nb, unsigned long val,
 				void *data)
 {
 	struct mce *m = (struct mce *)data;
 
-	if (!m)
-		return NOTIFY_DONE;
-
-	if (atomic_read(&num_notifiers))
-		return NOTIFY_DONE;
-
-	__print_mce(m);
+	if (m)
+		__print_mce(m);
 
 	return NOTIFY_DONE;
 }
@@ -628,6 +599,49 @@ static struct notifier_block mce_default_nb = {
 	.priority	= MCE_PRIO_LOWEST,
 };
 
+static void update_default_notifier_registration(void)
+{
+	bool has_notifiers = !!atomic_read(&num_notifiers);
+
+retry:
+	if (has_notifiers)
+		blocking_notifier_chain_unregister(&x86_mce_decoder_chain,
+						   &mce_default_nb);
+	else
+		blocking_notifier_chain_cond_register(&x86_mce_decoder_chain,
+						      &mce_default_nb);
+
+	if (has_notifiers != !!atomic_read(&num_notifiers)) {
+		has_notifiers = !has_notifiers;
+		goto retry;
+	}
+}
+
+static void mce_register_decode_chain_internal(struct notifier_block *nb)
+{
+	if (WARN_ON(nb->priority > MCE_PRIO_MCELOG &&
+		    nb->priority < MCE_PRIO_EDAC))
+		return;
+
+	blocking_notifier_chain_register(&x86_mce_decoder_chain, nb);
+}
+
+void mce_register_decode_chain(struct notifier_block *nb)
+{
+	atomic_inc(&num_notifiers);
+	mce_register_decode_chain_internal(nb);
+	update_default_notifier_registration();
+}
+EXPORT_SYMBOL_GPL(mce_register_decode_chain);
+
+void mce_unregister_decode_chain(struct notifier_block *nb)
+{
+	atomic_dec(&num_notifiers);
+	update_default_notifier_registration();
+	blocking_notifier_chain_unregister(&x86_mce_decoder_chain, nb);
+}
+EXPORT_SYMBOL_GPL(mce_unregister_decode_chain);
+
 /*
  * Read ADDR and MISC registers.
  */
@@ -1972,7 +1986,7 @@ int __init mcheck_init(void)
 	mce_register_decode_chain_internal(&first_nb);
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
 		mce_register_decode_chain_internal(&mce_uc_nb);
-	mce_register_decode_chain_internal(&mce_default_nb);
+	update_default_notifier_registration();
 	mcheck_vendor_init_severity();
 
 	INIT_WORK(&mce_work, mce_gen_pool_process);
-- 
2.22.0.3.gb49bb57c8208.dirty