Skip to contentSkip to conversation

Unexport do_machine_check() and machine_check_poll()

RFC PATCH In review Borislav Petkov 2 participants 6 messages Mar 14, 2016

Subsystems (2)
x86 KVM x86
lore
Share & tools
lore

Borislav Petkov

author maintainer
#0 lore raw

RFC PATCH 1/1 Unexport do_machine_check() and machine_check_poll()

Hey Tony,

how about the below, untested change?

Some backporting work to SLE11 got me pondering over why we're exporting all those MCA-internal things to modules. Modules don't have any business calling those so how about hiding them behind a single point mce_call() function which gets a command what to do? This way, we're free to change stuff later too, if we decide to do so.

Thoughts?

---

arch/x86/include/asm/mce.h +8 -2
index 2ea4527e462f..39c362168aba 100644
@@ -224,7 +224,6 @@ enum mcp_flags {
 	MCP_UC		= BIT(1),	/* log uncorrected errors */
 	MCP_DONTLOG	= BIT(2),	/* only clear, don't log */
 };
-bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
 
 int mce_notify_irq(void);
 

@@ -243,7 +242,6 @@ extern void mce_disable_bank(int bank);
 
 /* Call the installed machine check handler for this CPU setup. */
 extern void (*machine_check_vector)(struct pt_regs *, long error_code);
-void do_machine_check(struct pt_regs *, long);
 
 /*
  * Threshold handler

@@ -287,4 +285,12 @@ struct cper_sec_mem_err;
 extern void apei_mce_report_mem_error(int corrected,
 				      struct cper_sec_mem_err *mem_err);
 
+enum mce_call_cmds = {
+	MCE_CALL_DECODE,	/* A struct mce for decoding is being supplied. */
+	MCE_CALL_MC,		/* Invoke #MC exception handler. */
+	MCE_CALL_POLL,		/* Poll MCA banks. */
+};
+
+void mce_call(enum mce_call_cmds cmd, struct mce *m);
+
 #endif /* _ASM_X86_MCE_H */
arch/x86/kernel/cpu/mcheck/mce-inject.c +2 -5
index 4cfba4371a71..e3b896b836c7 100644
@@ -49,13 +49,10 @@ static void inject_mce(struct mce *m)
 
 static void raise_poll(struct mce *m)
 {
-	unsigned long flags;
 	mce_banks_t b;
 
 	memset(&b, 0xff, sizeof(mce_banks_t));
-	local_irq_save(flags);
-	machine_check_poll(0, &b);
-	local_irq_restore(flags);
+	mce_call(MCE_CALL_POLL, &b);
 	m->finished = 0;
 }
 

@@ -72,7 +69,7 @@ static void raise_exception(struct mce *m, struct pt_regs *pregs)
 	}
 	/* in mcheck exeception handler, irq will be disabled */
 	local_irq_save(flags);
-	do_machine_check(pregs, 0);
+	mce_call(MCE_CALL_MC, pregs);
 	local_irq_restore(flags);
 	m->finished = 0;
 }
arch/x86/kernel/cpu/mcheck/mce-internal.h +2 -2
index 547720efd923..6bae2bc2b21f 100644
@@ -13,6 +13,8 @@ enum severity_level {
 	MCE_PANIC_SEVERITY,
 };
 
+void do_machine_check(struct pt_regs *, long);
+bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
 extern struct atomic_notifier_head x86_mce_decoder_chain;
 
 #define ATTR_LEN		16

@@ -79,5 +81,3 @@ static inline int apei_clear_mce(u64 record_id)
 	return -EINVAL;
 }
 #endif
-
-void mce_inject_log(struct mce *m);
arch/x86/kernel/cpu/mcheck/mce.c +26 -7
index a006f4cd792b..c39a8c4f8a42 100644
@@ -198,13 +198,34 @@ void mce_log(struct mce *mce)
 	set_bit(0, &mce_need_notify);
 }
 
-void mce_inject_log(struct mce *m)
+void mce_call(enum mce_call_cmds cmd, void *t)
 {
-	mutex_lock(&mce_chrdev_read_mutex);
-	mce_log(m);
-	mutex_unlock(&mce_chrdev_read_mutex);
+	switch (cmd) {
+	case MCE_CALL_DECODE:
+		mutex_lock(&mce_chrdev_read_mutex);
+		mce_log((struct mce *)t);
+		mutex_unlock(&mce_chrdev_read_mutex);
+		break;
+
+	case MCE_CALL_MC:
+		do_machine_check((struct pt_regs *)t, 0);
+		break;
+
+	case MCE_CALL_POLL: {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		machine_check_poll(0, (mce_flags_t *)t);
+		local_irq_restore(flags);
+		}
+		break;
+
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
 }
-EXPORT_SYMBOL_GPL(mce_inject_log);
+EXPORT_SYMBOL_GPL(mce_call);
 
 static struct notifier_block mce_srao_nb;
 

@@ -666,7 +687,6 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 
 	return error_seen;
 }
-EXPORT_SYMBOL_GPL(machine_check_poll);
 
 /*
  * Do a quick check if any of the events requires a panic.

@@ -1180,7 +1200,6 @@ out:
 done:
 	ist_exit(regs);
 }
-EXPORT_SYMBOL_GPL(do_machine_check);
 
 #ifndef CONFIG_MEMORY_FAILURE
 int memory_failure(unsigned long pfn, int vector, int flags)
arch/x86/kvm/vmx.c +1 -1
index e2951b6edbbc..8fdbb950416d 100644
@@ -5247,7 +5247,7 @@ static void kvm_machine_check(void)
 		.flags = X86_EFLAGS_IF,
 	};
 
-	do_machine_check(&regs, 0);
+	mce_call(MCE_CALL_MC, &regs);
 #endif
 }
arch/x86/ras/mce_amd_inj.c +1 -1
index 55d38cfa46c2..dbfb3e4c3440 100644
@@ -250,7 +250,7 @@ static void do_inject(void)
 		i_mce.status |= MCI_STATUS_MISCV;
 
 	if (inj_type == SW_INJ) {
-		mce_inject_log(&i_mce);
+		mce_call(MCE_CALL_DECODE, &i_mce);
 		return;
 	}
Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
author maintainer
#1 lore raw
Quoted 3 lines · Borislav Petkov

Hey Tony,

how about the below, untested change?

I meant this one, which actually builds:

---

arch/x86/include/asm/mce.h +7 -2
index 92b6f651fa4f..6d0845c8a025 100644
@@ -247,7 +247,6 @@ enum mcp_flags {
 	MCP_UC		= BIT(1),	/* log uncorrected errors */
 	MCP_DONTLOG	= BIT(2),	/* only clear, don't log */
 };
-bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
 
 int mce_notify_irq(void);
 

@@ -266,7 +265,6 @@ extern void mce_disable_bank(int bank);
 
 /* Call the installed machine check handler for this CPU setup. */
 extern void (*machine_check_vector)(struct pt_regs *, long error_code);
-void do_machine_check(struct pt_regs *, long);
 
 /*
  * Threshold handler

@@ -355,4 +353,11 @@ enum amd_df_mca_blocks {
 extern const char * const amd_df_mcablock_names[N_DF_BLOCKS];
 #endif
 
+enum mce_call_cmds {
+	MCE_CALL_DECODE,	/* A struct mce for decoding is being supplied. */
+	MCE_CALL_MC,		/* Invoke #MC exception handler. */
+	MCE_CALL_POLL		/* Poll MCA banks. */
+};
+
+void mce_call(enum mce_call_cmds cmd, void *t);
 #endif /* _ASM_X86_MCE_H */
arch/x86/kernel/cpu/mcheck/mce-inject.c +2 -5
index 517619ea6498..d6137320f773 100644
@@ -49,13 +49,10 @@ static void inject_mce(struct mce *m)
 
 static void raise_poll(struct mce *m)
 {
-	unsigned long flags;
 	mce_banks_t b;
 
 	memset(&b, 0xff, sizeof(mce_banks_t));
-	local_irq_save(flags);
-	machine_check_poll(0, &b);
-	local_irq_restore(flags);
+	mce_call(MCE_CALL_POLL, &b);
 	m->finished = 0;
 }
 

@@ -72,7 +69,7 @@ static void raise_exception(struct mce *m, struct pt_regs *pregs)
 	}
 	/* in mcheck exeception handler, irq will be disabled */
 	local_irq_save(flags);
-	do_machine_check(pregs, 0);
+	mce_call(MCE_CALL_MC, pregs);
 	local_irq_restore(flags);
 	m->finished = 0;
 }
arch/x86/kernel/cpu/mcheck/mce-internal.h +2 -2
index 547720efd923..6bae2bc2b21f 100644
@@ -13,6 +13,8 @@ enum severity_level {
 	MCE_PANIC_SEVERITY,
 };
 
+void do_machine_check(struct pt_regs *, long);
+bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
 extern struct atomic_notifier_head x86_mce_decoder_chain;
 
 #define ATTR_LEN		16

@@ -79,5 +81,3 @@ static inline int apei_clear_mce(u64 record_id)
 	return -EINVAL;
 }
 #endif
-
-void mce_inject_log(struct mce *m);
arch/x86/kernel/cpu/mcheck/mce.c +26 -7
index f0c921b03e42..32fa4549525f 100644
@@ -198,13 +198,34 @@ void mce_log(struct mce *mce)
 	set_bit(0, &mce_need_notify);
 }
 
-void mce_inject_log(struct mce *m)
+void mce_call(enum mce_call_cmds cmd, void *t)
 {
-	mutex_lock(&mce_chrdev_read_mutex);
-	mce_log(m);
-	mutex_unlock(&mce_chrdev_read_mutex);
+	switch (cmd) {
+	case MCE_CALL_DECODE:
+		mutex_lock(&mce_chrdev_read_mutex);
+		mce_log((struct mce *)t);
+		mutex_unlock(&mce_chrdev_read_mutex);
+		break;
+
+	case MCE_CALL_MC:
+		do_machine_check((struct pt_regs *)t, 0);
+		break;
+
+	case MCE_CALL_POLL: {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		machine_check_poll(0, (mce_banks_t *)t);
+		local_irq_restore(flags);
+		}
+		break;
+
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
 }
-EXPORT_SYMBOL_GPL(mce_inject_log);
+EXPORT_SYMBOL_GPL(mce_call);
 
 static struct notifier_block mce_srao_nb;
 

@@ -666,7 +687,6 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 
 	return error_seen;
 }
-EXPORT_SYMBOL_GPL(machine_check_poll);
 
 /*
  * Do a quick check if any of the events requires a panic.

@@ -1182,7 +1202,6 @@ out:
 out_ist:
 	ist_exit(regs);
 }
-EXPORT_SYMBOL_GPL(do_machine_check);
 
 #ifndef CONFIG_MEMORY_FAILURE
 int memory_failure(unsigned long pfn, int vector, int flags)
arch/x86/kvm/vmx.c +1 -1
index b92094ee135e..dd308d916fe6 100644
@@ -5251,7 +5251,7 @@ static void kvm_machine_check(void)
 		.flags = X86_EFLAGS_IF,
 	};
 
-	do_machine_check(&regs, 0);
+	mce_call(MCE_CALL_MC, &regs);
 #endif
 }
arch/x86/ras/mce_amd_inj.c +1 -1
index 55d38cfa46c2..dbfb3e4c3440 100644
@@ -250,7 +250,7 @@ static void do_inject(void)
 		i_mce.status |= MCI_STATUS_MISCV;
 
 	if (inj_type == SW_INJ) {
-		mce_inject_log(&i_mce);
+		mce_call(MCE_CALL_DECODE, &i_mce);
 		return;
 	}
Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
#2 lore raw
In reply to Borislav Petkov
Quoted 9 lines · Borislav Petkov

Hey Tony,

how about the below, untested change?

Some backporting work to SLE11 got me pondering over why we're exporting all those MCA-internal things to modules. Modules don't have any business calling those so how about hiding them behind a single point mce_call() function which gets a command what to do? This way, we're free to change stuff later too, if we decide to do so.

It doesn't seem like a very natural fit ... the three routines take very different arguments which you bundle into a "void *".

I'm also not sure what we gain. Now we have one, complicated, exported function that still lets modules do all the things they could do with the three separate functions. Is there some benefit to having fewer exports?

What am I missing?

-Tony

author maintainer
#3 lore raw
Quoted 2 lines · Tony

It doesn't seem like a very natural fit ... the three routines take very different arguments which you bundle into a "void *".

Yeah, that was the quick'n'dirty approach.

Quoted 4 lines

I'm also not sure what we gain. Now we have one, complicated, exported function that still lets modules do all the things they could do with the three separate functions. Is there some benefit to having fewer exports?

So the small benefit is that our ABI has a single function instead of two.

Quoted 1 line

What am I missing?

Yeah, you're right the patch is probably not the right thing to do.

But the sentiment is: I want to unexport do_machine_check() and machine_check_poll() and not let external modules call into them directly. Why, you ask? Because they have no business doing that. Those two are MCA, more-or-less, internal functionality and it probably is ok if mce-inject or kvm/vmx use them but the export is IMO too wide. Think out-of-tree modules and whatnot here.

So I guess exporting even mce_call() is wrong - I'd like to not export anything to users and allow only the existing two mce-inject and kvm/vmx call them.

But the original reason why I started looking at those is that during a backport to SLE11, I had a kABI issue due to machine_check_poll() and I started questioning why is that function even exported? And it shouldn't be - internal kernel users should be able to get that functionality in a different way, without the wide export. I probably should think a bit more about how.

I hope I'm making a bit more sense now...

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
#4 lore raw
Quoted 3 lines

But the sentiment is: I want to unexport do_machine_check() and machine_check_poll() and not let external modules call into them directly. Why, you ask? Because they have no business doing that.

EXPORT is a big hammer ... we either let every module have access to a function, or none. It sounds like you want a way to just export to a few trusted friendly areas that have a real need to access, and make this invisible to everyone else.

I can't imagine a way to absolutely enforce that ... whatever mechanism you choose could be abused by someone willing to have their module lie and say "sure, I'm a KVM user that is allowed to use that".

Perhaps there's a way to implement an advisory scheme ... which would make it blatantly obvious when modules are hooking into things that they shouldn't. The only similar thing we have now is EXPORT_GPL which doesn't look scalable to lots of uses.

-Tony

author maintainer
#5 lore raw
Quoted 9 lines · Tony
Quoted 3 lines

But the sentiment is: I want to unexport do_machine_check() and machine_check_poll() and not let external modules call into them directly. Why, you ask? Because they have no business doing that.

EXPORT is a big hammer ... we either let every module have access to a function, or none. It sounds like you want a way to just export to a few trusted friendly areas that have a real need to access, and make this invisible to everyone else.

Quoted 8 lines

I can't imagine a way to absolutely enforce that ... whatever mechanism you choose could be abused by someone willing to have their module lie and say "sure, I'm a KVM user that is allowed to use that".

Perhaps there's a way to implement an advisory scheme ... which would make it blatantly obvious when modules are hooking into things that they shouldn't. The only similar thing we have now is EXPORT_GPL which doesn't look scalable to lots of uses.

Yap. Exactly.

Right, so the optimal thing to do IMHO is make those two users solve their needs differently, so that there's no requirement for exports at all. And the fact that both are modules - and they need to be modules for obvious reasons - doesn't make it any easier.

I can't think of something useful right now though... I'll need to ponder on it a while longer.

Thanks for listening!

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.