Re: [PATCH 5/9] ARM: oabi-compat: rework epoll_wait/epoll_pwait emulation

[Arnd Bergmann <arnd@arndb.de>]

On Tue, Sep 8, 2020 at 8:20 AM Christoph Hellwig <hch@lst.de> wrote:
> > @@ -264,68 +266,24 @@ asmlinkage long sys_oabi_epoll_ctl(int epfd, int op, int fd,
> >       return do_epoll_ctl(epfd, op, fd, &kernel, false);
> >  }
> >
> > -static long do_oabi_epoll_wait(int epfd, struct oabi_epoll_event __user *events,
> > -                            int maxevents, int timeout)
> > +struct epoll_event __user *
> > +epoll_put_uevent(__poll_t revents, __u64 data, struct epoll_event __user *uevent)
> >  {
> > +     if (in_oabi_syscall()) {
> > +             struct oabi_epoll_event *oevent = (void __user *)uevent;
> >
> > +             if (__put_user(revents, &oevent->events) ||
> > +                 __put_user(data, &oevent->data))
> > +                     return NULL;
> >
> > +             return (void __user *)uevent+1;

FWIW, this line needs to be

         return (void __user *)(oevent+1);

It turns out that while I thought I had tested this already, my earlier
tests were on the EABI Debian 5 instead of the OABI version of the
same distro. I reproduced it both ways now and LTP successfully
found that bug ;-)

> I wonder if we'd be better off doing the in_oabi_syscall() branch in
> the common code.  E.g. rename in_oabi_syscall to in_legacy_syscall and
> stub it out for all other architectures.  Then just do
>
>         if (in_oabi_syscall()
>                 legacy_syscall_foo_bit();
>         else
>                 normal_syscall_foo_bit();
>
> in common code, where so far only arm provides
> legacy_syscall_foo_bit().

I tried out different ways, the first one I had was with an #ifdef in the
C code that I did not like much.

Moving the different code path into common code would avoid that
#ifdef but also put the rather obscure oabi-compat code into a
much more prominent location. I'd prefer to keep it out of there
as much as possible and hope we don't need to do this anywhere
else. x86-32 has some similar issues with struct layout, but that
already goes through the normal compat layer on 64-bit kernels.

> Tons of long lines again in this patch..

Fixed now.

       Arnd