Illegal instruction - a workaround or fix ?

Illegal instruction - a workaround or fix ?

[Florian Lohoff]

Hi,
i seem to get the illegal instruction stuff killed when
doing this.

diff -u -r1.17 sysmips.c
--- arch/mips/kernel/sysmips.c	2001/02/09 21:05:46	1.17
+++ arch/mips/kernel/sysmips.c	2001/03/11 16:11:30
@@ -111,13 +111,14 @@
 
 		((struct pt_regs *)&cmd)->regs[2] = tmp;
 		((struct pt_regs *)&cmd)->regs[7] = 0;
-
+#if 0
 		__asm__ __volatile__(
 			"move\t$29, %0\n\t"
 			"j\to32_ret_from_sys_call"
 			: /* No outputs */
 			: "r" (&cmd));
 		/* Unreached */
+#endif
 	}
 
 	case MIPS_FIXADE:


Could someone please explain me the difference betweeen "handle_sys"
which leads to "o32_ret_from_sys_call" and ... which leads to
"ret_from_sys_call".

Flo
-- 
Florian Lohoff                  flo@rfc822.org             +49-5201-669912
     Why is it called "common sense" when nobody seems to have any?

Re: Illegal instruction - a workaround or fix ?

[Ralf Baechle]

Thanks, that was the hint I needed.  o32_ret_from_sys_call expects the
content of s-registers to be unchanged from userspace but sys_sysmips
clobbers them.

Below a patch from the famous ``Smoke This, It's Good For You (TM)''
series.  Lemme know if it helps.

  Ralf

--- arch/mips/kernel/sysmips.c	2001/02/09 21:05:46	1.17
+++ arch/mips/kernel/sysmips.c	2001/03/12 11:12:20
@@ -19,6 +19,7 @@
 
 #include <asm/cachectl.h>
 #include <asm/pgalloc.h>
+#include <asm/stackframe.h>
 #include <asm/sysmips.h>
 #include <asm/uaccess.h>
 
@@ -47,8 +48,9 @@
 	return address;
 }
 
-asmlinkage int
-sys_sysmips(int cmd, int arg1, int arg2, int arg3)
+save_static_function(sys_sysmips);
+static_unused int
+_sys_sysmips(int cmd, int arg1, int arg2, int arg3)
 {
 	int	*p;
 	char	*name;
@@ -114,7 +116,7 @@
 
 		__asm__ __volatile__(
 			"move\t$29, %0\n\t"
-			"j\to32_ret_from_sys_call"
+			"j\to32_ret_from_sys_call_restore_static"
 			: /* No outputs */
 			: "r" (&cmd));
 		/* Unreached */
--- arch/mips/kernel/scall_o32.S	2000/08/08 18:54:49	1.14
+++ arch/mips/kernel/scall_o32.S	2001/03/12 11:09:12
@@ -86,6 +86,10 @@
 	RESTORE_SOME
 	RESTORE_SP_AND_RET
 
+EXPORT(o32_ret_from_sys_call_restore_static)
+	RESTORE_STATIC
+	j	o32_ret_from_sys_call
+
 o32_handle_softirq:
 	jal	do_softirq
 	b	9b

Re: Illegal instruction - a workaround or fix ?

[Florian Lohoff]

On Mon, Mar 12, 2001 at 12:21:34PM +0100, Ralf Baechle wrote:
> Thanks, that was the hint I needed.  o32_ret_from_sys_call expects the
> content of s-registers to be unchanged from userspace but sys_sysmips
> clobbers them.
> 
> Below a patch from the famous ``Smoke This, It's Good For You (TM)''
> series.  Lemme know if it helps.

As mentioned on IRC - This "Oopses" for me ...

Flo
-- 
Florian Lohoff                  flo@rfc822.org             +49-5201-669912
     Why is it called "common sense" when nobody seems to have any?

Re: Illegal instruction - a workaround or fix ?

[Pete Popov]

Florian Lohoff wrote:
> 
> On Mon, Mar 12, 2001 at 12:21:34PM +0100, Ralf Baechle wrote:
> > Thanks, that was the hint I needed.  o32_ret_from_sys_call expects the
> > content of s-registers to be unchanged from userspace but sys_sysmips
> > clobbers them.
> >
> > Below a patch from the famous ``Smoke This, It's Good For You (TM)''
> > series.  Lemme know if it helps.
> 
> As mentioned on IRC - This "Oopses" for me ...

I'm bringing this up again because none of the related patches on this
topic have been applied to the latest cvs kernel.  The patch Florian
refers to above oopses for me as well.  This patch below, from Florian,
but updated against the latest cvs kernel, works (at least the few
simple tests I've run do work now).  

--- arch/mips/kernel/sysmips.c.old      Fri Apr 20 11:58:38 2001
+++ arch/mips/kernel/sysmips.c  Fri Apr 20 11:59:59 2001
@@ -99,7 +99,7 @@
                        ".word\t1b, 3b\n\t"
                        ".word\t2b, 3b\n\t"
                        ".previous\n\t"
-                       : "=&r" (tmp), "=o" (* (u32 *) p), "=r" (errno)
+                       : "=&r" (retval), "=o" (* (u32 *) p), "=r"
(errno)
                        : "r" (arg2), "o" (* (u32 *) p), "2" (errno)
                        : "$1");
 
@@ -110,14 +110,7 @@
                if (current->ptrace & PT_TRACESYS)
                        syscall_trace();
 
-               ((struct pt_regs *)&cmd)->regs[2] = tmp;
-               ((struct pt_regs *)&cmd)->regs[7] = 0;
-
-               __asm__ __volatile__(
-                       "move\t$29, %0\n\t"
-                       "j\to32_ret_from_sys_call"
-                       : /* No outputs */
-                       : "r" (&cmd));
+               goto out;
                /* Unreached */
 #else
        printk("sys_sysmips(MIPS_ATOMIC_SET, ...) not ready for
!CONFIG_CPU_HAS_LLSC\n");

Re: Illegal instruction - a workaround or fix ?

[Ralf Baechle]

On Fri, Apr 20, 2001 at 12:05:03PM -0700, Pete Popov wrote:

> I'm bringing this up again because none of the related patches on this
> topic have been applied to the latest cvs kernel.  The patch Florian
> refers to above oopses for me as well.  This patch below, from Florian,
> but updated against the latest cvs kernel, works (at least the few
> simple tests I've run do work now).  

This patch is broken for small negative numbers in the memory.

Florian now has a better patch that should be correct.

  Ralf

Re: Illegal instruction - a workaround or fix ?

[Pete Popov]

Ralf Baechle wrote:
> 
> On Fri, Apr 20, 2001 at 12:05:03PM -0700, Pete Popov wrote:
> 
> > I'm bringing this up again because none of the related patches on this
> > topic have been applied to the latest cvs kernel.  The patch Florian
> > refers to above oopses for me as well.  This patch below, from Florian,
> > but updated against the latest cvs kernel, works (at least the few
> > simple tests I've run do work now).
> 
> This patch is broken for small negative numbers in the memory.
> 
> Florian now has a better patch that should be correct.

Are you talking about the fast_sysmips patch Florian posted? 

Florian, if you have a newer patch than what you last posted on the
mailing list, would you mind sending it?

Thanks,

Pete

Re: Illegal instruction - a workaround or fix ?

[Florian Lohoff]

On Fri, Apr 20, 2001 at 12:19:00PM -0700, Pete Popov wrote:
> Are you talking about the fast_sysmips patch Florian posted? 
> 
> Florian, if you have a newer patch than what you last posted on the
> mailing list, would you mind sending it?

I havent got anything newer - I am using that patch now on 2 machines
running continuesly as a build machine open in the net. Nobody
yet has reported any problems (Which isnt a good indicator)

I am not shure if it is really correct so someone with deep knowledge
of the inners of sysmips should check if its really ok.

Nevertheless - It works for me ...

Flo
-- 
Florian Lohoff                  flo@rfc822.org             +49-5201-669912
     Why is it called "common sense" when nobody seems to have any?

Re: Illegal instruction - a workaround or fix ?

[Florian Lohoff]

On Fri, Apr 20, 2001 at 12:05:03PM -0700, Pete Popov wrote:
> 
> I'm bringing this up again because none of the related patches on this
> topic have been applied to the latest cvs kernel.  The patch Florian
> refers to above oopses for me as well.  This patch below, from Florian,
> but updated against the latest cvs kernel, works (at least the few
> simple tests I've run do work now).  
> 

The patch is definitly only a workaround and not a fix - The problem
is that the sysmips saves registers on the stack in the function
prolog. When taking the shortcut exit which is needed to not garble
the return code which can be "unsigned int" and not interpret it
as an -ESOMETHING. When doing this you need to skip a couple of instructions in
scall_o32.S which makes the shortcut necessary. On the shortcut
the compiler does not generate a correct epilgue (How should it know)
so the registers get garbled in the syscall which lets your programs
die.

The asm variant tries to only touch registers already saved in
scall_o32.S (and also restored on exit) which are not anymore
used or register we are allowed to change anyway (caller saved).

A different solution would be to take the usual exit in sysmips via
the return at the end (for which the compiler generated a correct
epilogue) and modify the return address - This is an very ugly hack
and you cant tell where the compiler stores the ra on the stack.

Flo
-- 
Florian Lohoff                  flo@rfc822.org             +49-5201-669912
     Why is it called "common sense" when nobody seems to have any?

Re: Illegal instruction - a workaround or fix ?

[Maciej W. Rozycki]

On Fri, 20 Apr 2001, Florian Lohoff wrote:

> A different solution would be to take the usual exit in sysmips via
> the return at the end (for which the compiler generated a correct
> epilogue) and modify the return address - This is an very ugly hack
> and you cant tell where the compiler stores the ra on the stack.

 It could be doable with __builtin_frame_address().  Haven't investigated
it further, though. 

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: Illegal instruction - a workaround or fix ?

[Ralf Baechle]

On Mon, Apr 30, 2001 at 11:34:32AM +0200, Maciej W. Rozycki wrote:

> > A different solution would be to take the usual exit in sysmips via
> > the return at the end (for which the compiler generated a correct
> > epilogue) and modify the return address - This is an very ugly hack
> > and you cant tell where the compiler stores the ra on the stack.
> 
>  It could be doable with __builtin_frame_address().  Haven't investigated
> it further, though. 

MIPS ABI doesn't define that ra gets stored at a constant offset in
the stackframe, so that won't work.

  Ralf

Re: Illegal instruction - a workaround or fix ?

[Maciej W. Rozycki]

On Mon, 30 Apr 2001, Ralf Baechle wrote:

> >  It could be doable with __builtin_frame_address().  Haven't investigated
> > it further, though. 
> 
> MIPS ABI doesn't define that ra gets stored at a constant offset in
> the stackframe, so that won't work.

 Hmm, I think we check look how gcc gets __builtin_return_address() 
(specifically for levels greater than 0) and use the same way.  We don't
need to stick to the ABI in the kernel (building non-PIC we already
violate it anyway) and we can assume the code is to be built by gcc. 

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +