[PATCH 1/2] asmlinkage_protect replaces prevent_tail_call

[PATCH 1/2] asmlinkage_protect replaces prevent_tail_call

[Roland McGrath]

The prevent_tail_call() macro works around the problem of the compiler
clobbering argument words on the stack, which for asmlinkage functions
is the caller's (user's) struct pt_regs.  The tail/sibling-call
optimization is not the only way that the compiler can decide to use
stack argument words as scratch space, which we have to prevent.
Other optimizations can do it too.

Until we have new compiler support to make "asmlinkage" binding on the
compiler's own use of the stack argument frame, we have work around all
the manifestations of this issue that crop up.

More cases seem to be prevented by also keeping the incoming argument
variables live at the end of the function.  This makes their original
stack slots attractive places to leave those variables, so the compiler
tends not clobber them for something else.  It's still no guarantee, but
it handles some observed cases that prevent_tail_call() did not.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/x86/kernel/tls.c     |    4 ++--
 fs/open.c                 |    8 ++++----
 include/asm-x86/linkage.h |   24 +++++++++++++++++++++++-
 include/linux/linkage.h   |    4 ++--
 kernel/exit.c             |    4 ++--
 kernel/uid16.c            |   22 +++++++++++-----------
 6 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/tls.c b/arch/x86/kernel/tls.c
index 022bcaa..ab6bf37 100644
--- a/arch/x86/kernel/tls.c
+++ b/arch/x86/kernel/tls.c
@@ -92,7 +92,7 @@ int do_set_thread_area(struct task_struct *p, int idx,
 asmlinkage int sys_set_thread_area(struct user_desc __user *u_info)
 {
 	int ret = do_set_thread_area(current, -1, u_info, 1);
-	prevent_tail_call(ret);
+	asmlinkage_protect(1, ret, u_info);
 	return ret;
 }
 
@@ -142,7 +142,7 @@ int do_get_thread_area(struct task_struct *p, int idx,
 asmlinkage int sys_get_thread_area(struct user_desc __user *u_info)
 {
 	int ret = do_get_thread_area(current, -1, u_info);
-	prevent_tail_call(ret);
+	asmlinkage_protect(1, ret, u_info);
 	return ret;
 }
 
diff --git a/fs/open.c b/fs/open.c
index a4b1202..3fa4e4f 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -335,7 +335,7 @@ asmlinkage long sys_ftruncate(unsigned int fd, unsigned long length)
 {
 	long ret = do_sys_ftruncate(fd, length, 1);
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(2, ret, fd, length);
 	return ret;
 }
 
@@ -350,7 +350,7 @@ asmlinkage long sys_ftruncate64(unsigned int fd, loff_t length)
 {
 	long ret = do_sys_ftruncate(fd, length, 0);
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(2, ret, fd, length);
 	return ret;
 }
 #endif
@@ -1067,7 +1067,7 @@ asmlinkage long sys_open(const char __user *filename, int flags, int mode)
 
 	ret = do_sys_open(AT_FDCWD, filename, flags, mode);
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(3, ret, filename, flags, mode);
 	return ret;
 }
 
@@ -1081,7 +1081,7 @@ asmlinkage long sys_openat(int dfd, const char __user *filename, int flags,
 
 	ret = do_sys_open(dfd, filename, flags, mode);
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(4, ret, dfd, filename, flags, mode);
 	return ret;
 }
 
diff --git a/include/asm-x86/linkage.h b/include/asm-x86/linkage.h
index 31739c7..d605eeb 100644
--- a/include/asm-x86/linkage.h
+++ b/include/asm-x86/linkage.h
@@ -8,12 +8,34 @@
 
 #ifdef CONFIG_X86_32
 #define asmlinkage CPP_ASMLINKAGE __attribute__((regparm(0)))
-#define prevent_tail_call(ret) __asm__ ("" : "=r" (ret) : "0" (ret))
 /*
  * For 32-bit UML - mark functions implemented in assembly that use
  * regparm input parameters:
  */
 #define asmregparm __attribute__((regparm(3)))
+
+#define asmlinkage_protect(n, ret, args...) \
+	__asmlinkage_protect##n(ret, ##args)
+#define __asmlinkage_protect_n(ret, args...) \
+	__asm__ __volatile__ ("" : "=r" (ret) : "0" (ret), ##args)
+#define __asmlinkage_protect0(ret) \
+	__asmlinkage_protect_n(ret)
+#define __asmlinkage_protect1(ret, arg1) \
+	__asmlinkage_protect_n(ret, "g" (arg1))
+#define __asmlinkage_protect2(ret, arg1, arg2) \
+	__asmlinkage_protect_n(ret, "g" (arg1), "g" (arg2))
+#define __asmlinkage_protect3(ret, arg1, arg2, arg3) \
+	__asmlinkage_protect_n(ret, "g" (arg1), "g" (arg2), "g" (arg3))
+#define __asmlinkage_protect4(ret, arg1, arg2, arg3, arg4) \
+	__asmlinkage_protect_n(ret, "g" (arg1), "g" (arg2), "g" (arg3), \
+			      "g" (arg4))
+#define __asmlinkage_protect5(ret, arg1, arg2, arg3, arg4, arg5) \
+	__asmlinkage_protect_n(ret, "g" (arg1), "g" (arg2), "g" (arg3), \
+			      "g" (arg4), "g" (arg5))
+#define __asmlinkage_protect6(ret, arg1, arg2, arg3, arg4, arg5, arg6) \
+	__asmlinkage_protect_n(ret, "g" (arg1), "g" (arg2), "g" (arg3), \
+			      "g" (arg4), "g" (arg5), "g" (arg6))
+
 #endif
 
 #ifdef CONFIG_X86_ALIGNMENT_16
diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index 0592936..fe2a39c 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -17,8 +17,8 @@
 # define asmregparm
 #endif
 
-#ifndef prevent_tail_call
-# define prevent_tail_call(ret) do { } while (0)
+#ifndef asmlinkage_protect
+# define asmlinkage_protect(n, ret, args...)	do { } while (0)
 #endif
 
 #ifndef __ALIGN
diff --git a/kernel/exit.c b/kernel/exit.c
index 53872bf..073005b 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1608,7 +1608,7 @@ asmlinkage long sys_waitid(int which, pid_t upid,
 	put_pid(pid);
 
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(5, ret, which, upid, infop, options, ru);
 	return ret;
 }
 
@@ -1640,7 +1640,7 @@ asmlinkage long sys_wait4(pid_t upid, int __user *stat_addr,
 	put_pid(pid);
 
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(4, ret, upid, stat_addr, options, ru);
 	return ret;
 }
 
diff --git a/kernel/uid16.c b/kernel/uid16.c
index dd308ba..3e41c16 100644
--- a/kernel/uid16.c
+++ b/kernel/uid16.c
@@ -21,7 +21,7 @@ asmlinkage long sys_chown16(const char __user * filename, old_uid_t user, old_gi
 {
 	long ret = sys_chown(filename, low2highuid(user), low2highgid(group));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(3, ret, filename, user, group);
 	return ret;
 }
 
@@ -29,7 +29,7 @@ asmlinkage long sys_lchown16(const char __user * filename, old_uid_t user, old_g
 {
 	long ret = sys_lchown(filename, low2highuid(user), low2highgid(group));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(3, ret, filename, user, group);
 	return ret;
 }
 
@@ -37,7 +37,7 @@ asmlinkage long sys_fchown16(unsigned int fd, old_uid_t user, old_gid_t group)
 {
 	long ret = sys_fchown(fd, low2highuid(user), low2highgid(group));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(3, ret, fd, user, group);
 	return ret;
 }
 
@@ -45,7 +45,7 @@ asmlinkage long sys_setregid16(old_gid_t rgid, old_gid_t egid)
 {
 	long ret = sys_setregid(low2highgid(rgid), low2highgid(egid));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(2, ret, rgid, egid);
 	return ret;
 }
 
@@ -53,7 +53,7 @@ asmlinkage long sys_setgid16(old_gid_t gid)
 {
 	long ret = sys_setgid(low2highgid(gid));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(1, ret, gid);
 	return ret;
 }
 
@@ -61,7 +61,7 @@ asmlinkage long sys_setreuid16(old_uid_t ruid, old_uid_t euid)
 {
 	long ret = sys_setreuid(low2highuid(ruid), low2highuid(euid));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(2, ret, ruid, euid);
 	return ret;
 }
 
@@ -69,7 +69,7 @@ asmlinkage long sys_setuid16(old_uid_t uid)
 {
 	long ret = sys_setuid(low2highuid(uid));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(1, ret, uid);
 	return ret;
 }
 
@@ -78,7 +78,7 @@ asmlinkage long sys_setresuid16(old_uid_t ruid, old_uid_t euid, old_uid_t suid)
 	long ret = sys_setresuid(low2highuid(ruid), low2highuid(euid),
 				 low2highuid(suid));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(3, ret, ruid, euid, suid);
 	return ret;
 }
 
@@ -98,7 +98,7 @@ asmlinkage long sys_setresgid16(old_gid_t rgid, old_gid_t egid, old_gid_t sgid)
 	long ret = sys_setresgid(low2highgid(rgid), low2highgid(egid),
 				 low2highgid(sgid));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(3, ret, rgid, egid, sgid);
 	return ret;
 }
 
@@ -117,7 +117,7 @@ asmlinkage long sys_setfsuid16(old_uid_t uid)
 {
 	long ret = sys_setfsuid(low2highuid(uid));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(1, ret, uid);
 	return ret;
 }
 
@@ -125,7 +125,7 @@ asmlinkage long sys_setfsgid16(old_gid_t gid)
 {
 	long ret = sys_setfsgid(low2highgid(gid));
 	/* avoid REGPARM breakage on x86: */
-	prevent_tail_call(ret);
+	asmlinkage_protect(1, ret, gid);
 	return ret;
 }
 

[PATCH 2/2] asmlinkage_protect sys_io_getevents

[Roland McGrath]

Use asmlinkage_protect in sys_io_getevents, because GCC for i386 with
CONFIG_FRAME_POINTER=n can decide to clobber an argument word on the
stack, i.e. the user struct pt_regs.  Here the problem is not a tail
call, but just the compiler's use of the stack when it inlines and
optimizes the body of the called function.  This seems to avoid it.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 fs/aio.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 6af9219..60a4cd4 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1790,6 +1790,7 @@ asmlinkage long sys_io_getevents(aio_context_t ctx_id,
 		put_ioctx(ioctx);
 	}
 
+	asmlinkage_protect(5, ret, ctx_id, min_nr, nr, events, timeout);
 	return ret;
 }
 

[PATCH] Fix compile breakage caused by asmlinkage_protect

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

git commit 54a015104136974262afa4b8ddd943ea70dec8a2
"asmlinkage_protect replaces prevent_tail_call" causes this build failure
on s390:

  AS      arch/s390/kernel/entry64.o
In file included from arch/s390/kernel/entry64.S:14:
include/linux/linkage.h:34: error: syntax error in macro parameter list
make[1]: *** [arch/s390/kernel/entry64.o] Error 1
make: *** [arch/s390/kernel] Error 2

So just surround the new define with an #ifndef __ASSEMBLY__ to prevent
any side effects on asm code.

Cc: Roland McGrath <roland@redhat.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 include/linux/linkage.h |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6/include/linux/linkage.h
===================================================================
--- linux-2.6.orig/include/linux/linkage.h
+++ linux-2.6/include/linux/linkage.h
@@ -30,9 +30,11 @@
  * protection to work (ie no more work that the compiler might
  * end up needing stack temporaries for).
  */
+#ifndef __ASSEMBLY__
 #ifndef asmlinkage_protect
 # define asmlinkage_protect(n, ret, args...)	do { } while (0)
 #endif
+#endif
 
 #ifndef __ALIGN
 #define __ALIGN		.align 4,0x90

Re: [PATCH] Fix compile breakage caused by asmlinkage_protect

[Linus Torvalds]

On Fri, 11 Apr 2008, Heiko Carstens wrote:
> 
> git commit 54a015104136974262afa4b8ddd943ea70dec8a2
> "asmlinkage_protect replaces prevent_tail_call" causes this build failure
> on s390:
> 
>   AS      arch/s390/kernel/entry64.o
> In file included from arch/s390/kernel/entry64.S:14:
> include/linux/linkage.h:34: error: syntax error in macro parameter list
> make[1]: *** [arch/s390/kernel/entry64.o] Error 1
> make: *** [arch/s390/kernel] Error 2

Ok, that's just _odd_.

> So just surround the new define with an #ifndef __ASSEMBLY__ to prevent
> any side effects on asm code.

There are no side effects on asm code. It just adds a #define that 
obviously won't be used.

Is the s390 assembler using some strange C pre-processor that is different 
from the main C preprocessor and doesn't understand this pattern?

I really think you should fix *that*, because otherwise you'll hit these 
kinds of bugs occasionally. There aren't that many asm files, it's not 
worth it optimizing them to use some faster-but-stupider preprocessor.

			Linus

Re: [PATCH] Fix compile breakage caused by asmlinkage_protect

[Jakub Jelinek]

On Fri, Apr 11, 2008 at 07:46:48AM -0700, Linus Torvalds wrote:
> On Fri, 11 Apr 2008, Heiko Carstens wrote:
> > 
> > git commit 54a015104136974262afa4b8ddd943ea70dec8a2
> > "asmlinkage_protect replaces prevent_tail_call" causes this build failure
> > on s390:
> > 
> >   AS      arch/s390/kernel/entry64.o
> > In file included from arch/s390/kernel/entry64.S:14:
> > include/linux/linkage.h:34: error: syntax error in macro parameter list
> > make[1]: *** [arch/s390/kernel/entry64.o] Error 1
> > make: *** [arch/s390/kernel] Error 2
> 
> Ok, that's just _odd_.

Not if (some) kernel assembly files are preprocessed with -traditional-cpp
or -traditional, which supports neither GNU style arg... nor
ISO C99 ... + __VA_ARGS__ vararg macros.

	Jakub

Re: [PATCH] Fix compile breakage caused by asmlinkage_protect

[Al Viro]

On Fri, Apr 11, 2008 at 07:46:48AM -0700, Linus Torvalds wrote:
> Ok, that's just _odd_.
> 
> > So just surround the new define with an #ifndef __ASSEMBLY__ to prevent
> > any side effects on asm code.
> 
> There are no side effects on asm code. It just adds a #define that 
> obviously won't be used.
> 
> Is the s390 assembler using some strange C pre-processor that is different 
> from the main C preprocessor and doesn't understand this pattern?
> 
> I really think you should fix *that*, because otherwise you'll hit these 
> kinds of bugs occasionally. There aren't that many asm files, it's not 
> worth it optimizing them to use some faster-but-stupider preprocessor.

FWIW, at least m68k and m32r cross-builds hit the same.  I think I've a very
good guess about the reasons:
arch/m32r/kernel/Makefile:EXTRA_AFLAGS  := -traditional
arch/m68k/fpsp040/Makefile:EXTRA_AFLAGS := -traditional
arch/m68k/ifpsp060/Makefile:EXTRA_AFLAGS := -traditional
arch/m68k/kernel/Makefile:EXTRA_AFLAGS := -traditional
arch/m68k/lib/Makefile:EXTRA_AFLAGS := -traditional
arch/m68k/math-emu/Makefile:EXTRA_AFLAGS := -traditional
arch/parisc/kernel/Makefile:AFLAGS_entry.o      := -traditional
arch/parisc/kernel/Makefile:AFLAGS_pacache.o := -traditional
arch/s390/kernel/Makefile:EXTRA_AFLAGS  := -traditional
arch/s390/lib/Makefile:EXTRA_AFLAGS := -traditional
arch/s390/math-emu/Makefile:EXTRA_AFLAGS := -traditional

and that gets us -traditional-cpp passed to cc1, with obvious resulting
unhappiness from vararg macro.

Re: [PATCH] Fix compile breakage caused by asmlinkage_protect

[Linus Torvalds]

On Fri, 11 Apr 2008, Al Viro wrote:
> 
> and that gets us -traditional-cpp passed to cc1, with obvious resulting
> unhappiness from vararg macro.

Yeah, I figured it out eventually. 

I do think the architectures should try to avoid it, if only because x86 
doesn't use -traditional (so they'll hit things like this unnecessarily 
otherwise), but I'll apply Heiko's minimal patch in the meantime.

		Linus

Re: [PATCH] Fix compile breakage caused by asmlinkage_protect

[Kyle McMartin]

On Fri, Apr 11, 2008 at 08:25:05AM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 11 Apr 2008, Al Viro wrote:
> > 
> > and that gets us -traditional-cpp passed to cc1, with obvious resulting
> > unhappiness from vararg macro.
> 
> Yeah, I figured it out eventually. 
> 
> I do think the architectures should try to avoid it, if only because x86 
> doesn't use -traditional (so they'll hit things like this unnecessarily 
> otherwise), but I'll apply Heiko's minimal patch in the meantime.
> 

Cool with me; I'll try to puzzle out why removing -traditional breaks on
those two specific files. Ugh, big cleanups likely.

cheers, Kyle

Re: [PATCH] Fix compile breakage caused by asmlinkage_protect

[Martin Schwidefsky]

On Fri, 2008-04-11 at 11:37 -0400, Kyle McMartin wrote:
> On Fri, Apr 11, 2008 at 08:25:05AM -0700, Linus Torvalds wrote:
> > On Fri, 11 Apr 2008, Al Viro wrote:
> > > and that gets us -traditional-cpp passed to cc1, with obvious resulting
> > > unhappiness from vararg macro.
> > 
> > Yeah, I figured it out eventually. 
> > 
> > I do think the architectures should try to avoid it, if only because x86 
> > doesn't use -traditional (so they'll hit things like this unnecessarily 
> > otherwise), but I'll apply Heiko's minimal patch in the meantime.
> > 
> 
> Cool with me; I'll try to puzzle out why removing -traditional breaks on
> those two specific files. Ugh, big cleanups likely.

So there is at least one architecture that really requires -traditional,
it is not an option to just remove them. For s390 the kernel compiles
fine without -traditional on the AFLAGS so we can as well remove them.
I'll queue the patch below. In the meantime Heikos patch will have to
do.

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

---
[PATCH] s390: remove -traditional from AFLAGS

From: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---

diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
index 4d3e383..cfb6ccc 100644
--- a/arch/s390/kernel/Makefile
+++ b/arch/s390/kernel/Makefile
@@ -2,7 +2,7 @@
 # Makefile for the linux kernel.
 #
 
-EXTRA_AFLAGS	:= -traditional
+EXTRA_AFLAGS	:= 
 
 #
 # Passing null pointers is ok for smp code, since we access the lowcore here.
diff --git a/arch/s390/lib/Makefile b/arch/s390/lib/Makefile
index 5208443..dd6f9f5 100644
--- a/arch/s390/lib/Makefile
+++ b/arch/s390/lib/Makefile
@@ -2,7 +2,7 @@
 # Makefile for s390-specific library files..
 #
 
-EXTRA_AFLAGS := -traditional
+EXTRA_AFLAGS := 
 
 lib-y += delay.o string.o uaccess_std.o uaccess_pt.o
 obj-$(CONFIG_32BIT) += div64.o qrnnd.o
diff --git a/arch/s390/math-emu/Makefile b/arch/s390/math-emu/Makefile
index 73b3e72..42828d3 100644
--- a/arch/s390/math-emu/Makefile
+++ b/arch/s390/math-emu/Makefile
@@ -5,4 +5,4 @@
 obj-$(CONFIG_MATHEMU) := math.o
 
 EXTRA_CFLAGS := -I$(src) -Iinclude/math-emu -w
-EXTRA_AFLAGS := -traditional
+EXTRA_AFLAGS := 

Re: [PATCH] Fix compile breakage caused by asmlinkage_protect

[Adrian Bunk]

On Fri, Apr 11, 2008 at 06:03:07PM +0200, Martin Schwidefsky wrote:
>...
> --- a/arch/s390/kernel/Makefile
> +++ b/arch/s390/kernel/Makefile
> @@ -2,7 +2,7 @@
>  # Makefile for the linux kernel.
>  #
>  
> -EXTRA_AFLAGS	:= -traditional
> +EXTRA_AFLAGS	:= 
>...

You can simply ditch the line.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed