[PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[David Woodhouse]

On Wed, 2006-08-30 at 23:59 +0000, Linux Kernel Mailing List wrote:
> commit 386dcafaacd212ef4a8aeed67a7db3ffbb44c7b2
> tree 99e470b46fb81a1cde164458d03c9c9d807c1231
> parent 266f0566761cf88906d634727b3d9fc2556f5cbd
> author Andi Kleen <ak@suse.de> 1156959438 +0200
> committer Linus Torvalds <torvalds@g5.osdl.org> 1156979116 -0700
> 
> [PATCH] i386: Remove __KERNEL__ ifdef around _syscall*()
> 
> After all their only point is having them in user space.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>

This patch is wrong. The kernel headers do _not_ exist merely to provide
a library of stuff for userspace to abuse. In particular, they do not
exist to provide arch maintainers with a convenient dumping ground for
helper functionality which already exists in userspace anyway.

These macros should be inside #ifdef __KERNEL__ for three reasons:

1. C libraries have their own syscall(); there's no _need_ for these
   macros to be exported by the kernel.

2. These macros work in-kernel but they are _not_ designed to work in
   the general case in userspace. The i386 version is broken for PIC
   code, for example.

3. We should be consistent about what we provide in kernel headers.
   Since we don't provide these macros on other architectures, we should
   not do so on i386 and x86_64 -- especially as the reason for doing so
   seems to be just that the arch maintainer doesn't want to use the
   proper glibc function in his test hacks.

With these two patches from Andi, we have restored _syscallX() to user
visibility on x86_64, while it's broken on i386 and absent on other
architectures. That's a very suboptimal state of affairs.

I see three possible ways to fix this. The first, and the sanest, is
just to put the ifdefs back and remove these macros from user view, as
in the patch below.

The second option, if the consensus really is that we should ignore
reason #1 above and export these unneeded macros, is that we can apply
the patch below and then also add generic versions of _syscall*() in
linux/unistd.h which just invoke libc's syscall(), instead of trying to
use the in-kernel assembly versions. That would at least address #2 and
#3 -- and in fact some distributions have been shipping with that
in /usr/include/linux/unistd.h for a long time already.

The third, and least sensible, option would be to accept what Andi has
done and then attempt to address #2 and #3 above by _fixing_ the macros
so that they work in the general case -- in all types of code and on all
architectures. Even in that case, the patch below should be applied to
remove these macros from user view as an interim measure until they are
actually fixed and consistent across architectures and code models.

What we have _now_, however, is entirely unacceptable. Please apply the
following patch to fix it:

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/include/asm-i386/unistd.h b/include/asm-i386/unistd.h
index d983b74..fc1c8dd 100644
--- a/include/asm-i386/unistd.h
+++ b/include/asm-i386/unistd.h
@@ -324,6 +324,8 @@ #define __NR_tee		315
 #define __NR_vmsplice		316
 #define __NR_move_pages		317
 
+#ifdef __KERNEL__
+
 #define NR_syscalls 318
 
 /*
@@ -423,8 +425,6 @@ __asm__ volatile ("push %%ebp ; push %%e
 __syscall_return(type,__res); \
 }
 
-#ifdef __KERNEL__
-
 #define __ARCH_WANT_IPC_PARSE_VERSION
 #define __ARCH_WANT_OLD_READDIR
 #define __ARCH_WANT_OLD_STAT
diff --git a/include/asm-x86_64/unistd.h b/include/asm-x86_64/unistd.h
index 2d89d30..40d96b3 100644
--- a/include/asm-x86_64/unistd.h
+++ b/include/asm-x86_64/unistd.h
@@ -620,6 +620,8 @@ __SYSCALL(__NR_vmsplice, sys_vmsplice)
 #define __NR_move_pages		279
 __SYSCALL(__NR_move_pages, sys_move_pages)
 
+#ifdef __KERNEL__
+
 #define __NR_syscall_max __NR_move_pages
 
 #ifndef __NO_STUBS
@@ -744,8 +746,6 @@ __syscall_return(type,__res); \
 
 #else /* __KERNEL_SYSCALLS__ */
 
-#ifdef __KERNEL__
-
 #include <linux/syscalls.h>
 #include <asm/ptrace.h>
 
@@ -838,9 +838,8 @@ asmlinkage long sys_rt_sigaction(int sig
 				struct sigaction __user *oact,
 				size_t sigsetsize);
 
-#endif
-
-#endif
+#endif /* __ASSEMBLY__ */
+#endif /* __NO_STUBS */
 
 /*
  * "Conditional" syscalls
@@ -850,6 +849,6 @@ #endif
  */
 #define cond_syscall(x) asm(".weak\t" #x "\n\t.set\t" #x ",sys_ni_syscall")
 
-#endif
+#endif /* __KERNEL__ */
 
-#endif
+#endif /* _ASM_X86_64_UNISTD_H_ */


-- 
dwmw2

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[Andrew Morton]

On Mon, 04 Sep 2006 12:27:29 -0700
David Woodhouse <dwmw2@infradead.org> wrote:

> On Wed, 2006-08-30 at 23:59 +0000, Linux Kernel Mailing List wrote:
> > commit 386dcafaacd212ef4a8aeed67a7db3ffbb44c7b2
> > tree 99e470b46fb81a1cde164458d03c9c9d807c1231
> > parent 266f0566761cf88906d634727b3d9fc2556f5cbd
> > author Andi Kleen <ak@suse.de> 1156959438 +0200
> > committer Linus Torvalds <torvalds@g5.osdl.org> 1156979116 -0700
> > 
> > [PATCH] i386: Remove __KERNEL__ ifdef around _syscall*()
> > 
> > After all their only point is having them in user space.
> > 
> > Signed-off-by: Andi Kleen <ak@suse.de>
> > Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> 
> This patch is wrong. The kernel headers do _not_ exist merely to provide
> a library of stuff for userspace to abuse. In particular, they do not
> exist to provide arch maintainers with a convenient dumping ground for
> helper functionality which already exists in userspace anyway.
> 
> These macros should be inside #ifdef __KERNEL__ for three reasons:
> 
> 1. C libraries have their own syscall(); there's no _need_ for these
>    macros to be exported by the kernel.
> 
> 2. These macros work in-kernel but they are _not_ designed to work in
>    the general case in userspace. The i386 version is broken for PIC
>    code, for example.
> 
> 3. We should be consistent about what we provide in kernel headers.
>    Since we don't provide these macros on other architectures, we should
>    not do so on i386 and x86_64 -- especially as the reason for doing so
>    seems to be just that the arch maintainer doesn't want to use the
>    proper glibc function in his test hacks.
> 
> With these two patches from Andi, we have restored _syscallX() to user
> visibility on x86_64, while it's broken on i386 and absent on other
> architectures. That's a very suboptimal state of affairs.
> 
> I see three possible ways to fix this. The first, and the sanest, is
> just to put the ifdefs back and remove these macros from user view, as
> in the patch below.
> 
> The second option, if the consensus really is that we should ignore
> reason #1 above and export these unneeded macros, is that we can apply
> the patch below and then also add generic versions of _syscall*() in
> linux/unistd.h which just invoke libc's syscall(), instead of trying to
> use the in-kernel assembly versions. That would at least address #2 and
> #3 -- and in fact some distributions have been shipping with that
> in /usr/include/linux/unistd.h for a long time already.
> 
> The third, and least sensible, option would be to accept what Andi has
> done and then attempt to address #2 and #3 above by _fixing_ the macros
> so that they work in the general case -- in all types of code and on all
> architectures. Even in that case, the patch below should be applied to
> remove these macros from user view as an interim measure until they are
> actually fixed and consistent across architectures and code models.

I think we're working on removing the _syscall() things altogether.  The
main problem is execve() and I have a series from Arnd queued up to fix
execve.

I think that there are some remaining uses after execve, but I'm not sure
what they are.  Arnd, do you recall?

> What we have _now_, however, is entirely unacceptable. Please apply the
> following patch to fix it:

whimper.  that gives me a reject storm to fix up.

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[Arnd Bergmann]

Am Monday 04 September 2006 21:38 schrieb Andrew Morton:
> I think we're working on removing the _syscall() things altogether.  The
> main problem is execve() and I have a series from Arnd queued up to fix
> execve.
>
> I think that there are some remaining uses after execve, but I'm not sure
> what they are.  Arnd, do you recall?

My patch series removed all users in the kernel.

The reason I backed out from removing _syscallX() along with it was
that it is still controversial whether it has any value to user space.

Andi's point was that it is an essential part of the kernel ABI and
we should not break existing source using these macros, especially
since the alternative syscall() function provided by glibc has been
frequently broken in the past on x86.

HPA made the point that syscall() from glibc is by design inefficient
on some architectures, so there may be reasons to use _syscallX() instead.

There are lots of arguments in favor of removing them.

	Arnd <><

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[David Woodhouse]

On Mon, 2006-09-04 at 21:56 +0200, Arnd Bergmann wrote:
> Andi's point was that it is an essential part of the kernel ABI and
> we should not break existing source using these macros, especially
> since the alternative syscall() function provided by glibc has been
> frequently broken in the past on x86.

That argument would make more sense if the _syscallX() macros weren't
even more broken. 2.6.17 was the first recent release in which they even
_compiled_ in PIC code on i386, because they clobbered %ebx.

A significant amount of code accessing syscalls directly is going to be
in some kind of library -- which means PIC code -- which means that
_syscallX() as provided by the kernel was just broken. This was 'fixed'
in March, which means there was a window of about a month when the
macros were working on i386, before I removed them from view.

In Fedora, we used to work around this by just providing a hack in
<linux/unistd.h> which invokes glibc's syscall() instead.

Nowadays, we don't ship _syscallX() at all in Fedora. When we dropped
them, I had to fix one or two userspace programs -- but not many.

One of the programs I had to fix was qemu. When I did so, I found it had
its own implementation of _syscallX() for PowerPC, for some reason. I
didn't investigate precisely what qemu's problem was with the kernel's
version -- the correct answer for userspace is to use libc's syscall(),
and that's what I did.

But arguing that we should keep _syscallX() because of historical bugs
with syscal() is fairly bizarre -- if anything, it's the other way
round. The _syscallX() macros have always been buggy in userspace, to
the point where we transparently redirected users of them to syscall()
instead.

> HPA made the point that syscall() from glibc is by design inefficient
> on some architectures, so there may be reasons to use _syscallX()
> instead.

I think he was talking about the placement of 64-bit arguments. Some
32-bit architectures require that 64-bit function arguments be put in an
_aligned_ pair of registers -- i.e. if you have 

	int foo(int a, uint64_t b);

... you get 'a' in the first argument register, but then the second
argument register is unused, and 'b' goes into the third and fourth,
because it needs to be in an even/odd register pair.

That isn't a coherent argument for using the kernel's _syscallX() macros
either, though -- because those just cast the 64-bit argument to a
single 'long', and are thus broken for 64-bit arguments _even_ on
machines where we don't have alignment constraints like the above.

Again, _syscallX() is more broken than syscall() is.

-- 
dwmw2

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[David Woodhouse]

On Mon, 2006-09-04 at 12:38 -0700, Andrew Morton wrote:
> I think we're working on removing the _syscall() things altogether.  The
> main problem is execve() and I have a series from Arnd queued up to fix
> execve.

Yes, that's an eminently sensible plan.

> I think that there are some remaining uses after execve, but I'm not sure
> what they are.  Arnd, do you recall?

There was some SH64 debugging code which can be removed altogether, and
some code in UML which can be fixed to use syscall(). Arnd's patch
sequence did both.

And then, of course, there's some of Andi's test code in userspace -- in
which for some reason he doesn't _want_ to use glibc's syscall()
function. That seems to be why he restored the broken _syscallX()
functions to userspace visibility after we'd removed them once.

Not entirely sure what we can do about that, except perhaps adding what
Fedora had in linux/unistd.h for a long time -- versions of _syscallX()
in #ifndef __KERNEL__ which do use glibc's syscall() function properly.
That was my 'option 2', but it's not something that should really live
in kernel headers, IMHO.

> > What we have _now_, however, is entirely unacceptable. Please apply the
> > following patch to fix it:
> 
> whimper.  that gives me a reject storm to fix up.

Sorry :)

You could drop the addition of comments after the #endifs, if that helps
-- the only important part is moving the #ifdef __KERNEL__ up to the end
of the syscall table. I suspect it doesn't help though -- the hunk which
conflicts is almost certainly going to be the addition of the #ifdef at
the end of the syscall table.

-- 
dwmw2

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[David Woodhouse]

On Mon, 2006-09-04 at 12:38 -0700, Andrew Morton wrote:
> > What we have _now_, however, is entirely unacceptable. Please apply the
> > following patch to fix it:
> 
> whimper.  that gives me a reject storm to fix up.

It shouldn't bother you for long -- the intention was that the patch
would be applied ASAP, so that this regression doesn't make its way into
2.6.18 final.

-- 
dwmw2

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[Andi Kleen]

On Sunday 10 September 2006 11:48, David Woodhouse wrote:
> On Mon, 2006-09-04 at 12:38 -0700, Andrew Morton wrote:
> > > What we have _now_, however, is entirely unacceptable. Please apply the
> > > following patch to fix it:
> >
> > whimper.  that gives me a reject storm to fix up.
>
> It shouldn't bother you for long -- the intention was that the patch
> would be applied ASAP, so that this regression doesn't make its way into
> 2.6.18 final.

I don't think there is anything unacceptable in 2.6.18 in this area right now.
So 2.6.18 should be fine, and hopefully in the future we will keep this
useful facility too.

-Andi

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[David Woodhouse]

On Sun, 2006-09-10 at 10:33 +0200, Andi Kleen wrote:
> I don't think there is anything unacceptable in 2.6.18 in this area right now.
> So 2.6.18 should be fine, and hopefully in the future we will keep this
> useful facility too.

A facility which isn't needed in-kernel and which has patches lined up
in -mm to remove it entirely.

A facility which isn't provided to userspace on other architectures, and
which has been historically broken on i386 anyway.

A facility which is unnecessary because glibc provides a functional
version of its own (which even your own numactl uses).

Kernel headers are _not_ a dumping ground for random crap like this.
We're supposed to be trying to clean them up; in fact we _had_ cleaned
this particular wart up -- and you reverted the fix, afaict without even
the courtesy a Cc to myself, linux-arch or linux-kernel.

Please fix this regression before 2.6.18.

-- 
dwmw2

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[Arnd Bergmann]

Am Monday 04 September 2006 21:27 schrieb David Woodhouse:
> 1. C libraries have their own syscall(); there's no _need_ for these
>    macros to be exported by the kernel.
>
> 2. These macros work in-kernel but they are _not_ designed to work in
>    the general case in userspace. The i386 version is broken for PIC
>    code, for example.

In -mm, they are no longer used in the kernel, and it's hard to even
try to use them since the 'errno' variable is gone as well.

> 3. We should be consistent about what we provide in kernel headers.
>    Since we don't provide these macros on other architectures, we should
>    not do so on i386 and x86_64

Yes, fully agreed.

> I see three possible ways to fix this. The first, and the sanest, is
> just to put the ifdefs back and remove these macros from user view, as
> in the patch below.

Putting #ifdef __KERNEL__ back in makes it consistently wrong on all
architectures, which is better than the current wrong on all architectures
but inconsistent state.

After merging my patches in early 2.6.19, the consequence of this should
be to remove the macros entirely so we don't get any new users.

> The second option, if the consensus really is that we should ignore
> reason #1 above and export these unneeded macros, is that we can apply
> the patch below and then also add generic versions of _syscall*() in
> linux/unistd.h which just invoke libc's syscall(), instead of trying to
> use the in-kernel assembly versions. That would at least address #2 and
> #3 -- and in fact some distributions have been shipping with that
> in /usr/include/linux/unistd.h for a long time already.

I'd rather have them in include/asm-generic/unistd.h and include that
from every <asm/unistd.h> to cause the least breakage in existing
user space applications.

> The third, and least sensible, option would be to accept what Andi has
> done and then attempt to address #2 and #3 above by _fixing_ the macros
> so that they work in the general case -- in all types of code and on all
> architectures. Even in that case, the patch below should be applied to
> remove these macros from user view as an interim measure until they are
> actually fixed and consistent across architectures and code models.

That should also follow by a change in 2.6.19 -- put all the macros in
#ifndef __KERNEL__, so they can /only/ be used in user space.

	Arnd <><

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[Andi Kleen]

> 1. C libraries have their own syscall(); there's no _need_ for these
>    macros to be exported by the kernel.

I like having them there. I find it it useful for my test programs.
It's also useful for numactl for once.

> 3. We should be consistent about what we provide in kernel headers.
>    Since we don't provide these macros on other architectures, we should
>    not do so on i386 and x86_64 -- especially as the reason for doing so
>    seems to be just that the arch maintainer doesn't want to use the
>    proper glibc function in his test hacks.

We've always provided them and continuing to do so is consistent.

-Andi

Re: [PATCH] Revert [i386/x86_64]: Remove __KERNEL__ ifdef around _syscall*()

[David Woodhouse]

On Mon, 2006-09-04 at 22:30 +0200, Andi Kleen wrote:
> > 1. C libraries have their own syscall(); there's no _need_ for these
> >    macros to be exported by the kernel.
> 
> I like having them there. I find it it useful for my test programs.

Nevertheless, you could happily use syscall() as provided by glibc -- or
you could keep your own copy of _syscallX around. You don't need to
pollute the kernel with this stuff just so that you can use it in your
own test programs.

> It's also useful for numactl for once.

What makes you say that? Numactl seems used glibc's syscall(), not the
kernel's _syscallX(). Although it does provide its own syscall6() on
both i386 and x86_64 -- citing bugs in both kernel and glibc versions,
although not actually giving bug numbers. But basically, it's using
glibc's syscall().

> > 3. We should be consistent about what we provide in kernel headers.
> >    Since we don't provide these macros on other architectures, we should
> >    not do so on i386 and x86_64 -- especially as the reason for doing so
> >    seems to be just that the arch maintainer doesn't want to use the
> >    proper glibc function in his test hacks.
> 
> We've always provided them and continuing to do so is consistent.

They've always been broken, and we've _never_ been consistent about what
we provide to userspace in kernel headers in the past -- we're only
recently starting to get our act together in that respect, and your
patches _reverted_ some of the cleanup which was part of that process.

Our best option, at this point, is to remove the kernel versions again
and let userspace use syscall(). Like numactl() already does.

-- 
dwmw2