Mailing List Archive

You're doing user-space references without get_user().

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, Aug 27, 2010 at 8:48 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> You're doing user-space references without get_user().
>
>        -hpa
>
> --
> H. Peter Anvin, Intel Open Source Technology Center
> I work for Intel.  I don't speak on their behalf.
>
>

Here with get_user.
I CC Natale which is my beta tester, he have some Alix boards running 24/24
with my patch

--- a/arch/x86/kernel/Makefile 2010-08-27 19:42:01.795858001 +0200
+++ b/arch/x86/kernel/Makefile 2010-08-27 19:42:12.525858001 +0200
@@ -88,6 +88,8 @@
obj-$(CONFIG_APB_TIMER) += apb_timer.o

obj-$(CONFIG_K8_NB) += k8.o
+obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
+obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
obj-$(CONFIG_DEBUG_RODATA_TEST) += test_rodata.o
obj-$(CONFIG_DEBUG_NX_TEST) += test_nx.o

--- a/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:01.855858001 +0200
+++ b/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:12.535858001 +0200
@@ -137,11 +137,15 @@
return;
}

+#ifdef CONFIG_GEODE_NOPL
if (c->x86_model == 10) {
- /* AMD Geode LX is model 10 */
- /* placeholder for any needed mods */
+ /* Geode only lacks the NOPL instruction to be i686,
+ but we can promote it to a i686 class cpu
+ and emulate NOPLs in the exception handler*/
+ boot_cpu_data.x86 = 6;
return;
}
+#endif
}

static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
--- a/arch/x86/kernel/entry_32.S 2010-08-27 19:42:01.735858001 +0200
+++ b/arch/x86/kernel/entry_32.S 2010-08-27 19:42:12.535858001 +0200
@@ -978,7 +978,11 @@
RING0_INT_FRAME
pushl $0
CFI_ADJUST_CFA_OFFSET 4
+#ifdef CONFIG_GEODE_NOPL
+ pushl $do_nopl_emu
+#else
pushl $do_invalid_op
+#endif
CFI_ADJUST_CFA_OFFSET 4
jmp error_code
CFI_ENDPROC
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ b/arch/x86/kernel/nopl_emu.c 2010-08-27 22:09:58.005858001 +0200
@@ -0,0 +1,124 @@
+/*
+ * linux/arch/x86/kernel/nopl_emu.c
+ *
+ * Copyright (C) 2002 Willy Tarreau
+ * Copyright (C) 2010 Matteo Croce
+ */
+
+#include <asm/processor.h>
+#include <asm/traps.h>
+#include <asm/uaccess.h>
+
+/* This code can be used to allow the AMD Geode to hopefully correctly execute
+ * some code which was originally compiled for an i686, by emulating NOPL,
+ * the only missing i686 instruction in the CPU
+ *
+ * Willy Tarreau <willy@meta-x.org>
+ * Matteo Croce <technoboy85@gmail.com>
+ */
+
+static inline int do_1f(u8 *ip)
+{
+ u8 val1, val2;
+ int length = 3;
+ get_user(val1, ip);
+ switch (val1) {
+ case 0x84:
+ get_user(val1, ip + 5);
+ if (!val1)
+ length++;
+ else
+ return 0;
+ case 0x80:
+ get_user(val1, ip + 4);
+ get_user(val2, ip + 3);
+ if (!val1 && !val2)
+ length += 2;
+ else
+ return 0;
+ case 0x44:
+ get_user(val1, ip + 2);
+ if (!val1)
+ length++;
+ else
+ return 0;
+ case 0x40:
+ get_user(val1, ip + 1);
+ if (!val1)
+ length++;
+ else
+ return 0;
+ case 0x00:
+ return length;
+ }
+ return 0;
+}
+
+static inline int do_0f(u8 *ip)
+{
+ u8 val;
+ get_user(val, ip);
+ if (val == 0x1f)
+ return do_1f(ip + 1);
+ return 0;
+}
+
+static inline int do_66(u8 *ip)
+{
+ u8 val;
+ get_user(val, ip);
+ if (val == 0x90)
+ return 2;
+ if (val == 0x0f) {
+ int res = do_0f(ip + 1);
+ if (res)
+ return res + 1;
+ else
+ return 0;
+ }
+ return 0;
+}
+
+static inline int do_start(u8 *ip)
+{
+ u8 val;
+ get_user(val, ip);
+ if (val == 0x0f)
+ return do_0f(ip + 1);
+ if (val == 0x66)
+ return do_66(ip + 1);
+ return 0;
+}
+
+/* [do_nopl_emu] is called by exception 6 after an invalid opcode has been
+ * encountered. It will try to emulate it by doing nothing,
+ * and will send a SIGILL or SIGSEGV to the process if not possible.
+ * the NOPL can have variable length opcodes:
+
+bytes number opcode
+ 2 66 90
+ 3 0f 1f 00
+ 4 0f 1f 40 00
+ 5 0f 1f 44 00 00
+ 6 66 0f 1f 44 00 00
+ 7 0f 1f 80 00 00 00 00
+ 8 0f 1f 84 00 00 00 00 00
+ 9 66 0f 1f 84 00 00 00 00 00
+*/
+void do_nopl_emu(struct pt_regs *regs, long error_code)
+{
+ u8 *ip = (u8 *)instruction_pointer(regs);
+ int res = do_start(ip);
+
+ if (res) {
+ int i = 0;
+ do {
+ ip += res;
+ i++;
+ res = do_start(ip);
+ } while(res);
+ printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
+ regs->ip = (typeof(regs->ip))ip;
+ } else
+ do_invalid_op(regs, error_code);
+}


--
Matteo Croce
OpenWrt developer
  _______                     ________        __
 |       |.-----.-----.-----.|  |  |  |.----.|  |_
 |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
 |_______||   __|_____|__|__||________||__|  |____|
          |__| W I R E L E S S   F R E E D O M
 KAMIKAZE (bleeding edge) ------------------
  * 10 oz Vodka       Shake well with ice and strain
  * 10 oz Triple sec  mixture into 10 shot glasses.
  * 10 oz lime juice  Salute!
 ---------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

27.08.2010 23:15, Matteo Croce skrev:
> On Fri, Aug 27, 2010 at 8:48 PM, H. Peter Anvin<hpa@zytor.com> wrote:
>> You're doing user-space references without get_user().
>>
>> -hpa
>>
>> --
>> H. Peter Anvin, Intel Open Source Technology Center
>> I work for Intel. I don't speak on their behalf.
>>
>>
>
> Here with get_user.
> I CC Natale which is my beta tester, he have some Alix boards running 24/24
> with my patch
>
> --- a/arch/x86/kernel/Makefile 2010-08-27 19:42:01.795858001 +0200
> +++ b/arch/x86/kernel/Makefile 2010-08-27 19:42:12.525858001 +0200
> @@ -88,6 +88,8 @@
> obj-$(CONFIG_APB_TIMER) += apb_timer.o
>
> obj-$(CONFIG_K8_NB) += k8.o
> +obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
> +obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o

Same line added twice...

--
Thomas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

You actually have to check the get_user return, or use exception brackets....

"Matteo Croce" <matteo@openwrt.org> wrote:

>On Fri, Aug 27, 2010 at 8:48 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> You're doing user-space references without get_user().
>>
>>        -hpa
>>
>> --
>> H. Peter Anvin, Intel Open Source Technology Center
>> I work for Intel.  I don't speak on their behalf.
>>
>>
>
>Here with get_user.
>I CC Natale which is my beta tester, he have some Alix boards running 24/24
>with my patch
>
>--- a/arch/x86/kernel/Makefile 2010-08-27 19:42:01.795858001 +0200
>+++ b/arch/x86/kernel/Makefile 2010-08-27 19:42:12.525858001 +0200
>@@ -88,6 +88,8 @@
> obj-$(CONFIG_APB_TIMER) += apb_timer.o
>
> obj-$(CONFIG_K8_NB) += k8.o
>+obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
>+obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
> obj-$(CONFIG_DEBUG_RODATA_TEST) += test_rodata.o
> obj-$(CONFIG_DEBUG_NX_TEST) += test_nx.o
>
>--- a/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:01.855858001 +0200
>+++ b/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:12.535858001 +0200
>@@ -137,11 +137,15 @@
> return;
> }
>
>+#ifdef CONFIG_GEODE_NOPL
> if (c->x86_model == 10) {
>- /* AMD Geode LX is model 10 */
>- /* placeholder for any needed mods */
>+ /* Geode only lacks the NOPL instruction to be i686,
>+ but we can promote it to a i686 class cpu
>+ and emulate NOPLs in the exception handler*/
>+ boot_cpu_data.x86 = 6;
> return;
> }
>+#endif
> }
>
> static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
>--- a/arch/x86/kernel/entry_32.S 2010-08-27 19:42:01.735858001 +0200
>+++ b/arch/x86/kernel/entry_32.S 2010-08-27 19:42:12.535858001 +0200
>@@ -978,7 +978,11 @@
> RING0_INT_FRAME
> pushl $0
> CFI_ADJUST_CFA_OFFSET 4
>+#ifdef CONFIG_GEODE_NOPL
>+ pushl $do_nopl_emu
>+#else
> pushl $do_invalid_op
>+#endif
> CFI_ADJUST_CFA_OFFSET 4
> jmp error_code
> CFI_ENDPROC
>--- /dev/null 1970-01-01 00:00:00.000000000 +0000
>+++ b/arch/x86/kernel/nopl_emu.c 2010-08-27 22:09:58.005858001 +0200
>@@ -0,0 +1,124 @@
>+/*
>+ * linux/arch/x86/kernel/nopl_emu.c
>+ *
>+ * Copyright (C) 2002 Willy Tarreau
>+ * Copyright (C) 2010 Matteo Croce
>+ */
>+
>+#include <asm/processor.h>
>+#include <asm/traps.h>
>+#include <asm/uaccess.h>
>+
>+/* This code can be used to allow the AMD Geode to hopefully correctly execute
>+ * some code which was originally compiled for an i686, by emulating NOPL,
>+ * the only missing i686 instruction in the CPU
>+ *
>+ * Willy Tarreau <willy@meta-x.org>
>+ * Matteo Croce <technoboy85@gmail.com>
>+ */
>+
>+static inline int do_1f(u8 *ip)
>+{
>+ u8 val1, val2;
>+ int length = 3;
>+ get_user(val1, ip);
>+ switch (val1) {
>+ case 0x84:
>+ get_user(val1, ip + 5);
>+ if (!val1)
>+ length++;
>+ else
>+ return 0;
>+ case 0x80:
>+ get_user(val1, ip + 4);
>+ get_user(val2, ip + 3);
>+ if (!val1 && !val2)
>+ length += 2;
>+ else
>+ return 0;
>+ case 0x44:
>+ get_user(val1, ip + 2);
>+ if (!val1)
>+ length++;
>+ else
>+ return 0;
>+ case 0x40:
>+ get_user(val1, ip + 1);
>+ if (!val1)
>+ length++;
>+ else
>+ return 0;
>+ case 0x00:
>+ return length;
>+ }
>+ return 0;
>+}
>+
>+static inline int do_0f(u8 *ip)
>+{
>+ u8 val;
>+ get_user(val, ip);
>+ if (val == 0x1f)
>+ return do_1f(ip + 1);
>+ return 0;
>+}
>+
>+static inline int do_66(u8 *ip)
>+{
>+ u8 val;
>+ get_user(val, ip);
>+ if (val == 0x90)
>+ return 2;
>+ if (val == 0x0f) {
>+ int res = do_0f(ip + 1);
>+ if (res)
>+ return res + 1;
>+ else
>+ return 0;
>+ }
>+ return 0;
>+}
>+
>+static inline int do_start(u8 *ip)
>+{
>+ u8 val;
>+ get_user(val, ip);
>+ if (val == 0x0f)
>+ return do_0f(ip + 1);
>+ if (val == 0x66)
>+ return do_66(ip + 1);
>+ return 0;
>+}
>+
>+/* [do_nopl_emu] is called by exception 6 after an invalid opcode has been
>+ * encountered. It will try to emulate it by doing nothing,
>+ * and will send a SIGILL or SIGSEGV to the process if not possible.
>+ * the NOPL can have variable length opcodes:
>+
>+bytes number opcode
>+ 2 66 90
>+ 3 0f 1f 00
>+ 4 0f 1f 40 00
>+ 5 0f 1f 44 00 00
>+ 6 66 0f 1f 44 00 00
>+ 7 0f 1f 80 00 00 00 00
>+ 8 0f 1f 84 00 00 00 00 00
>+ 9 66 0f 1f 84 00 00 00 00 00
>+*/
>+void do_nopl_emu(struct pt_regs *regs, long error_code)
>+{
>+ u8 *ip = (u8 *)instruction_pointer(regs);
>+ int res = do_start(ip);
>+
>+ if (res) {
>+ int i = 0;
>+ do {
>+ ip += res;
>+ i++;
>+ res = do_start(ip);
>+ } while(res);
>+ printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
>+ regs->ip = (typeof(regs->ip))ip;
>+ } else
>+ do_invalid_op(regs, error_code);
>+}
>
>
>--
>Matteo Croce
>OpenWrt developer
>  _______                     ________        __
> |       |.-----.-----.-----.|  |  |  |.----.|  |_
> |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
> |_______||   __|_____|__|__||________||__|  |____|
>          |__| W I R E L E S S   F R E E D O M
> KAMIKAZE (bleeding edge) ------------------
>  * 10 oz Vodka       Shake well with ice and strain
>  * 10 oz Triple sec  mixture into 10 shot glasses.
>  * 10 oz lime juice  Salute!
> ---------------------------------------------------
>

--
Sent from my mobile phone. Please pardon any lack of formatting.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

can I ignore the return value when I expect val to be non zero?
the doc says: "On error, the variable @x is set to zero."

On Fri, Aug 27, 2010 at 10:49 PM, Thomas Backlund <tmb@mandriva.org> wrote:
> 27.08.2010 23:15, Matteo Croce skrev:
>>
>> On Fri, Aug 27, 2010 at 8:48 PM, H. Peter Anvin<hpa@zytor.com>  wrote:
>>>
>>> You're doing user-space references without get_user().
>>>
>>>        -hpa
>>>
>>> --
>>> H. Peter Anvin, Intel Open Source Technology Center
>>> I work for Intel.  I don't speak on their behalf.
>>>
>>>
>>
>> Here with get_user.
>> I CC Natale which is my beta tester, he have some Alix boards running
>> 24/24
>> with my patch
>>
>> --- a/arch/x86/kernel/Makefile  2010-08-27 19:42:01.795858001 +0200
>> +++ b/arch/x86/kernel/Makefile  2010-08-27 19:42:12.525858001 +0200
>> @@ -88,6 +88,8 @@
>>  obj-$(CONFIG_APB_TIMER)               += apb_timer.o
>>
>>  obj-$(CONFIG_K8_NB)           += k8.o
>> +obj-$(CONFIG_GEODE_NOPL)       += nopl_emu.o
>> +obj-$(CONFIG_GEODE_NOPL)       += nopl_emu.o
>
> Same line added twice...
>
> --
> Thomas
>



--
Matteo Croce
OpenWrt developer
  _______                     ________        __
 |       |.-----.-----.-----.|  |  |  |.----.|  |_
 |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
 |_______||   __|_____|__|__||________||__|  |____|
          |__| W I R E L E S S   F R E E D O M
 KAMIKAZE (bleeding edge) ------------------
  * 10 oz Vodka       Shake well with ice and strain
  * 10 oz Triple sec  mixture into 10 shot glasses.
  * 10 oz lime juice  Salute!
 ---------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, Aug 27, 2010 at 11:32 PM, Matteo Croce <matteo@openwrt.org> wrote:
> can I ignore the return value when I expect val to be non zero?
> the doc says: "On error, the variable @x is set to zero."
>
> On Fri, Aug 27, 2010 at 10:49 PM, Thomas Backlund <tmb@mandriva.org> wrote:
>> 27.08.2010 23:15, Matteo Croce skrev:
>>>
>>> On Fri, Aug 27, 2010 at 8:48 PM, H. Peter Anvin<hpa@zytor.com>  wrote:
>>>>
>>>> You're doing user-space references without get_user().
>>>>
>>>>        -hpa
>>>>
>>>> --
>>>> H. Peter Anvin, Intel Open Source Technology Center
>>>> I work for Intel.  I don't speak on their behalf.
>>>>
>>>>
>>>
>>> Here with get_user.
>>> I CC Natale which is my beta tester, he have some Alix boards running
>>> 24/24
>>> with my patch
>>>
>>> --- a/arch/x86/kernel/Makefile  2010-08-27 19:42:01.795858001 +0200
>>> +++ b/arch/x86/kernel/Makefile  2010-08-27 19:42:12.525858001 +0200
>>> @@ -88,6 +88,8 @@
>>>  obj-$(CONFIG_APB_TIMER)               += apb_timer.o
>>>
>>>  obj-$(CONFIG_K8_NB)           += k8.o
>>> +obj-$(CONFIG_GEODE_NOPL)       += nopl_emu.o
>>> +obj-$(CONFIG_GEODE_NOPL)       += nopl_emu.o
>>
>> Same line added twice...
>>
>> --
>> Thomas
>>
>
>
>
> --
> Matteo Croce
> OpenWrt developer
>   _______                     ________        __
>  |       |.-----.-----.-----.|  |  |  |.----.|  |_
>  |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
>  |_______||   __|_____|__|__||________||__|  |____|
>           |__| W I R E L E S S   F R E E D O M
>  KAMIKAZE (bleeding edge) ------------------
>   * 10 oz Vodka       Shake well with ice and strain
>   * 10 oz Triple sec  mixture into 10 shot glasses.
>   * 10 oz lime juice  Salute!
>  ---------------------------------------------------
>

Here is with proper checks

--- a/arch/x86/kernel/Makefile 2010-08-27 19:42:01.795858001 +0200
+++ b/arch/x86/kernel/Makefile 2010-08-27 19:42:12.525858001 +0200
@@ -88,6 +88,8 @@
obj-$(CONFIG_APB_TIMER) += apb_timer.o

obj-$(CONFIG_K8_NB) += k8.o
+obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
+obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
obj-$(CONFIG_DEBUG_RODATA_TEST) += test_rodata.o
obj-$(CONFIG_DEBUG_NX_TEST) += test_nx.o

--- a/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:01.855858001 +0200
+++ b/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:12.535858001 +0200
@@ -137,11 +137,15 @@
return;
}

+#ifdef CONFIG_GEODE_NOPL
if (c->x86_model == 10) {
- /* AMD Geode LX is model 10 */
- /* placeholder for any needed mods */
+ /* Geode only lacks the NOPL instruction to be i686,
+ but we can promote it to a i686 class cpu
+ and emulate NOPLs in the exception handler*/
+ boot_cpu_data.x86 = 6;
return;
}
+#endif
}

static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
--- a/arch/x86/kernel/entry_32.S 2010-08-27 19:42:01.735858001 +0200
+++ b/arch/x86/kernel/entry_32.S 2010-08-27 19:42:12.535858001 +0200
@@ -978,7 +978,11 @@
RING0_INT_FRAME
pushl $0
CFI_ADJUST_CFA_OFFSET 4
+#ifdef CONFIG_GEODE_NOPL
+ pushl $do_nopl_emu
+#else
pushl $do_invalid_op
+#endif
CFI_ADJUST_CFA_OFFSET 4
jmp error_code
CFI_ENDPROC
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ b/arch/x86/kernel/nopl_emu.c 2010-08-28 00:11:52.627085002 +0200
@@ -0,0 +1,128 @@
+/*
+ * linux/arch/x86/kernel/nopl_emu.c
+ *
+ * Copyright (C) 2002 Willy Tarreau
+ * Copyright (C) 2010 Matteo Croce
+ */
+
+#include <asm/processor.h>
+#include <asm/traps.h>
+#include <asm/uaccess.h>
+
+/* This code can be used to allow the AMD Geode to hopefully correctly execute
+ * some code which was originally compiled for an i686, by emulating NOPL,
+ * the only missing i686 instruction in the CPU
+ *
+ * Willy Tarreau <willy@meta-x.org>
+ * Matteo Croce <technoboy85@gmail.com>
+ */
+
+static inline int do_1f(u8 *ip)
+{
+ u8 val1, val2;
+ int length = 3;
+ if (get_user(val1, ip))
+ return 0;
+ switch (val1) {
+ case 0x84:
+ get_user(val1, ip + 5);
+ if (!val1)
+ length++;
+ else
+ return 0;
+ case 0x80:
+ get_user(val1, ip + 4);
+ get_user(val2, ip + 3);
+ if (!val1 && !val2)
+ length += 2;
+ else
+ return 0;
+ case 0x44:
+ get_user(val1, ip + 2);
+ if (!val1)
+ length++;
+ else
+ return 0;
+ case 0x40:
+ get_user(val1, ip + 1);
+ if (!val1)
+ length++;
+ else
+ return 0;
+ case 0x00:
+ return length;
+ }
+ return 0;
+}
+
+static inline int do_0f(u8 *ip)
+{
+ u8 val;
+ if (get_user(val, ip))
+ return 0;
+ if (val == 0x1f)
+ return do_1f(ip + 1);
+ return 0;
+}
+
+static inline int do_66(u8 *ip)
+{
+ u8 val;
+ if (get_user(val, ip))
+ return 0;
+ if (val == 0x90)
+ return 2;
+ if (val == 0x0f) {
+ int res = do_0f(ip + 1);
+ if (res)
+ return res + 1;
+ else
+ return 0;
+ }
+ return 0;
+}
+
+static inline int do_start(u8 *ip)
+{
+ u8 val;
+ if (get_user(val, ip))
+ return 0;
+ if (val == 0x0f)
+ return do_0f(ip + 1);
+ if (val == 0x66)
+ return do_66(ip + 1);
+ return 0;
+}
+
+/* [do_nopl_emu] is called by exception 6 after an invalid opcode has been
+ * encountered. It will try to emulate it by doing nothing,
+ * and will send a SIGILL or SIGSEGV to the process if not possible.
+ * the NOPL can have variable length opcodes:
+
+bytes number opcode
+ 2 66 90
+ 3 0f 1f 00
+ 4 0f 1f 40 00
+ 5 0f 1f 44 00 00
+ 6 66 0f 1f 44 00 00
+ 7 0f 1f 80 00 00 00 00
+ 8 0f 1f 84 00 00 00 00 00
+ 9 66 0f 1f 84 00 00 00 00 00
+*/
+void do_nopl_emu(struct pt_regs *regs, long error_code)
+{
+ u8 *ip = (u8 *)instruction_pointer(regs);
+ int res = do_start(ip);
+
+ if (res) {
+ int i = 0;
+ do {
+ ip += res;
+ i++;
+ res = do_start(ip);
+ } while(res);
+ printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
+ regs->ip = (typeof(regs->ip))ip;
+ } else
+ do_invalid_op(regs, error_code);
+}


--
Matteo Croce
OpenWrt developer
  _______                     ________        __
 |       |.-----.-----.-----.|  |  |  |.----.|  |_
 |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
 |_______||   __|_____|__|__||________||__|  |____|
          |__| W I R E L E S S   F R E E D O M
 KAMIKAZE (bleeding edge) ------------------
  * 10 oz Vodka       Shake well with ice and strain
  * 10 oz Triple sec  mixture into 10 shot glasses.
  * 10 oz lime juice  Salute!
 ---------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sat, Aug 28, 2010 at 12:16 AM, Matteo Croce <matteo@openwrt.org> wrote:
> On Fri, Aug 27, 2010 at 11:32 PM, Matteo Croce <matteo@openwrt.org> wrote:
>> can I ignore the return value when I expect val to be non zero?
>> the doc says: "On error, the variable @x is set to zero."
>>
>> On Fri, Aug 27, 2010 at 10:49 PM, Thomas Backlund <tmb@mandriva.org> wrote:
>>> 27.08.2010 23:15, Matteo Croce skrev:
>>>>
>>>> On Fri, Aug 27, 2010 at 8:48 PM, H. Peter Anvin<hpa@zytor.com>  wrote:
>>>>>
>>>>> You're doing user-space references without get_user().
>>>>>
>>>>>        -hpa
>>>>>
>>>>> --
>>>>> H. Peter Anvin, Intel Open Source Technology Center
>>>>> I work for Intel.  I don't speak on their behalf.
>>>>>
>>>>>
>>>>
>>>> Here with get_user.
>>>> I CC Natale which is my beta tester, he have some Alix boards running
>>>> 24/24
>>>> with my patch
>>>>
>>>> --- a/arch/x86/kernel/Makefile  2010-08-27 19:42:01.795858001 +0200
>>>> +++ b/arch/x86/kernel/Makefile  2010-08-27 19:42:12.525858001 +0200
>>>> @@ -88,6 +88,8 @@
>>>>  obj-$(CONFIG_APB_TIMER)               += apb_timer.o
>>>>
>>>>  obj-$(CONFIG_K8_NB)           += k8.o
>>>> +obj-$(CONFIG_GEODE_NOPL)       += nopl_emu.o
>>>> +obj-$(CONFIG_GEODE_NOPL)       += nopl_emu.o
>>>
>>> Same line added twice...
>>>
>>> --
>>> Thomas
>>>
>>
>>
>>
>> --
>> Matteo Croce
>> OpenWrt developer
>>   _______                     ________        __
>>  |       |.-----.-----.-----.|  |  |  |.----.|  |_
>>  |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
>>  |_______||   __|_____|__|__||________||__|  |____|
>>           |__| W I R E L E S S   F R E E D O M
>>  KAMIKAZE (bleeding edge) ------------------
>>   * 10 oz Vodka       Shake well with ice and strain
>>   * 10 oz Triple sec  mixture into 10 shot glasses.
>>   * 10 oz lime juice  Salute!
>>  ---------------------------------------------------
>>
>
> Here is with proper checks
>
> --- a/arch/x86/kernel/Makefile  2010-08-27 19:42:01.795858001 +0200
> +++ b/arch/x86/kernel/Makefile  2010-08-27 19:42:12.525858001 +0200
> @@ -88,6 +88,8 @@
>  obj-$(CONFIG_APB_TIMER)                += apb_timer.o
>
>  obj-$(CONFIG_K8_NB)            += k8.o
> +obj-$(CONFIG_GEODE_NOPL)       += nopl_emu.o
> +obj-$(CONFIG_GEODE_NOPL)       += nopl_emu.o
>  obj-$(CONFIG_DEBUG_RODATA_TEST)        += test_rodata.o
>  obj-$(CONFIG_DEBUG_NX_TEST)    += test_nx.o
>
> --- a/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:01.855858001 +0200
> +++ b/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:12.535858001 +0200
> @@ -137,11 +137,15 @@
>                return;
>        }
>
> +#ifdef CONFIG_GEODE_NOPL
>        if (c->x86_model == 10) {
> -               /* AMD Geode LX is model 10 */
> -               /* placeholder for any needed mods */
> +               /* Geode only lacks the NOPL instruction to be i686,
> +                  but we can promote it to a i686 class cpu
> +                  and emulate NOPLs in the exception handler*/
> +               boot_cpu_data.x86 = 6;
>                return;
>        }
> +#endif
>  }
>
>  static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
> --- a/arch/x86/kernel/entry_32.S        2010-08-27 19:42:01.735858001 +0200
> +++ b/arch/x86/kernel/entry_32.S        2010-08-27 19:42:12.535858001 +0200
> @@ -978,7 +978,11 @@
>        RING0_INT_FRAME
>        pushl $0
>        CFI_ADJUST_CFA_OFFSET 4
> +#ifdef CONFIG_GEODE_NOPL
> +       pushl $do_nopl_emu
> +#else
>        pushl $do_invalid_op
> +#endif
>        CFI_ADJUST_CFA_OFFSET 4
>        jmp error_code
>        CFI_ENDPROC
> --- /dev/null   1970-01-01 00:00:00.000000000 +0000
> +++ b/arch/x86/kernel/nopl_emu.c        2010-08-28 00:11:52.627085002 +0200
> @@ -0,0 +1,128 @@
> +/*
> + *  linux/arch/x86/kernel/nopl_emu.c
> + *
> + *  Copyright (C) 2002  Willy Tarreau
> + *  Copyright (C) 2010  Matteo Croce
> + */
> +
> +#include <asm/processor.h>
> +#include <asm/traps.h>
> +#include <asm/uaccess.h>
> +
> +/* This code can be used to allow the AMD Geode to hopefully correctly execute
> + * some code which was originally compiled for an i686, by emulating NOPL,
> + * the only missing i686 instruction in the CPU
> + *
> + * Willy Tarreau <willy@meta-x.org>
> + * Matteo Croce <technoboy85@gmail.com>
> + */
> +
> +static inline int do_1f(u8 *ip)
> +{
> +       u8 val1, val2;
> +       int length = 3;
> +       if (get_user(val1, ip))
> +               return 0;
> +       switch (val1) {
> +       case 0x84:
> +               get_user(val1, ip + 5);
> +               if (!val1)
> +                       length++;
> +               else
> +                       return 0;
> +       case 0x80:
> +               get_user(val1, ip + 4);
> +               get_user(val2, ip + 3);
> +               if (!val1 && !val2)
> +                       length += 2;
> +               else
> +                       return 0;
> +       case 0x44:
> +               get_user(val1, ip + 2);
> +               if (!val1)
> +                       length++;
> +               else
> +                       return 0;
> +       case 0x40:
> +               get_user(val1, ip + 1);
> +               if (!val1)
> +                       length++;
> +               else
> +                       return 0;
> +       case 0x00:
> +               return length;
> +       }
> +       return 0;
> +}
> +
> +static inline int do_0f(u8 *ip)
> +{
> +       u8 val;
> +       if (get_user(val, ip))
> +               return 0;
> +       if (val == 0x1f)
> +               return do_1f(ip + 1);
> +       return 0;
> +}
> +
> +static inline int do_66(u8 *ip)
> +{
> +       u8 val;
> +       if (get_user(val, ip))
> +               return 0;
> +       if (val == 0x90)
> +               return 2;
> +       if (val == 0x0f) {
> +               int res = do_0f(ip + 1);
> +               if (res)
> +                       return res + 1;
> +               else
> +                       return 0;
> +       }
> +       return 0;
> +}
> +
> +static inline int do_start(u8 *ip)
> +{
> +       u8 val;
> +       if (get_user(val, ip))
> +               return 0;
> +       if (val == 0x0f)
> +               return do_0f(ip + 1);
> +       if (val == 0x66)
> +               return do_66(ip + 1);
> +       return 0;
> +}
> +
> +/* [do_nopl_emu] is called by exception 6 after an invalid opcode has been
> + * encountered. It will try to emulate it by doing nothing,
> + * and will send a SIGILL or SIGSEGV to the process if not possible.
> + * the NOPL can have variable length opcodes:
> +
> +bytes number   opcode
> +       2       66 90
> +       3       0f 1f 00
> +       4       0f 1f 40 00
> +       5       0f 1f 44 00 00
> +       6       66 0f 1f 44 00 00
> +       7       0f 1f 80 00 00 00 00
> +       8       0f 1f 84 00 00 00 00 00
> +       9       66 0f 1f 84 00 00 00 00 00
> +*/
> +void do_nopl_emu(struct pt_regs *regs, long error_code)
> +{
> +       u8 *ip = (u8 *)instruction_pointer(regs);
> +       int res = do_start(ip);
> +
> +       if (res) {
> +               int i = 0;
> +               do {
> +                       ip += res;
> +                       i++;
> +                       res = do_start(ip);
> +               } while(res);
> +               printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
> +               regs->ip = (typeof(regs->ip))ip;
> +       } else
> +               do_invalid_op(regs, error_code);
> +}
>
>
> --
> Matteo Croce
> OpenWrt developer
>   _______                     ________        __
>  |       |.-----.-----.-----.|  |  |  |.----.|  |_
>  |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
>  |_______||   __|_____|__|__||________||__|  |____|
>           |__| W I R E L E S S   F R E E D O M
>  KAMIKAZE (bleeding edge) ------------------
>   * 10 oz Vodka       Shake well with ice and strain
>   * 10 oz Triple sec  mixture into 10 shot glasses.
>   * 10 oz lime juice  Salute!
>  ---------------------------------------------------
>

Sorry still left the duplicated line

--- a/arch/x86/kernel/Makefile 2010-08-27 19:42:01.795858001 +0200
+++ b/arch/x86/kernel/Makefile 2010-08-28 00:16:53.607507000 +0200
@@ -88,6 +88,7 @@
obj-$(CONFIG_APB_TIMER) += apb_timer.o

obj-$(CONFIG_K8_NB) += k8.o
+obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
obj-$(CONFIG_DEBUG_RODATA_TEST) += test_rodata.o
obj-$(CONFIG_DEBUG_NX_TEST) += test_nx.o

--- a/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:01.855858001 +0200
+++ b/arch/x86/kernel/cpu/amd.c 2010-08-27 19:42:12.535858001 +0200
@@ -137,11 +137,15 @@
return;
}

+#ifdef CONFIG_GEODE_NOPL
if (c->x86_model == 10) {
- /* AMD Geode LX is model 10 */
- /* placeholder for any needed mods */
+ /* Geode only lacks the NOPL instruction to be i686,
+ but we can promote it to a i686 class cpu
+ and emulate NOPLs in the exception handler*/
+ boot_cpu_data.x86 = 6;
return;
}
+#endif
}

static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
--- a/arch/x86/kernel/entry_32.S 2010-08-27 19:42:01.735858001 +0200
+++ b/arch/x86/kernel/entry_32.S 2010-08-27 19:42:12.535858001 +0200
@@ -978,7 +978,11 @@
RING0_INT_FRAME
pushl $0
CFI_ADJUST_CFA_OFFSET 4
+#ifdef CONFIG_GEODE_NOPL
+ pushl $do_nopl_emu
+#else
pushl $do_invalid_op
+#endif
CFI_ADJUST_CFA_OFFSET 4
jmp error_code
CFI_ENDPROC
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ b/arch/x86/kernel/nopl_emu.c 2010-08-28 00:11:52.627085002 +0200
@@ -0,0 +1,128 @@
+/*
+ * linux/arch/x86/kernel/nopl_emu.c
+ *
+ * Copyright (C) 2002 Willy Tarreau
+ * Copyright (C) 2010 Matteo Croce
+ */
+
+#include <asm/processor.h>
+#include <asm/traps.h>
+#include <asm/uaccess.h>
+
+/* This code can be used to allow the AMD Geode to hopefully correctly execute
+ * some code which was originally compiled for an i686, by emulating NOPL,
+ * the only missing i686 instruction in the CPU
+ *
+ * Willy Tarreau <willy@meta-x.org>
+ * Matteo Croce <technoboy85@gmail.com>
+ */
+
+static inline int do_1f(u8 *ip)
+{
+ u8 val1, val2;
+ int length = 3;
+ if (get_user(val1, ip))
+ return 0;
+ switch (val1) {
+ case 0x84:
+ get_user(val1, ip + 5);
+ if (!val1)
+ length++;
+ else
+ return 0;
+ case 0x80:
+ get_user(val1, ip + 4);
+ get_user(val2, ip + 3);
+ if (!val1 && !val2)
+ length += 2;
+ else
+ return 0;
+ case 0x44:
+ get_user(val1, ip + 2);
+ if (!val1)
+ length++;
+ else
+ return 0;
+ case 0x40:
+ get_user(val1, ip + 1);
+ if (!val1)
+ length++;
+ else
+ return 0;
+ case 0x00:
+ return length;
+ }
+ return 0;
+}
+
+static inline int do_0f(u8 *ip)
+{
+ u8 val;
+ if (get_user(val, ip))
+ return 0;
+ if (val == 0x1f)
+ return do_1f(ip + 1);
+ return 0;
+}
+
+static inline int do_66(u8 *ip)
+{
+ u8 val;
+ if (get_user(val, ip))
+ return 0;
+ if (val == 0x90)
+ return 2;
+ if (val == 0x0f) {
+ int res = do_0f(ip + 1);
+ if (res)
+ return res + 1;
+ else
+ return 0;
+ }
+ return 0;
+}
+
+static inline int do_start(u8 *ip)
+{
+ u8 val;
+ if (get_user(val, ip))
+ return 0;
+ if (val == 0x0f)
+ return do_0f(ip + 1);
+ if (val == 0x66)
+ return do_66(ip + 1);
+ return 0;
+}
+
+/* [do_nopl_emu] is called by exception 6 after an invalid opcode has been
+ * encountered. It will try to emulate it by doing nothing,
+ * and will send a SIGILL or SIGSEGV to the process if not possible.
+ * the NOPL can have variable length opcodes:
+
+bytes number opcode
+ 2 66 90
+ 3 0f 1f 00
+ 4 0f 1f 40 00
+ 5 0f 1f 44 00 00
+ 6 66 0f 1f 44 00 00
+ 7 0f 1f 80 00 00 00 00
+ 8 0f 1f 84 00 00 00 00 00
+ 9 66 0f 1f 84 00 00 00 00 00
+*/
+void do_nopl_emu(struct pt_regs *regs, long error_code)
+{
+ u8 *ip = (u8 *)instruction_pointer(regs);
+ int res = do_start(ip);
+
+ if (res) {
+ int i = 0;
+ do {
+ ip += res;
+ i++;
+ res = do_start(ip);
+ } while(res);
+ printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
+ regs->ip = (typeof(regs->ip))ip;
+ } else
+ do_invalid_op(regs, error_code);
+}


--
Matteo Croce
OpenWrt developer
  _______                     ________        __
 |       |.-----.-----.-----.|  |  |  |.----.|  |_
 |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
 |_______||   __|_____|__|__||________||__|  |____|
          |__| W I R E L E S S   F R E E D O M
 KAMIKAZE (bleeding edge) ------------------
  * 10 oz Vodka       Shake well with ice and strain
  * 10 oz Triple sec  mixture into 10 shot glasses.
  * 10 oz lime juice  Salute!
 ---------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On 08/27/2010 02:32 PM, Matteo Croce wrote:
> can I ignore the return value when I expect val to be non zero?
> the doc says: "On error, the variable @x is set to zero."

No. You need to deliver a page fault to the application in this case.

The *real* test for this kind of crap is correct page fault behavior,
and so forth.

Also, at the very least you need to check for:

- CS == USER_CS
- IP in the proper range for user space

Your patch in its current form is one big security hole.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On 08/28/2010 02:07 AM, H. Peter Anvin wrote:
> On 08/27/2010 02:32 PM, Matteo Croce wrote:
>> can I ignore the return value when I expect val to be non zero?
>> the doc says: "On error, the variable @x is set to zero."
> No. You need to deliver a page fault to the application in this case.
>
> The *real* test for this kind of crap is correct page fault behavior,
> and so forth.
>
> Also, at the very least you need to check for:
>
> - CS == USER_CS

Is that mandated by the ABI? Is it illegal for an application to load a
segment with a nonzero base into the LDT and use it for CS?

What about vm86?

--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sat, Aug 28, 2010 at 1:07 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 08/27/2010 02:32 PM, Matteo Croce wrote:
>> can I ignore the return value when I expect val to be non zero?
>> the doc says: "On error, the variable @x is set to zero."
>
> No.  You need to deliver a page fault to the application in this case.
>
> The *real* test for this kind of crap is correct page fault behavior,
> and so forth.
>
> Also, at the very least you need to check for:
>
> - CS == USER_CS
> - IP in the proper range for user space
>
> Your patch in its current form is one big security hole.
>
>        -hpa
>
> --
> H. Peter Anvin, Intel Open Source Technology Center
> I work for Intel.  I don't speak on their behalf.
>
>

If the parsing fails due get_user returning error I call
`do_invalid_op(regs, error_code);`
which is the default handler, which does the page fault.
to check the CS I do `regs->cs != __USER_CS` but how to check the IP value?
convert_ip_to_linear() and then check something?

--
Matteo Croce
OpenWrt developer
  _______                     ________        __
 |       |.-----.-----.-----.|  |  |  |.----.|  |_
 |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
 |_______||   __|_____|__|__||________||__|  |____|
          |__| W I R E L E S S   F R E E D O M
 KAMIKAZE (bleeding edge) ------------------
  * 10 oz Vodka       Shake well with ice and strain
  * 10 oz Triple sec  mixture into 10 shot glasses.
  * 10 oz lime juice  Salute!
 ---------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

In my opinion, this patch shouldn't be seen as a solution in the slightest. To me, it's a ugly hack and a kludge at best!

Abstractly a NOP is meant to be exactly as it says on the tin, a No Operation.

It's meant to do nothing at all - for a predefined number of clock cycles.
A NOP is commonly used for timing purposes, this completely breaks that contract surely?

NOPL is not standard i686, it was undocumented and has just been de facto supported since the Pentium Pro.

The solution is surely to ensure that when something is compiled for generic i686, NOPL is nowhere to be seen... Any compiler that does otherwise is broken.

For example, until recently, the basic semantics for choosing the NOP sequence were completely wrong in binutils. This has, finally, been fixed!

http://sourceware.org/ml/binutils-cvs/2010-08/msg00057.html
http://gcc.gnu.org/ml/gcc/2010-08/msg00194.html

On 27 Aug 2010, at 19:10, Matteo Croce wrote:

> Hi,
> I have received many mails asking to refresh the patch so I decided to
> post them on the public mailing list
> In this version such feature is configurable via a kernel config option
>
> Signed-off-by: Matteo Croce <matteo@openwrt.org>
>
> --- a/arch/x86/kernel/Makefile 2010-08-27 00:27:50.101261000 +0200
> +++ b/arch/x86/kernel/Makefile 2010-08-27 00:31:11.391261002 +0200
> @@ -88,6 +88,8 @@
> obj-$(CONFIG_APB_TIMER) += apb_timer.o
>
> obj-$(CONFIG_K8_NB) += k8.o
> +obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
> +obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
> obj-$(CONFIG_DEBUG_RODATA_TEST) += test_rodata.o
> obj-$(CONFIG_DEBUG_NX_TEST) += test_nx.o
>
> --- a/arch/x86/kernel/cpu/amd.c 2010-08-27 00:27:50.161261000 +0200
> +++ b/arch/x86/kernel/cpu/amd.c 2010-08-27 00:34:13.371261000 +0200
> @@ -137,11 +137,15 @@
> return;
> }
>
> +#ifdef CONFIG_GEODE_NOPL
> if (c->x86_model == 10) {
> - /* AMD Geode LX is model 10 */
> - /* placeholder for any needed mods */
> + /* Geode only lacks the NOPL instruction to be i686,
> + but we can promote it to a i686 class cpu
> + and emulate NOPLs in the exception handler*/
> + boot_cpu_data.x86 = 6;
> return;
> }
> +#endif
> }
>
> static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
> --- a/arch/x86/kernel/entry_32.S 2010-08-27 00:27:50.051261000 +0200
> +++ b/arch/x86/kernel/entry_32.S 2010-08-27 00:57:35.531261000 +0200
> @@ -978,7 +978,11 @@
> RING0_INT_FRAME
> pushl $0
> CFI_ADJUST_CFA_OFFSET 4
> +#ifdef CONFIG_GEODE_NOPL
> + pushl $do_nopl_emu
> +#else
> pushl $do_invalid_op
> +#endif
> CFI_ADJUST_CFA_OFFSET 4
> jmp error_code
> CFI_ENDPROC
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ b/arch/x86/kernel/nopl_emu.c 2010-08-27 00:27:57.881261002 +0200
> @@ -0,0 +1,110 @@
> +/*
> + * linux/arch/x86/kernel/nopl_emu.c
> + *
> + * Copyright (C) 2002 Willy Tarreau
> + * Copyright (C) 2010 Matteo Croce
> + */
> +
> +#include <asm/processor.h>
> +#include <asm/traps.h>
> +
> +/* This code can be used to allow the AMD Geode to hopefully correctly execute
> + * some code which was originally compiled for an i686, by emulating NOPL,
> + * the only missing i686 instruction in the CPU
> + *
> + * Willy Tarreau <willy@meta-x.org>
> + * Matteo Croce <technoboy85@gmail.co>
> + */
> +
> +static inline int do_1f(u8 *ip)
> +{
> + int length = 3;
> + switch (*ip) {
> + case 0x84:
> + if (!ip[5])
> + length++;
> + else
> + return 0;
> + case 0x80:
> + if (!ip[4] && !ip[3])
> + length += 2;
> + else
> + return 0;
> + case 0x44:
> + if (!ip[2])
> + length++;
> + else
> + return 0;
> + case 0x40:
> + if (!ip[1])
> + length++;
> + else
> + return 0;
> + case 0x00:
> + return length;
> + }
> + return 0;
> +}
> +
> +static inline int do_0f(u8 *ip)
> +{
> + if (*ip == 0x1f)
> + return do_1f(ip + 1);
> + return 0;
> +}
> +
> +static inline int do_66(u8 *ip)
> +{
> + if (*ip == 0x90)
> + return 2;
> + if (*ip == 0x0f) {
> + int res = do_0f(ip + 1);
> + if (res)
> + return res + 1;
> + else
> + return 0;
> + }
> + return 0;
> +}
> +
> +static inline int do_start(u8 *ip)
> +{
> + if (*ip == 0x0f)
> + return do_0f(ip + 1);
> + if (*ip == 0x66)
> + return do_66(ip + 1);
> + return 0;
> +}
> +
> +/* [do_nopl_emu] is called by exception 6 after an invalid opcode has been
> + * encountered. It will try to emulate it by doing nothing,
> + * and will send a SIGILL or SIGSEGV to the process if not possible.
> + * the NOPL can have variable length opcodes:
> +
> +bytes number opcode
> + 2 66 90
> + 3 0f 1f 00
> + 4 0f 1f 40 00
> + 5 0f 1f 44 00 00
> + 6 66 0f 1f 44 00 00
> + 7 0f 1f 80 00 00 00 00
> + 8 0f 1f 84 00 00 00 00 00
> + 9 66 0f 1f 84 00 00 00 00 00
> +*/
> +void do_nopl_emu(struct pt_regs *regs, long error_code)
> +{
> + u8 *ip = (u8 *)instruction_pointer(regs);
> + int res = do_start(ip);
> +
> + if (res) {
> + int i = 0;
> + do {
> + ip += res;
> + i++;
> + res = do_start(ip);
> + } while(res);
> + printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
> + regs->ip = (typeof(regs->ip))ip;
> + } else
> + do_invalid_op(regs, error_code);
> +}
>
>
> --
> Matteo Croce
> OpenWrt developer
> Â _______Â Â Â Â Â Â Â Â Â Â Â ________Â Â Â Â __
> Â |Â Â Â Â |.-----.-----.-----.|Â |Â |Â |.----.|Â |_
> Â |Â Â -Â Â ||Â _Â |Â -__|Â Â Â ||Â |Â |Â ||Â Â _||Â Â _|
> Â |_______||Â Â __|_____|__|__||________||__|Â |____|
> Â Â Â Â Â |__| W I R E L E S SÂ Â F R E E D O M
> Â KAMIKAZE (bleeding edge) ------------------
>  * 10 oz Vodka    Shake well with ice and strain
>  * 10 oz Triple sec mixture into 10 shot glasses.
>  * 10 oz lime juice Salute!
> Â ---------------------------------------------------
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tuesday 07 September 2010, 17:57:17 Nick Lowe wrote:
> In my opinion, this patch shouldn't be seen as a solution in the
> slightest. To me, it's a ugly hack and a kludge at best!
>
> Abstractly a NOP is meant to be exactly as it says on the tin, a No
> Operation.
>
> It's meant to do nothing at all - for a predefined number of clock
> cycles. A NOP is commonly used for timing purposes, this completely
> breaks that contract surely?
>
> NOPL is not standard i686, it was undocumented and has just been de facto
> supported since the Pentium Pro.
>
> The solution is surely to ensure that when something is compiled for
> generic i686, NOPL is nowhere to be seen... Any compiler that does
> otherwise is broken.

From a user POV, this patch done right is the better solution, because you
cannot ensure, that all code _generally_ comply to your *policy*. This is
not a kernel only issue! Keep processes crashing is worse IMHO.

With properly done, I mean:
- runtime check, if iopl opcode exists
- emulate, if not

Hence, don't bind this to nor label it after a certain cpu model.

Remember: we do not live in a perfect world.

Pete

> For example, until recently, the basic semantics for choosing the NOP
> sequence were completely wrong in binutils. This has, finally, been
> fixed!
>
> http://sourceware.org/ml/binutils-cvs/2010-08/msg00057.html
> http://gcc.gnu.org/ml/gcc/2010-08/msg00194.html
>
> On 27 Aug 2010, at 19:10, Matteo Croce wrote:
> > Hi,
> > I have received many mails asking to refresh the patch so I decided to
> > post them on the public mailing list
> > In this version such feature is configurable via a kernel config option
> >
> > Signed-off-by: Matteo Croce <matteo@openwrt.org>
> >
> > --- a/arch/x86/kernel/Makefile 2010-08-27 00:27:50.101261000 +0200
> > +++ b/arch/x86/kernel/Makefile 2010-08-27 00:31:11.391261002 +0200
> > @@ -88,6 +88,8 @@
> > obj-$(CONFIG_APB_TIMER) += apb_timer.o
> >
> > obj-$(CONFIG_K8_NB) += k8.o
> > +obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
> > +obj-$(CONFIG_GEODE_NOPL) += nopl_emu.o
> > obj-$(CONFIG_DEBUG_RODATA_TEST) += test_rodata.o
> > obj-$(CONFIG_DEBUG_NX_TEST) += test_nx.o
> >
> > --- a/arch/x86/kernel/cpu/amd.c 2010-08-27 00:27:50.161261000 +0200
> > +++ b/arch/x86/kernel/cpu/amd.c 2010-08-27 00:34:13.371261000 +0200
> > @@ -137,11 +137,15 @@
> > return;
> > }
> >
> > +#ifdef CONFIG_GEODE_NOPL
> > if (c->x86_model == 10) {
> > - /* AMD Geode LX is model 10 */
> > - /* placeholder for any needed mods */
> > + /* Geode only lacks the NOPL instruction to be i686,
> > + but we can promote it to a i686 class cpu
> > + and emulate NOPLs in the exception handler*/
> > + boot_cpu_data.x86 = 6;
> > return;
> > }
> > +#endif
> > }
> >
> > static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
> > --- a/arch/x86/kernel/entry_32.S 2010-08-27 00:27:50.051261000 +0200
> > +++ b/arch/x86/kernel/entry_32.S 2010-08-27 00:57:35.531261000 +0200
> > @@ -978,7 +978,11 @@
> > RING0_INT_FRAME
> > pushl $0
> > CFI_ADJUST_CFA_OFFSET 4
> > +#ifdef CONFIG_GEODE_NOPL
> > + pushl $do_nopl_emu
> > +#else
> > pushl $do_invalid_op
> > +#endif
> > CFI_ADJUST_CFA_OFFSET 4
> > jmp error_code
> > CFI_ENDPROC
> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> > +++ b/arch/x86/kernel/nopl_emu.c 2010-08-27 00:27:57.881261002 +0200
> > @@ -0,0 +1,110 @@
> > +/*
> > + * linux/arch/x86/kernel/nopl_emu.c
> > + *
> > + * Copyright (C) 2002 Willy Tarreau
> > + * Copyright (C) 2010 Matteo Croce
> > + */
> > +
> > +#include <asm/processor.h>
> > +#include <asm/traps.h>
> > +
> > +/* This code can be used to allow the AMD Geode to hopefully correctly
> > execute + * some code which was originally compiled for an i686, by
> > emulating NOPL, + * the only missing i686 instruction in the CPU
> > + *
> > + * Willy Tarreau <willy@meta-x.org>
> > + * Matteo Croce <technoboy85@gmail.co>
> > + */
> > +
> > +static inline int do_1f(u8 *ip)
> > +{
> > + int length = 3;
> > + switch (*ip) {
> > + case 0x84:
> > + if (!ip[5])
> > + length++;
> > + else
> > + return 0;
> > + case 0x80:
> > + if (!ip[4] && !ip[3])
> > + length += 2;
> > + else
> > + return 0;
> > + case 0x44:
> > + if (!ip[2])
> > + length++;
> > + else
> > + return 0;
> > + case 0x40:
> > + if (!ip[1])
> > + length++;
> > + else
> > + return 0;
> > + case 0x00:
> > + return length;
> > + }
> > + return 0;
> > +}
> > +
> > +static inline int do_0f(u8 *ip)
> > +{
> > + if (*ip == 0x1f)
> > + return do_1f(ip + 1);
> > + return 0;
> > +}
> > +
> > +static inline int do_66(u8 *ip)
> > +{
> > + if (*ip == 0x90)
> > + return 2;
> > + if (*ip == 0x0f) {
> > + int res = do_0f(ip + 1);
> > + if (res)
> > + return res + 1;
> > + else
> > + return 0;
> > + }
> > + return 0;
> > +}
> > +
> > +static inline int do_start(u8 *ip)
> > +{
> > + if (*ip == 0x0f)
> > + return do_0f(ip + 1);
> > + if (*ip == 0x66)
> > + return do_66(ip + 1);
> > + return 0;
> > +}
> > +
> > +/* [do_nopl_emu] is called by exception 6 after an invalid opcode has
> > been + * encountered. It will try to emulate it by doing nothing,
> > + * and will send a SIGILL or SIGSEGV to the process if not possible.
> > + * the NOPL can have variable length opcodes:
> > +
> > +bytes number opcode
> > + 2 66 90
> > + 3 0f 1f 00
> > + 4 0f 1f 40 00
> > + 5 0f 1f 44 00 00
> > + 6 66 0f 1f 44 00 00
> > + 7 0f 1f 80 00 00 00 00
> > + 8 0f 1f 84 00 00 00 00 00
> > + 9 66 0f 1f 84 00 00 00 00 00
> > +*/
> > +void do_nopl_emu(struct pt_regs *regs, long error_code)
> > +{
> > + u8 *ip = (u8 *)instruction_pointer(regs);
> > + int res = do_start(ip);
> > +
> > + if (res) {
> > + int i = 0;
> > + do {
> > + ip += res;
> > + i++;
> > + res = do_start(ip);
> > + } while(res);
> > + printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
> > + regs->ip = (typeof(regs->ip))ip;
> > + } else
> > + do_invalid_op(regs, error_code);
> > +}
> >
> >
> > --
> > Matteo Croce
> > OpenWrt developer
> > Â _______Â Â Â Â Â Â Â Â Â Â Â ________Â Â Â Â __
> > Â |Â Â Â Â |.-----.-----.-----.|Â |Â |Â |.----.|Â |_
> > Â |Â Â -Â Â ||Â _Â |Â -__|Â Â Â ||Â |Â |Â ||Â Â _||Â Â _|
> > Â |_______||Â Â __|_____|__|__||________||__|Â |____|
> > Â Â Â Â Â |__| W I R E L E S SÂ Â F R E E D O M
> > Â KAMIKAZE (bleeding edge) ------------------
> >  * 10 oz Vodka    Shake well with ice and strain
> >  * 10 oz Triple sec mixture into 10 shot glasses.
> >  * 10 oz lime juice Salute!
> > Â ---------------------------------------------------
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> > in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi,

If a process crashes because of NOPL usage, you get the feedback loop to fix it.

The beauty of open source is that we can recompile to correct the bad
assumption; the latest version of binutils (GAS) finally corrects the
bad semantic that generic i686 includes NOPL.

http://sourceware.org/ml/binutils-cvs/2010-08/msg00057.html
http://gcc.gnu.org/ml/gcc/2010-08/msg00194.html

That fixes the vast vast majority of cases, so I really don't see the
need for this.

I've posted a RFC on "Promoting Crusoe and Geode Processors to i686
Status" if you have any thoughts on that?

Cheers,

Nick

On Wed, Sep 8, 2010 at 10:15 AM, Hans-Peter Jansen <hpj@urpla.net> wrote:
> On Tuesday 07 September 2010, 17:57:17 Nick Lowe wrote:
>> In my opinion, this patch shouldn't be seen as a solution in the
>> slightest. To me, it's a ugly hack and a kludge at best!
>>
>> Abstractly a NOP is meant to be exactly as it says on the tin, a No
>> Operation.
>>
>> It's meant to do nothing at all - for a predefined number of clock
>> cycles. A NOP is commonly used for timing purposes, this completely
>> breaks that contract surely?
>>
>> NOPL is not standard i686, it was undocumented and has just been de facto
>> supported since the Pentium Pro.
>>
>> The solution is surely to ensure that when something is compiled for
>> generic i686, NOPL is nowhere to be seen... Any compiler that does
>> otherwise is broken.
>
> From a user POV, this patch done right is the better solution, because you
> cannot ensure, that all code _generally_ comply to your *policy*. This is
> not a kernel only issue! Keep processes crashing is worse IMHO.
>
> With properly done, I mean:
>  - runtime check, if iopl opcode exists
>  - emulate, if not
>
> Hence, don't bind this to nor label it after a certain cpu model.
>
> Remember: we do not live in a perfect world.
>
> Pete
>
>> For example, until recently, the basic semantics for choosing the NOP
>> sequence were completely wrong in binutils. This has, finally, been
>> fixed!
>>
>> http://sourceware.org/ml/binutils-cvs/2010-08/msg00057.html
>> http://gcc.gnu.org/ml/gcc/2010-08/msg00194.html
>>
>> On 27 Aug 2010, at 19:10, Matteo Croce wrote:
>> > Hi,
>> > I have received many mails asking to refresh the patch so I decided to
>> > post them on the public mailing list
>> > In this version such feature is configurable via a kernel config option
>> >
>> > Signed-off-by: Matteo Croce <matteo@openwrt.org>
>> >
>> > --- a/arch/x86/kernel/Makefile      2010-08-27 00:27:50.101261000 +0200
>> > +++ b/arch/x86/kernel/Makefile      2010-08-27 00:31:11.391261002 +0200
>> > @@ -88,6 +88,8 @@
>> > obj-$(CONFIG_APB_TIMER)             += apb_timer.o
>> >
>> > obj-$(CONFIG_K8_NB)         += k8.o
>> > +obj-$(CONFIG_GEODE_NOPL)   += nopl_emu.o
>> > +obj-$(CONFIG_GEODE_NOPL)   += nopl_emu.o
>> > obj-$(CONFIG_DEBUG_RODATA_TEST)     += test_rodata.o
>> > obj-$(CONFIG_DEBUG_NX_TEST) += test_nx.o
>> >
>> > --- a/arch/x86/kernel/cpu/amd.c     2010-08-27 00:27:50.161261000 +0200
>> > +++ b/arch/x86/kernel/cpu/amd.c     2010-08-27 00:34:13.371261000 +0200
>> > @@ -137,11 +137,15 @@
>> >             return;
>> >     }
>> >
>> > +#ifdef CONFIG_GEODE_NOPL
>> >     if (c->x86_model == 10) {
>> > -           /* AMD Geode LX is model 10 */
>> > -           /* placeholder for any needed mods */
>> > +           /* Geode only lacks the NOPL instruction to be i686,
>> > +              but we can promote it to a i686 class cpu
>> > +              and emulate NOPLs in the exception handler*/
>> > +           boot_cpu_data.x86 = 6;
>> >             return;
>> >     }
>> > +#endif
>> > }
>> >
>> > static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
>> > --- a/arch/x86/kernel/entry_32.S    2010-08-27 00:27:50.051261000 +0200
>> > +++ b/arch/x86/kernel/entry_32.S    2010-08-27 00:57:35.531261000 +0200
>> > @@ -978,7 +978,11 @@
>> >     RING0_INT_FRAME
>> >     pushl $0
>> >     CFI_ADJUST_CFA_OFFSET 4
>> > +#ifdef CONFIG_GEODE_NOPL
>> > +   pushl $do_nopl_emu
>> > +#else
>> >     pushl $do_invalid_op
>> > +#endif
>> >     CFI_ADJUST_CFA_OFFSET 4
>> >     jmp error_code
>> >     CFI_ENDPROC
>> > --- /dev/null       1970-01-01 00:00:00.000000000 +0000
>> > +++ b/arch/x86/kernel/nopl_emu.c    2010-08-27 00:27:57.881261002 +0200
>> > @@ -0,0 +1,110 @@
>> > +/*
>> > + *  linux/arch/x86/kernel/nopl_emu.c
>> > + *
>> > + *  Copyright (C) 2002  Willy Tarreau
>> > + *  Copyright (C) 2010  Matteo Croce
>> > + */
>> > +
>> > +#include <asm/processor.h>
>> > +#include <asm/traps.h>
>> > +
>> > +/* This code can be used to allow the AMD Geode to hopefully correctly
>> > execute + * some code which was originally compiled for an i686, by
>> > emulating NOPL, + * the only missing i686 instruction in the CPU
>> > + *
>> > + * Willy Tarreau <willy@meta-x.org>
>> > + * Matteo Croce <technoboy85@gmail.co>
>> > + */
>> > +
>> > +static inline int do_1f(u8 *ip)
>> > +{
>> > +   int length = 3;
>> > +   switch (*ip) {
>> > +   case 0x84:
>> > +           if (!ip[5])
>> > +                   length++;
>> > +           else
>> > +                   return 0;
>> > +   case 0x80:
>> > +           if (!ip[4] && !ip[3])
>> > +                   length += 2;
>> > +           else
>> > +                   return 0;
>> > +   case 0x44:
>> > +           if (!ip[2])
>> > +                   length++;
>> > +           else
>> > +                   return 0;
>> > +   case 0x40:
>> > +           if (!ip[1])
>> > +                   length++;
>> > +           else
>> > +                   return 0;
>> > +   case 0x00:
>> > +           return length;
>> > +   }
>> > +   return 0;
>> > +}
>> > +
>> > +static inline int do_0f(u8 *ip)
>> > +{
>> > +   if (*ip == 0x1f)
>> > +           return do_1f(ip + 1);
>> > +   return 0;
>> > +}
>> > +
>> > +static inline int do_66(u8 *ip)
>> > +{
>> > +   if (*ip == 0x90)
>> > +           return 2;
>> > +   if (*ip == 0x0f) {
>> > +           int res = do_0f(ip + 1);
>> > +           if (res)
>> > +                   return res + 1;
>> > +           else
>> > +                   return 0;
>> > +   }
>> > +   return 0;
>> > +}
>> > +
>> > +static inline int do_start(u8 *ip)
>> > +{
>> > +   if (*ip == 0x0f)
>> > +           return do_0f(ip + 1);
>> > +   if (*ip == 0x66)
>> > +           return do_66(ip + 1);
>> > +   return 0;
>> > +}
>> > +
>> > +/* [do_nopl_emu] is called by exception 6 after an invalid opcode has
>> > been + * encountered. It will try to emulate it by doing nothing,
>> > + * and will send a SIGILL or SIGSEGV to the process if not possible.
>> > + * the NOPL can have variable length opcodes:
>> > +
>> > +bytes number       opcode
>> > +   2       66 90
>> > +   3       0f 1f 00
>> > +   4       0f 1f 40 00
>> > +   5       0f 1f 44 00 00
>> > +   6       66 0f 1f 44 00 00
>> > +   7       0f 1f 80 00 00 00 00
>> > +   8       0f 1f 84 00 00 00 00 00
>> > +   9       66 0f 1f 84 00 00 00 00 00
>> > +*/
>> > +void do_nopl_emu(struct pt_regs *regs, long error_code)
>> > +{
>> > +   u8 *ip = (u8 *)instruction_pointer(regs);
>> > +   int res = do_start(ip);
>> > +
>> > +   if (res) {
>> > +           int i = 0;
>> > +           do {
>> > +                   ip += res;
>> > +                   i++;
>> > +                   res = do_start(ip);
>> > +           } while(res);
>> > +           printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
>> > +           regs->ip = (typeof(regs->ip))ip;
>> > +   } else
>> > +           do_invalid_op(regs, error_code);
>> > +}
>> >
>> >
>> > --
>> > Matteo Croce
>> > OpenWrt developer
>> > Â  _______Â  Â  Â  Â  Â  Â  Â  Â  Â  Â Â  ________Â  Â  Â  Â  __
>> > Â |Â  Â  Â Â  |.-----.-----.-----.|Â  |Â  |Â  |.----.|Â  |_
>> > Â |Â Â  -Â Â  ||Â  _Â  |Â  -__|Â  Â Â  ||Â  |Â  |Â  ||Â Â  _||Â Â  _|
>> > Â |_______||Â Â  __|_____|__|__||________||__|Â  |____|
>> > Â  Â  Â  Â  Â  |__| W I R E L E S SÂ Â  F R E E D O M
>> > Â KAMIKAZE (bleeding edge) ------------------
>> >   * 10 oz Vodka  Â  Â   Shake well with ice and strain
>> >   * 10 oz Triple sec  mixture into 10 shot glasses.
>> >   * 10 oz lime juice  Salute!
>> > Â ---------------------------------------------------
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>> > in the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>> in the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I also really don't think that you can get away with emulating it.
You're changing the contract of NOPL to something non-deterministic.
How is that acceptable?

What about the situation where these NOPs have been used to
synchronise? With the change in contract, you subtly break execution
there.

That's far worse than it not running in the first place, surely?

Nick

On Wed, Sep 8, 2010 at 12:34 PM, Nick Lowe <nick.lowe@gmail.com> wrote:
> Hi,
>
> If a process crashes because of NOPL usage, you get the feedback loop to fix it.
>
> The beauty of open source is that we can recompile to correct the bad
> assumption; the latest version of binutils (GAS) finally corrects the
> bad semantic that generic i686 includes NOPL.
>
> http://sourceware.org/ml/binutils-cvs/2010-08/msg00057.html
> http://gcc.gnu.org/ml/gcc/2010-08/msg00194.html
>
> That fixes the vast vast majority of cases, so I really don't see the
> need for this.
>
> I've posted a RFC on "Promoting Crusoe and Geode Processors to i686
> Status" if you have any thoughts on that?
>
> Cheers,
>
> Nick
>
> On Wed, Sep 8, 2010 at 10:15 AM, Hans-Peter Jansen <hpj@urpla.net> wrote:
>> On Tuesday 07 September 2010, 17:57:17 Nick Lowe wrote:
>>> In my opinion, this patch shouldn't be seen as a solution in the
>>> slightest. To me, it's a ugly hack and a kludge at best!
>>>
>>> Abstractly a NOP is meant to be exactly as it says on the tin, a No
>>> Operation.
>>>
>>> It's meant to do nothing at all - for a predefined number of clock
>>> cycles. A NOP is commonly used for timing purposes, this completely
>>> breaks that contract surely?
>>>
>>> NOPL is not standard i686, it was undocumented and has just been de facto
>>> supported since the Pentium Pro.
>>>
>>> The solution is surely to ensure that when something is compiled for
>>> generic i686, NOPL is nowhere to be seen... Any compiler that does
>>> otherwise is broken.
>>
>> From a user POV, this patch done right is the better solution, because you
>> cannot ensure, that all code _generally_ comply to your *policy*. This is
>> not a kernel only issue! Keep processes crashing is worse IMHO.
>>
>> With properly done, I mean:
>>  - runtime check, if iopl opcode exists
>>  - emulate, if not
>>
>> Hence, don't bind this to nor label it after a certain cpu model.
>>
>> Remember: we do not live in a perfect world.
>>
>> Pete
>>
>>> For example, until recently, the basic semantics for choosing the NOP
>>> sequence were completely wrong in binutils. This has, finally, been
>>> fixed!
>>>
>>> http://sourceware.org/ml/binutils-cvs/2010-08/msg00057.html
>>> http://gcc.gnu.org/ml/gcc/2010-08/msg00194.html
>>>
>>> On 27 Aug 2010, at 19:10, Matteo Croce wrote:
>>> > Hi,
>>> > I have received many mails asking to refresh the patch so I decided to
>>> > post them on the public mailing list
>>> > In this version such feature is configurable via a kernel config option
>>> >
>>> > Signed-off-by: Matteo Croce <matteo@openwrt.org>
>>> >
>>> > --- a/arch/x86/kernel/Makefile      2010-08-27 00:27:50.101261000 +0200
>>> > +++ b/arch/x86/kernel/Makefile      2010-08-27 00:31:11.391261002 +0200
>>> > @@ -88,6 +88,8 @@
>>> > obj-$(CONFIG_APB_TIMER)             += apb_timer.o
>>> >
>>> > obj-$(CONFIG_K8_NB)         += k8.o
>>> > +obj-$(CONFIG_GEODE_NOPL)   += nopl_emu.o
>>> > +obj-$(CONFIG_GEODE_NOPL)   += nopl_emu.o
>>> > obj-$(CONFIG_DEBUG_RODATA_TEST)     += test_rodata.o
>>> > obj-$(CONFIG_DEBUG_NX_TEST) += test_nx.o
>>> >
>>> > --- a/arch/x86/kernel/cpu/amd.c     2010-08-27 00:27:50.161261000 +0200
>>> > +++ b/arch/x86/kernel/cpu/amd.c     2010-08-27 00:34:13.371261000 +0200
>>> > @@ -137,11 +137,15 @@
>>> >             return;
>>> >     }
>>> >
>>> > +#ifdef CONFIG_GEODE_NOPL
>>> >     if (c->x86_model == 10) {
>>> > -           /* AMD Geode LX is model 10 */
>>> > -           /* placeholder for any needed mods */
>>> > +           /* Geode only lacks the NOPL instruction to be i686,
>>> > +              but we can promote it to a i686 class cpu
>>> > +              and emulate NOPLs in the exception handler*/
>>> > +           boot_cpu_data.x86 = 6;
>>> >             return;
>>> >     }
>>> > +#endif
>>> > }
>>> >
>>> > static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
>>> > --- a/arch/x86/kernel/entry_32.S    2010-08-27 00:27:50.051261000 +0200
>>> > +++ b/arch/x86/kernel/entry_32.S    2010-08-27 00:57:35.531261000 +0200
>>> > @@ -978,7 +978,11 @@
>>> >     RING0_INT_FRAME
>>> >     pushl $0
>>> >     CFI_ADJUST_CFA_OFFSET 4
>>> > +#ifdef CONFIG_GEODE_NOPL
>>> > +   pushl $do_nopl_emu
>>> > +#else
>>> >     pushl $do_invalid_op
>>> > +#endif
>>> >     CFI_ADJUST_CFA_OFFSET 4
>>> >     jmp error_code
>>> >     CFI_ENDPROC
>>> > --- /dev/null       1970-01-01 00:00:00.000000000 +0000
>>> > +++ b/arch/x86/kernel/nopl_emu.c    2010-08-27 00:27:57.881261002 +0200
>>> > @@ -0,0 +1,110 @@
>>> > +/*
>>> > + *  linux/arch/x86/kernel/nopl_emu.c
>>> > + *
>>> > + *  Copyright (C) 2002  Willy Tarreau
>>> > + *  Copyright (C) 2010  Matteo Croce
>>> > + */
>>> > +
>>> > +#include <asm/processor.h>
>>> > +#include <asm/traps.h>
>>> > +
>>> > +/* This code can be used to allow the AMD Geode to hopefully correctly
>>> > execute + * some code which was originally compiled for an i686, by
>>> > emulating NOPL, + * the only missing i686 instruction in the CPU
>>> > + *
>>> > + * Willy Tarreau <willy@meta-x.org>
>>> > + * Matteo Croce <technoboy85@gmail.co>
>>> > + */
>>> > +
>>> > +static inline int do_1f(u8 *ip)
>>> > +{
>>> > +   int length = 3;
>>> > +   switch (*ip) {
>>> > +   case 0x84:
>>> > +           if (!ip[5])
>>> > +                   length++;
>>> > +           else
>>> > +                   return 0;
>>> > +   case 0x80:
>>> > +           if (!ip[4] && !ip[3])
>>> > +                   length += 2;
>>> > +           else
>>> > +                   return 0;
>>> > +   case 0x44:
>>> > +           if (!ip[2])
>>> > +                   length++;
>>> > +           else
>>> > +                   return 0;
>>> > +   case 0x40:
>>> > +           if (!ip[1])
>>> > +                   length++;
>>> > +           else
>>> > +                   return 0;
>>> > +   case 0x00:
>>> > +           return length;
>>> > +   }
>>> > +   return 0;
>>> > +}
>>> > +
>>> > +static inline int do_0f(u8 *ip)
>>> > +{
>>> > +   if (*ip == 0x1f)
>>> > +           return do_1f(ip + 1);
>>> > +   return 0;
>>> > +}
>>> > +
>>> > +static inline int do_66(u8 *ip)
>>> > +{
>>> > +   if (*ip == 0x90)
>>> > +           return 2;
>>> > +   if (*ip == 0x0f) {
>>> > +           int res = do_0f(ip + 1);
>>> > +           if (res)
>>> > +                   return res + 1;
>>> > +           else
>>> > +                   return 0;
>>> > +   }
>>> > +   return 0;
>>> > +}
>>> > +
>>> > +static inline int do_start(u8 *ip)
>>> > +{
>>> > +   if (*ip == 0x0f)
>>> > +           return do_0f(ip + 1);
>>> > +   if (*ip == 0x66)
>>> > +           return do_66(ip + 1);
>>> > +   return 0;
>>> > +}
>>> > +
>>> > +/* [do_nopl_emu] is called by exception 6 after an invalid opcode has
>>> > been + * encountered. It will try to emulate it by doing nothing,
>>> > + * and will send a SIGILL or SIGSEGV to the process if not possible.
>>> > + * the NOPL can have variable length opcodes:
>>> > +
>>> > +bytes number       opcode
>>> > +   2       66 90
>>> > +   3       0f 1f 00
>>> > +   4       0f 1f 40 00
>>> > +   5       0f 1f 44 00 00
>>> > +   6       66 0f 1f 44 00 00
>>> > +   7       0f 1f 80 00 00 00 00
>>> > +   8       0f 1f 84 00 00 00 00 00
>>> > +   9       66 0f 1f 84 00 00 00 00 00
>>> > +*/
>>> > +void do_nopl_emu(struct pt_regs *regs, long error_code)
>>> > +{
>>> > +   u8 *ip = (u8 *)instruction_pointer(regs);
>>> > +   int res = do_start(ip);
>>> > +
>>> > +   if (res) {
>>> > +           int i = 0;
>>> > +           do {
>>> > +                   ip += res;
>>> > +                   i++;
>>> > +                   res = do_start(ip);
>>> > +           } while(res);
>>> > +           printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
>>> > +           regs->ip = (typeof(regs->ip))ip;
>>> > +   } else
>>> > +           do_invalid_op(regs, error_code);
>>> > +}
>>> >
>>> >
>>> > --
>>> > Matteo Croce
>>> > OpenWrt developer
>>> > Â  _______Â  Â  Â  Â  Â  Â  Â  Â  Â  Â Â  ________Â  Â  Â  Â  __
>>> > Â |Â  Â  Â Â  |.-----.-----.-----.|Â  |Â  |Â  |.----.|Â  |_
>>> > Â |Â Â  -Â Â  ||Â  _Â  |Â  -__|Â  Â Â  ||Â  |Â  |Â  ||Â Â  _||Â Â  _|
>>> > Â |_______||Â Â  __|_____|__|__||________||__|Â  |____|
>>> > Â  Â  Â  Â  Â  |__| W I R E L E S SÂ Â  F R E E D O M
>>> > Â KAMIKAZE (bleeding edge) ------------------
>>> >   * 10 oz Vodka  Â  Â   Shake well with ice and strain
>>> >   * 10 oz Triple sec  mixture into 10 shot glasses.
>>> >   * 10 oz lime juice  Salute!
>>> > Â ---------------------------------------------------
>>> > --
>>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>> > in the body of a message to majordomo@vger.kernel.org
>>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> > Please read the FAQ at  http://www.tux.org/lkml/
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>> in the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi,

It would only be through back-porting a form of this patch that it
would affect existing distributions, surely?

Why should it make it to the stable tree and a new kernel, though? The
chances are that if a distribution moves to a new kernel, they'll move
all the other packages up with it.

The Latest Ubuntu, for example, has been compiled against a version of
binutils that does not have this problem.

And nobody has answered what happens when NOPLs are used to
synchronise with something else? Surely that can lead to subtle, hard
to debug breakage? Isn't that plain worse than refusing to run or
breaking out of execution?

A CMOV was never used in that way so the fact it took longer to trap
and execute an emulation wouldn't have been a problem.

Cheers,

Nick

On Wed, Sep 8, 2010 at 6:56 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> If a process crashes because of NOPL usage, you get the feedback loop to fix it.
>
> Trouble is that means rebuilding entire distributios, who don't
> neccessarily care about Geode. We had the same problem before with cmov
> and the VIA processors, GCC programmers didn't read the Intel PPro
> manual and got it wrong but it was the end users who suffered, and most of
> them were not in a position to rebuild their distro (and every security
> update, and test them all ...) and their distro didn't care.
>
> So lots of people ran a kernel with a CMOV hack in it - never got
> mainstream which is unfortunate because if it had it would have helped
> a lot more people and had the hole in it fixed.
>
>> The beauty of open source is that we can recompile to correct the bad
>> assumption; the latest version of binutils (GAS) finally corrects the
>> bad semantic that generic i686 includes NOPL.
>
> And the reality is that this won't happen. Yes you can do it but it's an
> enormously slow and inefficient way to tackle the problem.
>
> So the patch (corrected) makes complete sense.
>
> Alan
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

> If a process crashes because of NOPL usage, you get the feedback loop to fix it.

Trouble is that means rebuilding entire distributios, who don't
neccessarily care about Geode. We had the same problem before with cmov
and the VIA processors, GCC programmers didn't read the Intel PPro
manual and got it wrong but it was the end users who suffered, and most of
them were not in a position to rebuild their distro (and every security
update, and test them all ...) and their distro didn't care.

So lots of people ran a kernel with a CMOV hack in it - never got
mainstream which is unfortunate because if it had it would have helped
a lot more people and had the hole in it fixed.

> The beauty of open source is that we can recompile to correct the bad
> assumption; the latest version of binutils (GAS) finally corrects the
> bad semantic that generic i686 includes NOPL.

And the reality is that this won't happen. Yes you can do it but it's an
enormously slow and inefficient way to tackle the problem.

So the patch (corrected) makes complete sense.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

> And nobody has answered what happens when NOPLs are used to
> synchronise with something else? Surely that can lead to subtle, hard
> to debug breakage? Isn't that plain worse than refusing to run or
> breaking out of execution?

NOPL for synchronisation - of user space. I'd say thats sufficiently
unlikely not to care. It's still an improvement on crashing.

The other point you make is a good one but it's not clear that all the
proprietary apps for example will get recompiled, so to many users its
not a solution. Similarly the long term stable distros aren't going to
recompile everything and invalidate all their test data, so I doubt that
things like Centos will.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi,

It's still not making any sense to me what-so-ever. And I'm really
trying to be open minded!

1) Those enterprise long term distributions aren't going to be
targeting the Crusoe or Geode. It's not their market. Would they even
back port, therefore, if it was an issue?
(This hypothetical anyway as it assuming they have an i686 targeted
build already. Who does? Centos, RHEL and SUSE Enterprise are all i386
targeted at the moment for x86.)

2) Nor is it likely, considering their focus, that end users will want
to run them on their Crusoe or Geode hardware if they were i686
targeted. (Again, hypothetical, as they're not targeted at i686!)

3) Distributions that specifically target the Crusoe or Geode already,
by definition, won't be using an i686 targeted build at the moment.

4) As for proprietary apps, again, is it likely that they're targeting
i686 already? Or is it far more likely they're compiled for i386 et.
al.

5) If they are targeting i686 already, end users won't have been able
to run them on their Crusoe or Geode hardware before so they're not
going to miss them now.

6) What we are talking about is future distributions, whatever they
are, that are now or are changing to target i686 which end users will
ultimately want to migrate to and run.
Again, at the moment, most are i386 targeted, so they all work regardless.
(They'll need to recompile everything anyway if they decide to move,
so it's completely moot surely...)

Thanks,

Nick

On Wed, Sep 8, 2010 at 8:07 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> And nobody has answered what happens when NOPLs are used to
>> synchronise with something else? Surely that can lead to subtle, hard
>> to debug breakage? Isn't that plain worse than refusing to run or
>> breaking out of execution?
>
> NOPL for synchronisation - of user space. I'd say thats sufficiently
> unlikely not to care. It's still an improvement on crashing.
>
> The other point you make is a good one but it's not clear that all the
> proprietary apps for example will get recompiled, so to many users its
> not a solution. Similarly the long term stable distros aren't going to
> recompile everything and invalidate all their test data, so I doubt that
> things like Centos will.
>
> Alan
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On 08/29/2010 06:39 AM, Matteo Croce wrote:
> If the parsing fails due get_user returning error I call
> `do_invalid_op(regs, error_code);`
> which is the default handler, which does the page fault.

No, it doesn't. It does an SIGILL, not a SIGSEGV. An application which
does its own VM management depends on the difference.

Also, you only test for specific forms of NOPL, whereas the right thing
is to recognize the overall forms, not just byte sequences.

> to check the CS I do `regs->cs != __USER_CS` but how to check the IP value?
> convert_ip_to_linear() and then check something?

get_user() will check for the validity of a linear address, and yes,
convert_ip_to_linear() should give you the linear address to check for.
However, you also have to check for the CPU mode, since the byte
sequences mean different things in 16-, 32- and 64-bit mode.

All of this is why I'm extremely reluctant to allow in an ad hoc hack
like this one ... there just are way too many pitfalls, any of which can
turn into a security hole.

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On 09/08/2010 02:11 PM, Alan Cox wrote:
>
> Getting the stuff right in the current kernel makes it easier to backport
> and fix and do tidily. It's not as if NOPL is complicated to emulate.

The execution phase is easy ;) The parsing phase is rather hard. This
is part of why I would much rather see a single x86 interpreter in the
kernel, and the one single interpreter that we need anyway is the one in
KVM.

I haven't looked at what it would take to generalize it to also support
trap and emulate of arbitrary instructions (not just NOPL and CMOV and
so on), but x86 is subtle and quick to anger, and doing it wrong can
trivially introduce security holes.

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Okay, you win! :)

Were some form of this patch to be merged in the future, the bintutils
fix is likely to result in it never being hit anyway, so I suppose it
doesn't matter much.
In the long run, it could probably be removed.

I still think it's an incredibly ugly hack! :P

Cheers,

Nick

On Wed, Sep 8, 2010 at 10:11 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> 1) Those enterprise long term distributions aren't going to be
>> targeting the Crusoe or Geode. It's not their market. Would they even
>
> See this is the problem. You've forgotten *users*. It's easy to roll a
> special kernel, its hell on earth
>
>> back port, therefore, if it was an issue?
>> (This hypothetical anyway as it assuming they have an i686 targeted
>> build already. Who does? Centos, RHEL and SUSE Enterprise are all i386
>> targeted at the moment for x86.)
>
> Wrong for some packages - and enough to break things. Bits like glibc for
> example and chunks of X and proprietary apps.
>
>>. (Again, hypothetical, as they're not targeted at i686!)
>
> See above
>
>> 4) As for proprietary apps, again, is it likely that they're targeting
>> i686 already? Or is it far more likely they're compiled for i386 et.
>> al.
>
> Lots used -i686 because the compiler people said its faster and it works
> on all 686 platforms. Neither of which it turns out is exactly true in
> many cases.
>
>> 6) What we are talking about is future distributions, whatever they
>> are, that are now or are changing to target i686 which end users will
>> ultimately want to migrate to and run.
>
> s/we/you
>
> Getting the stuff right in the current kernel makes it easier to backport
> and fix and do tidily. It's not as if NOPL is complicated to emulate.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

> 1) Those enterprise long term distributions aren't going to be
> targeting the Crusoe or Geode. It's not their market. Would they even

See this is the problem. You've forgotten *users*. It's easy to roll a
special kernel, its hell on earth

> back port, therefore, if it was an issue?
> (This hypothetical anyway as it assuming they have an i686 targeted
> build already. Who does? Centos, RHEL and SUSE Enterprise are all i386
> targeted at the moment for x86.)

Wrong for some packages - and enough to break things. Bits like glibc for
example and chunks of X and proprietary apps.

>. (Again, hypothetical, as they're not targeted at i686!)

See above

> 4) As for proprietary apps, again, is it likely that they're targeting
> i686 already? Or is it far more likely they're compiled for i386 et.
> al.

Lots used -i686 because the compiler people said its faster and it works
on all 686 platforms. Neither of which it turns out is exactly true in
many cases.

> 6) What we are talking about is future distributions, whatever they
> are, that are now or are changing to target i686 which end users will
> ultimately want to migrate to and run.

s/we/you

Getting the stuff right in the current kernel makes it easier to backport
and fix and do tidily. It's not as if NOPL is complicated to emulate.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/