Re: -mm merge plans for 2.6.21

Re: -mm merge plans for 2.6.21

[Heiko Carstens]

On Fri, Feb 09, 2007 at 02:50:12PM -0800, Davide Libenzi wrote:
> On Fri, 9 Feb 2007, David Woodhouse wrote:
> 
> > On Fri, 2007-02-09 at 13:45 -0800, Andrew Morton wrote:
> > > > I would strongly recommend that in the general case, you don't merge new
> > > > system calls unless the corresponding compat_ system call is
> > > > implemented.
> > > 
> > > Good point. 
> > 
> > It's a _damn_ good point, but I see we went ahead and merged
> > sys_epoll_pwait without it anyway -- despite the fact that it's
> > include/linux/eventpoll.h which contains the example of why we should
> > think first :)
> > 
> > I think I even threw together an untested implementation of
> > compat_sys_epoll_pwait() at one point to assist with that task, but it
> > didn't seem to help much.
> 
> Damn! I always forget. Doing it right now ...

Which remembers me that I think that MIPS is using the non-compat version
of sys_epoll_pwait for compat syscalls. But maybe MIPS doesn't need a compat
syscall for some reason. Dunno.

Re: -mm merge plans for 2.6.21

[David Woodhouse]

On Sat, 2007-02-10 at 11:22 +0100, Heiko Carstens wrote:
> Which remembers me that I think that MIPS is using the non-compat version
> of sys_epoll_pwait for compat syscalls. But maybe MIPS doesn't need a compat
> syscall for some reason. Dunno. 

It's OK as long as the 64-bit kernel, N32 and O32 userspace all agree
there there's 32 bits of padding between the fields of this structure:

struct epoll_event {
        __u32 events;
        __u64 data;
};

I suspect it's a fairly safe bet that N32 userspace agrees; if the O32
ABI is different then it would need the compat syscall.

-- 
dwmw2

Re: -mm merge plans for 2.6.21

[Ralf Baechle]

On Sat, Feb 10, 2007 at 11:22:05AM +0100, Heiko Carstens wrote:

> Which remembers me that I think that MIPS is using the non-compat version
> of sys_epoll_pwait for compat syscalls. But maybe MIPS doesn't need a compat
> syscall for some reason. Dunno.

Which reminds me that x86_64 i386 compat doesn't wire up sys_epoll_pwait ;-)

  Ralf

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

diff --git a/arch/x86_64/ia32/ia32entry.S b/arch/x86_64/ia32/ia32entry.S
index b4aa875..5993262 100644
--- a/arch/x86_64/ia32/ia32entry.S
+++ b/arch/x86_64/ia32/ia32entry.S
@@ -718,4 +718,5 @@ ia32_sys_call_table:
 	.quad compat_sys_vmsplice
 	.quad compat_sys_move_pages
 	.quad sys_getcpu
+	.quad sys_epoll_pwait
 ia32_syscall_end:		

Re: -mm merge plans for 2.6.21

[Ralf Baechle]

On Sat, Feb 10, 2007 at 10:32:07AM +0000, David Woodhouse wrote:

> On Sat, 2007-02-10 at 11:22 +0100, Heiko Carstens wrote:
> > Which remembers me that I think that MIPS is using the non-compat version
> > of sys_epoll_pwait for compat syscalls. But maybe MIPS doesn't need a compat
> > syscall for some reason. Dunno. 
> 
> It's OK as long as the 64-bit kernel, N32 and O32 userspace all agree
> there there's 32 bits of padding between the fields of this structure:
> 
> struct epoll_event {
>         __u32 events;
>         __u64 data;
> };
> 
> I suspect it's a fairly safe bet that N32 userspace agrees; if the O32
> ABI is different then it would need the compat syscall.

That is correct - and apparently for all ABIs because I wasn't able to find
a compat_sys_epoll_pwait at all.

Unfortunately struct epoll_event contains a gap so it bets on identical
padding rules between native and compat ABI and anyway, padding is wasted
space so the struct members should have been swapped when this structure
was created.  Oh well, too late.

  Ralf

Re: -mm merge plans for 2.6.21

[Davide Libenzi]

On Sat, 10 Feb 2007, Ralf Baechle wrote:

> Unfortunately struct epoll_event contains a gap so it bets on identical
> padding rules between native and compat ABI and anyway, padding is wasted
> space so the struct members should have been swapped when this structure
> was created.  Oh well, too late.

You really should have needed padding in there, since even if you swapped 
the members, sizeof(struct epoll_event) would still need to be 16, if 
alignof(u64) == 8. Either adding an extra auxilliary u32, or make the two 
members be u64, would have been ok.
I'll be posting a patch that adds the compat_ layer for epoll in 
kernel/compat.c. Architectures that needs it (currently only ARM-EABI and 
IA64 use a compat code for epoll), should wire them up.



- Davide

Re: -mm merge plans for 2.6.21

[Andi Kleen]

On Saturday 10 February 2007 22:05, Ralf Baechle wrote:
> On Sat, Feb 10, 2007 at 11:22:05AM +0100, Heiko Carstens wrote:
> 
> > Which remembers me that I think that MIPS is using the non-compat version
> > of sys_epoll_pwait for compat syscalls. But maybe MIPS doesn't need a compat
> > syscall for some reason. Dunno.
> 
> Which reminds me that x86_64 i386 compat doesn't wire up sys_epoll_pwait ;-)

Added thanks.

-Andi

Re: -mm merge plans for 2.6.21

[David Woodhouse]

On Sat, 2007-02-10 at 21:34 +0000, Ralf Baechle wrote:
> Unfortunately struct epoll_event contains a gap so it bets on identical
> padding rules between native and compat ABI and anyway, padding is wasted
> space so the struct members should have been swapped when this structure
> was created.  Oh well, too late. 

Indeed. That was the example I was thinking of when I suggested the "no
new syscalls without _simultaneous_ compat version" rule.

-- 
dwmw2

Re: -mm merge plans for 2.6.21

[Ralf Baechle]

On Sun, Feb 11, 2007 at 04:33:41PM +0100, David Woodhouse wrote:

> On Sat, 2007-02-10 at 21:34 +0000, Ralf Baechle wrote:
> > Unfortunately struct epoll_event contains a gap so it bets on identical
> > padding rules between native and compat ABI and anyway, padding is wasted
> > space so the struct members should have been swapped when this structure
> > was created.  Oh well, too late. 
> 
> Indeed. That was the example I was thinking of when I suggested the "no
> new syscalls without _simultaneous_ compat version" rule.

The need for a compat ABI is not always obvious.

  Ralf

Re: -mm merge plans for 2.6.21

[Heiko Carstens]

On Sat, Feb 10, 2007 at 09:34:47PM +0000, Ralf Baechle wrote:
> On Sat, Feb 10, 2007 at 10:32:07AM +0000, David Woodhouse wrote:
> 
> > On Sat, 2007-02-10 at 11:22 +0100, Heiko Carstens wrote:
> > > Which remembers me that I think that MIPS is using the non-compat version
> > > of sys_epoll_pwait for compat syscalls. But maybe MIPS doesn't need a compat
> > > syscall for some reason. Dunno. 
> > 
> > It's OK as long as the 64-bit kernel, N32 and O32 userspace all agree
> > there there's 32 bits of padding between the fields of this structure:
> > 
> > struct epoll_event {
> >         __u32 events;
> >         __u64 data;
> > };
> > 
> > I suspect it's a fairly safe bet that N32 userspace agrees; if the O32
> > ABI is different then it would need the compat syscall.
> 
> That is correct - and apparently for all ABIs because I wasn't able to find
> a compat_sys_epoll_pwait at all.

Hmm.. so you don't need to do some fancy compat conversion for the sigset_t
that gets passed? Why is that? I don't get it...

Re: -mm merge plans for 2.6.21

[Davide Libenzi]

On Sun, 11 Feb 2007, Heiko Carstens wrote:

> On Sat, Feb 10, 2007 at 09:34:47PM +0000, Ralf Baechle wrote:
> > On Sat, Feb 10, 2007 at 10:32:07AM +0000, David Woodhouse wrote:
> > 
> > > On Sat, 2007-02-10 at 11:22 +0100, Heiko Carstens wrote:
> > > > Which remembers me that I think that MIPS is using the non-compat version
> > > > of sys_epoll_pwait for compat syscalls. But maybe MIPS doesn't need a compat
> > > > syscall for some reason. Dunno. 
> > > 
> > > It's OK as long as the 64-bit kernel, N32 and O32 userspace all agree
> > > there there's 32 bits of padding between the fields of this structure:
> > > 
> > > struct epoll_event {
> > >         __u32 events;
> > >         __u64 data;
> > > };
> > > 
> > > I suspect it's a fairly safe bet that N32 userspace agrees; if the O32
> > > ABI is different then it would need the compat syscall.
> > 
> > That is correct - and apparently for all ABIs because I wasn't able to find
> > a compat_sys_epoll_pwait at all.
> 
> Hmm.. so you don't need to do some fancy compat conversion for the sigset_t
> that gets passed? Why is that? I don't get it...

The compat_sys_epoll_pwait function has two sources of compat. One is the 
sigset_t and the other one is the struct epoll_event. The sigset_t compat 
is always needed, while the struct epoll_event may be needed. The code of 
(upcoming) compat_sys_epoll_pwait takes care of both, with a smart 
compile-time check to wire sys_epoll_wait or compat_sys_epoll_wait, 
depending on the need of the struct epoll_event compat handling.
So yes, compat_sys_epoll_pwait is needed.



- Davide

Re: -mm merge plans for 2.6.21

[Ralf Baechle]

On Sun, Feb 11, 2007 at 05:14:46PM +0100, Heiko Carstens wrote:

> Hmm.. so you don't need to do some fancy compat conversion for the sigset_t
> that gets passed? Why is that? I don't get it...

Ah, I finally get your point.  Yes, that needs conversion.

  Ralf