[RFC PATCH] Unexport do_machine_check() and machine_check_poll()

[RFC PATCH] Unexport do_machine_check() and machine_check_poll()

[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.

Thoughts?

---
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 2ea4527e462f..39c362168aba 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -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 */
diff --git a/arch/x86/kernel/cpu/mcheck/mce-inject.c b/arch/x86/kernel/cpu/mcheck/mce-inject.c
index 4cfba4371a71..e3b896b836c7 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-inject.c
+++ b/arch/x86/kernel/cpu/mcheck/mce-inject.c
@@ -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;
 }
diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
index 547720efd923..6bae2bc2b21f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
@@ -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);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index a006f4cd792b..c39a8c4f8a42 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -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)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e2951b6edbbc..8fdbb950416d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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
 }
 
diff --git a/arch/x86/ras/mce_amd_inj.c b/arch/x86/ras/mce_amd_inj.c
index 55d38cfa46c2..dbfb3e4c3440 100644
--- a/arch/x86/ras/mce_amd_inj.c
+++ b/arch/x86/ras/mce_amd_inj.c
@@ -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;
 	}
 

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH] Unexport do_machine_check() and machine_check_poll()

[Borislav Petkov]

On Mon, Mar 14, 2016 at 05:38:54PM +0100, Borislav Petkov wrote:
> Hey Tony,
> 
> how about the below, untested change?

I meant this one, which actually builds:

---
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 92b6f651fa4f..6d0845c8a025 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -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 */
diff --git a/arch/x86/kernel/cpu/mcheck/mce-inject.c b/arch/x86/kernel/cpu/mcheck/mce-inject.c
index 517619ea6498..d6137320f773 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-inject.c
+++ b/arch/x86/kernel/cpu/mcheck/mce-inject.c
@@ -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;
 }
diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
index 547720efd923..6bae2bc2b21f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
@@ -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);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index f0c921b03e42..32fa4549525f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -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)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b92094ee135e..dd308d916fe6 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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
 }
 
diff --git a/arch/x86/ras/mce_amd_inj.c b/arch/x86/ras/mce_amd_inj.c
index 55d38cfa46c2..dbfb3e4c3440 100644
--- a/arch/x86/ras/mce_amd_inj.c
+++ b/arch/x86/ras/mce_amd_inj.c
@@ -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;
 	}

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH] Unexport do_machine_check() and machine_check_poll()

[Luck, Tony]

On Mon, Mar 14, 2016 at 05:38:54PM +0100, Borislav Petkov wrote:
> 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

Re: [RFC PATCH] Unexport do_machine_check() and machine_check_poll()

[Borislav Petkov]

On Mon, Mar 14, 2016 at 09:55:17AM -0700, Luck, Tony wrote:
> 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.

> 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.

> 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...

-- 
Regards/Gruss,
    Boris.

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

RE: [RFC PATCH] Unexport do_machine_check() and machine_check_poll()

[Luck, Tony]

> 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

Re: [RFC PATCH] Unexport do_machine_check() and machine_check_poll()

[Borislav Petkov]

On Mon, Mar 14, 2016 at 06:24:23PM +0000, Luck, Tony wrote:
> > 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.

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!

-- 
Regards/Gruss,
    Boris.

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