* [PATCH v3 1/5] x86/paravirt: Add _safe to the read_msr and write_msr PV hooks
[not found] <cover.1457723023.git.luto@kernel.org>
@ 2016-03-11 19:06 ` Andy Lutomirski
2016-03-11 19:06 ` [PATCH v3 2/5] x86/msr: Carry on after a non-"safe" MSR access fails without !panic_on_oops Andy Lutomirski
` (5 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Andy Lutomirski @ 2016-03-11 19:06 UTC (permalink / raw)
To: X86 ML
Cc: KVM list, Peter Zijlstra, Linus Torvalds, linux-kernel,
xen-devel, Andy Lutomirski, Paolo Bonzini, Andrew Morton,
Arjan van de Ven
These hooks match the _safe variants, so name them accordingly.
This will make room for unsafe PV hooks.
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/include/asm/paravirt.h | 33 +++++++++++++++++----------------
arch/x86/include/asm/paravirt_types.h | 8 ++++----
arch/x86/kernel/paravirt.c | 4 ++--
arch/x86/xen/enlighten.c | 4 ++--
4 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index f6192502149e..2e49228ed9a3 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -129,34 +129,35 @@ static inline void wbinvd(void)
#define get_kernel_rpl() (pv_info.kernel_rpl)
-static inline u64 paravirt_read_msr(unsigned msr, int *err)
+static inline u64 paravirt_read_msr_safe(unsigned msr, int *err)
{
- return PVOP_CALL2(u64, pv_cpu_ops.read_msr, msr, err);
+ return PVOP_CALL2(u64, pv_cpu_ops.read_msr_safe, msr, err);
}
-static inline int paravirt_write_msr(unsigned msr, unsigned low, unsigned high)
+static inline int paravirt_write_msr_safe(unsigned msr,
+ unsigned low, unsigned high)
{
- return PVOP_CALL3(int, pv_cpu_ops.write_msr, msr, low, high);
+ return PVOP_CALL3(int, pv_cpu_ops.write_msr_safe, msr, low, high);
}
/* These should all do BUG_ON(_err), but our headers are too tangled. */
#define rdmsr(msr, val1, val2) \
do { \
int _err; \
- u64 _l = paravirt_read_msr(msr, &_err); \
+ u64 _l = paravirt_read_msr_safe(msr, &_err); \
val1 = (u32)_l; \
val2 = _l >> 32; \
} while (0)
#define wrmsr(msr, val1, val2) \
do { \
- paravirt_write_msr(msr, val1, val2); \
+ paravirt_write_msr_safe(msr, val1, val2); \
} while (0)
#define rdmsrl(msr, val) \
do { \
int _err; \
- val = paravirt_read_msr(msr, &_err); \
+ val = paravirt_read_msr_safe(msr, &_err); \
} while (0)
static inline void wrmsrl(unsigned msr, u64 val)
@@ -164,23 +165,23 @@ static inline void wrmsrl(unsigned msr, u64 val)
wrmsr(msr, (u32)val, (u32)(val>>32));
}
-#define wrmsr_safe(msr, a, b) paravirt_write_msr(msr, a, b)
+#define wrmsr_safe(msr, a, b) paravirt_write_msr_safe(msr, a, b)
/* rdmsr with exception handling */
-#define rdmsr_safe(msr, a, b) \
-({ \
- int _err; \
- u64 _l = paravirt_read_msr(msr, &_err); \
- (*a) = (u32)_l; \
- (*b) = _l >> 32; \
- _err; \
+#define rdmsr_safe(msr, a, b) \
+({ \
+ int _err; \
+ u64 _l = paravirt_read_msr_safe(msr, &_err); \
+ (*a) = (u32)_l; \
+ (*b) = _l >> 32; \
+ _err; \
})
static inline int rdmsrl_safe(unsigned msr, unsigned long long *p)
{
int err;
- *p = paravirt_read_msr(msr, &err);
+ *p = paravirt_read_msr_safe(msr, &err);
return err;
}
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 77db5616a473..5a06cccd36f0 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -155,10 +155,10 @@ struct pv_cpu_ops {
void (*cpuid)(unsigned int *eax, unsigned int *ebx,
unsigned int *ecx, unsigned int *edx);
- /* MSR, PMC and TSR operations.
- err = 0/-EFAULT. wrmsr returns 0/-EFAULT. */
- u64 (*read_msr)(unsigned int msr, int *err);
- int (*write_msr)(unsigned int msr, unsigned low, unsigned high);
+ /* MSR operations.
+ err = 0/-EIO. wrmsr returns 0/-EIO. */
+ u64 (*read_msr_safe)(unsigned int msr, int *err);
+ int (*write_msr_safe)(unsigned int msr, unsigned low, unsigned high);
u64 (*read_pmc)(int counter);
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index f08ac28b8136..8aad95478ae5 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -339,8 +339,8 @@ __visible struct pv_cpu_ops pv_cpu_ops = {
.write_cr8 = native_write_cr8,
#endif
.wbinvd = native_wbinvd,
- .read_msr = native_read_msr_safe,
- .write_msr = native_write_msr_safe,
+ .read_msr_safe = native_read_msr_safe,
+ .write_msr_safe = native_write_msr_safe,
.read_pmc = native_read_pmc,
.load_tr_desc = native_load_tr_desc,
.set_ldt = native_set_ldt,
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index d09e4c9d7cc5..ff2df20d0067 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1222,8 +1222,8 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
.wbinvd = native_wbinvd,
- .read_msr = xen_read_msr_safe,
- .write_msr = xen_write_msr_safe,
+ .read_msr_safe = xen_read_msr_safe,
+ .write_msr_safe = xen_write_msr_safe,
.read_pmc = xen_read_pmc,
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/5] x86/msr: Carry on after a non-"safe" MSR access fails without !panic_on_oops
[not found] <cover.1457723023.git.luto@kernel.org>
2016-03-11 19:06 ` [PATCH v3 1/5] x86/paravirt: Add _safe to the read_msr and write_msr PV hooks Andy Lutomirski
@ 2016-03-11 19:06 ` Andy Lutomirski
2016-03-11 19:06 ` [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr Andy Lutomirski
` (4 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Andy Lutomirski @ 2016-03-11 19:06 UTC (permalink / raw)
To: X86 ML
Cc: KVM list, Peter Zijlstra, Linus Torvalds, linux-kernel,
xen-devel, Andy Lutomirski, Paolo Bonzini, Andrew Morton,
Arjan van de Ven
This demotes an OOPS and likely panic due to a failed non-"safe" MSR
access to a WARN and, for RDMSR, a return value of zero. If
panic_on_oops is set, then failed unsafe MSR accesses will still
oops and panic.
To be clear, this type of failure should *not* happen. This patch
exists to minimize the chance of nasty undebuggable failures due on
systems that used to work due to a now-fixed CONFIG_PARAVIRT=y bug.
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/include/asm/msr.h | 10 ++++++++--
arch/x86/mm/extable.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 93fb7c1cffda..1487054a1a70 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -92,7 +92,10 @@ static inline unsigned long long native_read_msr(unsigned int msr)
{
DECLARE_ARGS(val, low, high);
- asm volatile("rdmsr" : EAX_EDX_RET(val, low, high) : "c" (msr));
+ asm volatile("1: rdmsr\n"
+ "2:\n"
+ _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_rdmsr_unsafe)
+ : EAX_EDX_RET(val, low, high) : "c" (msr));
if (msr_tracepoint_active(__tracepoint_read_msr))
do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), 0);
return EAX_EDX_VAL(val, low, high);
@@ -119,7 +122,10 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
static inline void native_write_msr(unsigned int msr,
unsigned low, unsigned high)
{
- asm volatile("wrmsr" : : "c" (msr), "a"(low), "d" (high) : "memory");
+ asm volatile("1: wrmsr\n"
+ "2:\n"
+ _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
+ : : "c" (msr), "a"(low), "d" (high) : "memory");
if (msr_tracepoint_active(__tracepoint_read_msr))
do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
}
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index 9dd7e4b7fcde..f310714e6e6d 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -49,6 +49,39 @@ bool ex_handler_ext(const struct exception_table_entry *fixup,
}
EXPORT_SYMBOL(ex_handler_ext);
+bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
+ struct pt_regs *regs, int trapnr)
+{
+ WARN(1, "unsafe MSR access error: RDMSR from 0x%x",
+ (unsigned int)regs->cx);
+
+ /* If panic_on_oops is set, don't try to recover. */
+ if (panic_on_oops)
+ return false;
+
+ regs->ip = ex_fixup_addr(fixup);
+ regs->ax = 0;
+ regs->dx = 0;
+ return true;
+}
+EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);
+
+bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup,
+ struct pt_regs *regs, int trapnr)
+{
+ WARN(1, "unsafe MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x)\n",
+ (unsigned int)regs->cx,
+ (unsigned int)regs->dx, (unsigned int)regs->ax);
+
+ /* If panic_on_oops is set, don't try to recover. */
+ if (panic_on_oops)
+ return false;
+
+ regs->ip = ex_fixup_addr(fixup);
+ return true;
+}
+EXPORT_SYMBOL(ex_handler_wrmsr_unsafe);
+
bool ex_has_fault_handler(unsigned long ip)
{
const struct exception_table_entry *e;
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr
[not found] <cover.1457723023.git.luto@kernel.org>
2016-03-11 19:06 ` [PATCH v3 1/5] x86/paravirt: Add _safe to the read_msr and write_msr PV hooks Andy Lutomirski
2016-03-11 19:06 ` [PATCH v3 2/5] x86/msr: Carry on after a non-"safe" MSR access fails without !panic_on_oops Andy Lutomirski
@ 2016-03-11 19:06 ` Andy Lutomirski
2016-03-11 19:06 ` [PATCH v3 4/5] x86/paravirt: Make "unsafe" MSR accesses unsafe even if PARAVIRT=y Andy Lutomirski
` (3 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Andy Lutomirski @ 2016-03-11 19:06 UTC (permalink / raw)
To: X86 ML
Cc: KVM list, Peter Zijlstra, Linus Torvalds, linux-kernel,
xen-devel, Andy Lutomirski, Paolo Bonzini, Andrew Morton,
Arjan van de Ven
This adds paravirt hooks for unsafe MSR access. On native, they
call native_{read,write}_msr. On Xen, they use
xen_{read,write}_msr_safe.
Nothing uses them yet for ease of bisection. The next patch will
use them in rdmsrl, wrmsrl, etc.
I intentionally didn't make them OOPS on #GP on Xen. I think that
should be done separately by the Xen maintainers.
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/include/asm/msr.h | 5 +++--
arch/x86/include/asm/paravirt.h | 11 +++++++++++
arch/x86/include/asm/paravirt_types.h | 10 ++++++++--
arch/x86/kernel/paravirt.c | 2 ++
arch/x86/xen/enlighten.c | 23 +++++++++++++++++++++++
5 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 1487054a1a70..13da359881d7 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -119,8 +119,9 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
return EAX_EDX_VAL(val, low, high);
}
-static inline void native_write_msr(unsigned int msr,
- unsigned low, unsigned high)
+/* Can be uninlined because referenced by paravirt */
+notrace static inline void native_write_msr(unsigned int msr,
+ unsigned low, unsigned high)
{
asm volatile("1: wrmsr\n"
"2:\n"
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 2e49228ed9a3..68297d87e85c 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -129,6 +129,17 @@ static inline void wbinvd(void)
#define get_kernel_rpl() (pv_info.kernel_rpl)
+static inline u64 paravirt_read_msr(unsigned msr)
+{
+ return PVOP_CALL1(u64, pv_cpu_ops.read_msr, msr);
+}
+
+static inline void paravirt_write_msr(unsigned msr,
+ unsigned low, unsigned high)
+{
+ return PVOP_VCALL3(pv_cpu_ops.write_msr, msr, low, high);
+}
+
static inline u64 paravirt_read_msr_safe(unsigned msr, int *err)
{
return PVOP_CALL2(u64, pv_cpu_ops.read_msr_safe, msr, err);
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 5a06cccd36f0..b85aec45a1b8 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -155,8 +155,14 @@ struct pv_cpu_ops {
void (*cpuid)(unsigned int *eax, unsigned int *ebx,
unsigned int *ecx, unsigned int *edx);
- /* MSR operations.
- err = 0/-EIO. wrmsr returns 0/-EIO. */
+ /* Unsafe MSR operations. These will warn or panic on failure. */
+ u64 (*read_msr)(unsigned int msr);
+ void (*write_msr)(unsigned int msr, unsigned low, unsigned high);
+
+ /*
+ * Safe MSR operations.
+ * read sets err to 0 or -EIO. write returns 0 or -EIO.
+ */
u64 (*read_msr_safe)(unsigned int msr, int *err);
int (*write_msr_safe)(unsigned int msr, unsigned low, unsigned high);
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 8aad95478ae5..f9583917c7c4 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -339,6 +339,8 @@ __visible struct pv_cpu_ops pv_cpu_ops = {
.write_cr8 = native_write_cr8,
#endif
.wbinvd = native_wbinvd,
+ .read_msr = native_read_msr,
+ .write_msr = native_write_msr,
.read_msr_safe = native_read_msr_safe,
.write_msr_safe = native_write_msr_safe,
.read_pmc = native_read_pmc,
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index ff2df20d0067..548cd3e02b9e 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1092,6 +1092,26 @@ static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
return ret;
}
+static u64 xen_read_msr(unsigned int msr)
+{
+ /*
+ * This will silently swallow a #GP from RDMSR. It may be worth
+ * changing that.
+ */
+ int err;
+
+ return xen_read_msr_safe(msr, &err);
+}
+
+static void xen_write_msr(unsigned int msr, unsigned low, unsigned high)
+{
+ /*
+ * This will silently swallow a #GP from WRMSR. It may be worth
+ * changing that.
+ */
+ xen_write_msr_safe(msr, low, high);
+}
+
void xen_setup_shared_info(void)
{
if (!xen_feature(XENFEAT_auto_translated_physmap)) {
@@ -1222,6 +1242,9 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
.wbinvd = native_wbinvd,
+ .read_msr = xen_read_msr,
+ .write_msr = xen_write_msr,
+
.read_msr_safe = xen_read_msr_safe,
.write_msr_safe = xen_write_msr_safe,
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 4/5] x86/paravirt: Make "unsafe" MSR accesses unsafe even if PARAVIRT=y
[not found] <cover.1457723023.git.luto@kernel.org>
` (2 preceding siblings ...)
2016-03-11 19:06 ` [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr Andy Lutomirski
@ 2016-03-11 19:06 ` Andy Lutomirski
2016-03-11 19:06 ` [PATCH v3 5/5] x86/msr: Set the return value to zero when native_rdmsr_safe fails Andy Lutomirski
` (2 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Andy Lutomirski @ 2016-03-11 19:06 UTC (permalink / raw)
To: X86 ML
Cc: KVM list, Peter Zijlstra, Linus Torvalds, linux-kernel,
xen-devel, Andy Lutomirski, Paolo Bonzini, Andrew Morton,
Arjan van de Ven
Enabling CONFIG_PARAVIRT had an unintended side effect: rdmsr turned
into rdmsr_safe and wrmsr turned into wrmsr_safe, even on bare
metal. Undo that by using the new unsafe paravirt MSR hooks.
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/include/asm/paravirt.h | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 68297d87e85c..0c99f10874e4 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -151,24 +151,21 @@ static inline int paravirt_write_msr_safe(unsigned msr,
return PVOP_CALL3(int, pv_cpu_ops.write_msr_safe, msr, low, high);
}
-/* These should all do BUG_ON(_err), but our headers are too tangled. */
#define rdmsr(msr, val1, val2) \
do { \
- int _err; \
- u64 _l = paravirt_read_msr_safe(msr, &_err); \
+ u64 _l = paravirt_read_msr(msr); \
val1 = (u32)_l; \
val2 = _l >> 32; \
} while (0)
#define wrmsr(msr, val1, val2) \
do { \
- paravirt_write_msr_safe(msr, val1, val2); \
+ paravirt_write_msr(msr, val1, val2); \
} while (0)
#define rdmsrl(msr, val) \
do { \
- int _err; \
- val = paravirt_read_msr_safe(msr, &_err); \
+ val = paravirt_read_msr(msr); \
} while (0)
static inline void wrmsrl(unsigned msr, u64 val)
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 5/5] x86/msr: Set the return value to zero when native_rdmsr_safe fails
[not found] <cover.1457723023.git.luto@kernel.org>
` (3 preceding siblings ...)
2016-03-11 19:06 ` [PATCH v3 4/5] x86/paravirt: Make "unsafe" MSR accesses unsafe even if PARAVIRT=y Andy Lutomirski
@ 2016-03-11 19:06 ` Andy Lutomirski
[not found] ` <35f2f107e0d85473a0e66c08f93d571a9c72b7fc.1457723023.git.luto@kernel.org>
[not found] ` <c7fb2aafd8a5885e9c333957abef9e29b691098e.1457723023.git.luto@kernel.org>
6 siblings, 0 replies; 14+ messages in thread
From: Andy Lutomirski @ 2016-03-11 19:06 UTC (permalink / raw)
To: X86 ML
Cc: KVM list, Peter Zijlstra, Linus Torvalds, linux-kernel,
xen-devel, Andy Lutomirski, Paolo Bonzini, Andrew Morton,
Arjan van de Ven
This will cause unchecked native_rdmsr_safe failures to return
deterministic results.
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/include/asm/msr.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 13da359881d7..e97e79f8a22b 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -109,7 +109,10 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
asm volatile("2: rdmsr ; xor %[err],%[err]\n"
"1:\n\t"
".section .fixup,\"ax\"\n\t"
- "3: mov %[fault],%[err] ; jmp 1b\n\t"
+ "3: mov %[fault],%[err]\n\t"
+ "xorl %%eax, %%eax\n\t"
+ "xorl %%edx, %%edx\n\t"
+ "jmp 1b\n\t"
".previous\n\t"
_ASM_EXTABLE(2b, 3b)
: [err] "=r" (*err), EAX_EDX_RET(val, low, high)
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/5] x86/msr: Carry on after a non-"safe" MSR access fails without !panic_on_oops
[not found] ` <35f2f107e0d85473a0e66c08f93d571a9c72b7fc.1457723023.git.luto@kernel.org>
@ 2016-03-12 15:31 ` Ingo Molnar
2016-03-12 15:36 ` Ingo Molnar
[not found] ` <20160312153615.GB17873@gmail.com>
2 siblings, 0 replies; 14+ messages in thread
From: Ingo Molnar @ 2016-03-12 15:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: KVM list, Peter Zijlstra, Linus Torvalds, X86 ML, linux-kernel,
xen-devel, Paolo Bonzini, Andrew Morton, Arjan van de Ven
* Andy Lutomirski <luto@kernel.org> wrote:
> +bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
> + struct pt_regs *regs, int trapnr)
> +{
> + WARN(1, "unsafe MSR access error: RDMSR from 0x%x",
> + (unsigned int)regs->cx);
Please make this WARN_ONCE(). There's no point in locking up the system with
WARN() spam, should this trigger frequently.
> + WARN(1, "unsafe MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x)\n",
> + (unsigned int)regs->cx,
> + (unsigned int)regs->dx, (unsigned int)regs->ax);
Ditto.
Thanks,
Ingo
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/5] x86/msr: Carry on after a non-"safe" MSR access fails without !panic_on_oops
[not found] ` <35f2f107e0d85473a0e66c08f93d571a9c72b7fc.1457723023.git.luto@kernel.org>
2016-03-12 15:31 ` [PATCH v3 2/5] x86/msr: Carry on after a non-"safe" MSR access fails without !panic_on_oops Ingo Molnar
@ 2016-03-12 15:36 ` Ingo Molnar
[not found] ` <20160312153615.GB17873@gmail.com>
2 siblings, 0 replies; 14+ messages in thread
From: Ingo Molnar @ 2016-03-12 15:36 UTC (permalink / raw)
To: Andy Lutomirski
Cc: KVM list, Peter Zijlstra, Linus Torvalds, X86 ML, linux-kernel,
xen-devel, Paolo Bonzini, Andrew Morton, Arjan van de Ven
* Andy Lutomirski <luto@kernel.org> wrote:
> This demotes an OOPS and likely panic due to a failed non-"safe" MSR
> access to a WARN and, for RDMSR, a return value of zero. If
> panic_on_oops is set, then failed unsafe MSR accesses will still
> oops and panic.
>
> To be clear, this type of failure should *not* happen. This patch
> exists to minimize the chance of nasty undebuggable failures due on
> systems that used to work due to a now-fixed CONFIG_PARAVIRT=y bug.
>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> arch/x86/include/asm/msr.h | 10 ++++++++--
> arch/x86/mm/extable.c | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index 93fb7c1cffda..1487054a1a70 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -92,7 +92,10 @@ static inline unsigned long long native_read_msr(unsigned int msr)
> {
> DECLARE_ARGS(val, low, high);
>
> - asm volatile("rdmsr" : EAX_EDX_RET(val, low, high) : "c" (msr));
> + asm volatile("1: rdmsr\n"
> + "2:\n"
> + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_rdmsr_unsafe)
> + : EAX_EDX_RET(val, low, high) : "c" (msr));
> if (msr_tracepoint_active(__tracepoint_read_msr))
> do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), 0);
> return EAX_EDX_VAL(val, low, high);
> @@ -119,7 +122,10 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
> static inline void native_write_msr(unsigned int msr,
> unsigned low, unsigned high)
> {
> - asm volatile("wrmsr" : : "c" (msr), "a"(low), "d" (high) : "memory");
> + asm volatile("1: wrmsr\n"
> + "2:\n"
> + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
> + : : "c" (msr), "a"(low), "d" (high) : "memory");
> if (msr_tracepoint_active(__tracepoint_read_msr))
> do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
> }
> diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
> index 9dd7e4b7fcde..f310714e6e6d 100644
> --- a/arch/x86/mm/extable.c
> +++ b/arch/x86/mm/extable.c
> @@ -49,6 +49,39 @@ bool ex_handler_ext(const struct exception_table_entry *fixup,
> }
> EXPORT_SYMBOL(ex_handler_ext);
>
> +bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
> + struct pt_regs *regs, int trapnr)
> +{
> + WARN(1, "unsafe MSR access error: RDMSR from 0x%x",
> + (unsigned int)regs->cx);
Btw., instead of the safe/unsafe naming (which has an emotional and security
secondary attribute), shouldn't we move this over to a _check() (or _checking())
naming instead that we do in other places in the kernel?
I.e.:
rdmsr(msr, l, h);
and:
if (rdmsr_check(msr, l, h)) {
...
}
and then we could name the helpers as _check() and _nocheck() - which is neutral
naming.
Thanks,
Ingo
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/5] x86/msr: Carry on after a non-"safe" MSR access fails without !panic_on_oops
[not found] ` <20160312153615.GB17873@gmail.com>
@ 2016-03-12 17:32 ` Andy Lutomirski
0 siblings, 0 replies; 14+ messages in thread
From: Andy Lutomirski @ 2016-03-12 17:32 UTC (permalink / raw)
To: Ingo Molnar
Cc: KVM list, Peter Zijlstra, Linus Torvalds, X86 ML, linux-kernel,
xen-devel, Andy Lutomirski, Paolo Bonzini, Andrew Morton,
Arjan van de Ven
On Sat, Mar 12, 2016 at 7:36 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@kernel.org> wrote:
>
>> This demotes an OOPS and likely panic due to a failed non-"safe" MSR
>> access to a WARN and, for RDMSR, a return value of zero. If
>> panic_on_oops is set, then failed unsafe MSR accesses will still
>> oops and panic.
>>
>> To be clear, this type of failure should *not* happen. This patch
>> exists to minimize the chance of nasty undebuggable failures due on
>> systems that used to work due to a now-fixed CONFIG_PARAVIRT=y bug.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>> arch/x86/include/asm/msr.h | 10 ++++++++--
>> arch/x86/mm/extable.c | 33 +++++++++++++++++++++++++++++++++
>> 2 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
>> index 93fb7c1cffda..1487054a1a70 100644
>> --- a/arch/x86/include/asm/msr.h
>> +++ b/arch/x86/include/asm/msr.h
>> @@ -92,7 +92,10 @@ static inline unsigned long long native_read_msr(unsigned int msr)
>> {
>> DECLARE_ARGS(val, low, high);
>>
>> - asm volatile("rdmsr" : EAX_EDX_RET(val, low, high) : "c" (msr));
>> + asm volatile("1: rdmsr\n"
>> + "2:\n"
>> + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_rdmsr_unsafe)
>> + : EAX_EDX_RET(val, low, high) : "c" (msr));
>> if (msr_tracepoint_active(__tracepoint_read_msr))
>> do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), 0);
>> return EAX_EDX_VAL(val, low, high);
>> @@ -119,7 +122,10 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
>> static inline void native_write_msr(unsigned int msr,
>> unsigned low, unsigned high)
>> {
>> - asm volatile("wrmsr" : : "c" (msr), "a"(low), "d" (high) : "memory");
>> + asm volatile("1: wrmsr\n"
>> + "2:\n"
>> + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
>> + : : "c" (msr), "a"(low), "d" (high) : "memory");
>> if (msr_tracepoint_active(__tracepoint_read_msr))
>> do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
>> }
>> diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
>> index 9dd7e4b7fcde..f310714e6e6d 100644
>> --- a/arch/x86/mm/extable.c
>> +++ b/arch/x86/mm/extable.c
>> @@ -49,6 +49,39 @@ bool ex_handler_ext(const struct exception_table_entry *fixup,
>> }
>> EXPORT_SYMBOL(ex_handler_ext);
>>
>> +bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
>> + struct pt_regs *regs, int trapnr)
>> +{
>> + WARN(1, "unsafe MSR access error: RDMSR from 0x%x",
>> + (unsigned int)regs->cx);
>
> Btw., instead of the safe/unsafe naming (which has an emotional and security
> secondary attribute), shouldn't we move this over to a _check() (or _checking())
> naming instead that we do in other places in the kernel?
>
> I.e.:
>
> rdmsr(msr, l, h);
>
> and:
>
> if (rdmsr_check(msr, l, h)) {
> ...
> }
>
> and then we could name the helpers as _check() and _nocheck() - which is neutral
> naming.
Will do as a separate followup series.
At least with this series applied, the functions named _safe all point
to each other correctly.
--Andy
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr
[not found] ` <c7fb2aafd8a5885e9c333957abef9e29b691098e.1457723023.git.luto@kernel.org>
@ 2016-03-14 14:02 ` Paolo Bonzini
[not found] ` <56E6C492.5060308@redhat.com>
1 sibling, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2016-03-14 14:02 UTC (permalink / raw)
To: Andy Lutomirski, X86 ML
Cc: KVM list, Peter Zijlstra, Arjan van de Ven, linux-kernel,
xen-devel, Andrew Morton, Linus Torvalds
On 11/03/2016 20:06, Andy Lutomirski wrote:
> This adds paravirt hooks for unsafe MSR access. On native, they
> call native_{read,write}_msr. On Xen, they use
> xen_{read,write}_msr_safe.
>
> Nothing uses them yet for ease of bisection. The next patch will
> use them in rdmsrl, wrmsrl, etc.
>
> I intentionally didn't make them OOPS on #GP on Xen. I think that
> should be done separately by the Xen maintainers.
Please do the same for KVM.
Paolo
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr
[not found] ` <56E6C492.5060308@redhat.com>
@ 2016-03-14 16:53 ` Andy Lutomirski
2016-03-14 16:58 ` Linus Torvalds
2016-03-15 8:56 ` Paolo Bonzini
0 siblings, 2 replies; 14+ messages in thread
From: Andy Lutomirski @ 2016-03-14 16:53 UTC (permalink / raw)
To: Paolo Bonzini
Cc: KVM list, Peter Zijlstra, Linus Torvalds, X86 ML, linux-kernel,
xen-devel, Andrew Morton, Arjan van de Ven
On Mar 14, 2016 7:03 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>
>
>
> On 11/03/2016 20:06, Andy Lutomirski wrote:
> > This adds paravirt hooks for unsafe MSR access. On native, they
> > call native_{read,write}_msr. On Xen, they use
> > xen_{read,write}_msr_safe.
> >
> > Nothing uses them yet for ease of bisection. The next patch will
> > use them in rdmsrl, wrmsrl, etc.
> >
> > I intentionally didn't make them OOPS on #GP on Xen. I think that
> > should be done separately by the Xen maintainers.
>
> Please do the same for KVM.
Can you clarify? KVM uses the native version, and the native version
only oopses with this series applied if panic_on_oops is set.
--Andy
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr
2016-03-14 16:53 ` Andy Lutomirski
@ 2016-03-14 16:58 ` Linus Torvalds
2016-03-14 17:02 ` Andy Lutomirski
[not found] ` <CALCETrWYmJdOeHqjR6-Uyqiyym35DOjFpsB1xhgTHO_JB==EMA@mail.gmail.com>
2016-03-15 8:56 ` Paolo Bonzini
1 sibling, 2 replies; 14+ messages in thread
From: Linus Torvalds @ 2016-03-14 16:58 UTC (permalink / raw)
To: Andy Lutomirski
Cc: KVM list, Peter Zijlstra, X86 ML, linux-kernel, xen-devel,
Paolo Bonzini, Andrew Morton, Arjan van de Ven
[-- Attachment #1.1: Type: text/plain, Size: 341 bytes --]
On Mar 14, 2016 9:53 AM, "Andy Lutomirski" <luto@amacapital.net> wrote:
>
> Can you clarify? KVM uses the native version, and the native version
> only oopses with this series applied if panic_on_oops is set.
Can we please remove that idiocy?
There is no reason to panic whatsoever. Seriously. What's the upside of
that logic?
Linus
[-- Attachment #1.2: Type: text/html, Size: 505 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr
2016-03-14 16:58 ` Linus Torvalds
@ 2016-03-14 17:02 ` Andy Lutomirski
[not found] ` <CALCETrWYmJdOeHqjR6-Uyqiyym35DOjFpsB1xhgTHO_JB==EMA@mail.gmail.com>
1 sibling, 0 replies; 14+ messages in thread
From: Andy Lutomirski @ 2016-03-14 17:02 UTC (permalink / raw)
To: Linus Torvalds
Cc: KVM list, Peter Zijlstra, X86 ML, linux-kernel, xen-devel,
Borislav Petkov, Paolo Bonzini, Andrew Morton, Arjan van de Ven
On Mon, Mar 14, 2016 at 9:58 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mar 14, 2016 9:53 AM, "Andy Lutomirski" <luto@amacapital.net> wrote:
>>
>> Can you clarify? KVM uses the native version, and the native version
>> only oopses with this series applied if panic_on_oops is set.
>
> Can we please remove that idiocy?
>
> There is no reason to panic whatsoever. Seriously. What's the upside of that
> logic?
I imagine that people who set panic_on_oops want their systems to stop
running user code if something happens that could corrupt the state or
if there's any sign that user code is trying some non-deterministic
exploit. So I'm guessing that they'd want this type of "the kernel
screwed up -- abort" to actually result in a panic.
As a concrete, although somewhat silly, example, suppose that a write
to MSR_SYSENTER_STACK fails. If that happened, then user code could
subsequently try to take over the kernel by evil manipulation of TF
and/or perf.
I'd be okay with removing this too, though, since arranging for MSR
access to fail seems unlikely as an exploit vector.
Borislav: SUSE actually uses panic_on_oops, right? What's their goal?
--Andy
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr
[not found] ` <CALCETrWYmJdOeHqjR6-Uyqiyym35DOjFpsB1xhgTHO_JB==EMA@mail.gmail.com>
@ 2016-03-15 8:49 ` Paolo Bonzini
0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2016-03-15 8:49 UTC (permalink / raw)
To: Andy Lutomirski, Linus Torvalds
Cc: KVM list, Peter Zijlstra, X86 ML, linux-kernel, xen-devel,
Borislav Petkov, Andrew Morton, Arjan van de Ven
On 14/03/2016 18:02, Andy Lutomirski wrote:
> On Mon, Mar 14, 2016 at 9:58 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> On Mar 14, 2016 9:53 AM, "Andy Lutomirski" <luto@amacapital.net> wrote:
>>>
>>> Can you clarify? KVM uses the native version, and the native version
>>> only oopses with this series applied if panic_on_oops is set.
>>
>> Can we please remove that idiocy?
>>
>> There is no reason to panic whatsoever. Seriously. What's the upside of that
>> logic?
>
> I imagine that people who set panic_on_oops want their systems to stop
> running user code if something happens that could corrupt the state or
> if there's any sign that user code is trying some non-deterministic
> exploit. So I'm guessing that they'd want this type of "the kernel
> screwed up -- abort" to actually result in a panic.
>
> As a concrete, although somewhat silly, example, suppose that a write
> to MSR_SYSENTER_STACK fails. If that happened, then user code could
> subsequently try to take over the kernel by evil manipulation of TF
> and/or perf.
>
> I'd be okay with removing this too, though, since arranging for MSR
> access to fail seems unlikely as an exploit vector.
>
> Borislav: SUSE actually uses panic_on_oops, right? What's their goal?
RHEL also does, and it's mostly to trap kernel page faults before they
do more damage such as filesystem corruption. The debug kernel has
panic_on_oops=0, while the production kernel has panic_on_oops=1.
Paolo
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr
2016-03-14 16:53 ` Andy Lutomirski
2016-03-14 16:58 ` Linus Torvalds
@ 2016-03-15 8:56 ` Paolo Bonzini
1 sibling, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2016-03-15 8:56 UTC (permalink / raw)
To: Andy Lutomirski
Cc: KVM list, Peter Zijlstra, Linus Torvalds, X86 ML, linux-kernel,
xen-devel, Andrew Morton, Arjan van de Ven
On 14/03/2016 17:53, Andy Lutomirski wrote:
> > Please do the same for KVM.
>
> Can you clarify? KVM uses the native version, and the native version
> only oopses with this series applied if panic_on_oops is set.
Since you fixed the panic_on_oops thing, I'm okay with letting KVM use
the unsafe version and seeing what breaks...
Paolo
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-03-15 8:56 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1457723023.git.luto@kernel.org>
2016-03-11 19:06 ` [PATCH v3 1/5] x86/paravirt: Add _safe to the read_msr and write_msr PV hooks Andy Lutomirski
2016-03-11 19:06 ` [PATCH v3 2/5] x86/msr: Carry on after a non-"safe" MSR access fails without !panic_on_oops Andy Lutomirski
2016-03-11 19:06 ` [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr Andy Lutomirski
2016-03-11 19:06 ` [PATCH v3 4/5] x86/paravirt: Make "unsafe" MSR accesses unsafe even if PARAVIRT=y Andy Lutomirski
2016-03-11 19:06 ` [PATCH v3 5/5] x86/msr: Set the return value to zero when native_rdmsr_safe fails Andy Lutomirski
[not found] ` <35f2f107e0d85473a0e66c08f93d571a9c72b7fc.1457723023.git.luto@kernel.org>
2016-03-12 15:31 ` [PATCH v3 2/5] x86/msr: Carry on after a non-"safe" MSR access fails without !panic_on_oops Ingo Molnar
2016-03-12 15:36 ` Ingo Molnar
[not found] ` <20160312153615.GB17873@gmail.com>
2016-03-12 17:32 ` Andy Lutomirski
[not found] ` <c7fb2aafd8a5885e9c333957abef9e29b691098e.1457723023.git.luto@kernel.org>
2016-03-14 14:02 ` [PATCH v3 3/5] x86/paravirt: Add paravirt_{read, write}_msr Paolo Bonzini
[not found] ` <56E6C492.5060308@redhat.com>
2016-03-14 16:53 ` Andy Lutomirski
2016-03-14 16:58 ` Linus Torvalds
2016-03-14 17:02 ` Andy Lutomirski
[not found] ` <CALCETrWYmJdOeHqjR6-Uyqiyym35DOjFpsB1xhgTHO_JB==EMA@mail.gmail.com>
2016-03-15 8:49 ` Paolo Bonzini
2016-03-15 8:56 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).