Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Thu, Dec 10, 2020 at 5:59 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Thu, Dec 10, 2020 at 3:34 PM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > On Thu, Dec 10, 2020 at 6:33 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > > On Sat, Nov 21, 2020 at 4:27 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > > > On Fri, Nov 20, 2020 at 11:28 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> > > > I would imagine this can be done like the way I proposed
> > > > for get_bitmap() in sys_migrate_pages:
> > > >
> > > > https://lore.kernel.org/lkml/20201102123151.2860165-4-arnd@kernel.org/
> > >
> > > Coming back to this. Current patchset includes new select and poll
> > > selftests to verify the changes. I need to send a small kselftest
> > > patch for that first.
> > >
> > > Assuming there's no time pressure, I will finish up and send the main
> > > changes after the merge window, for the next release then.
> > >
> > > Current state against linux-next at
> > > https://github.com/wdebruij/linux-next-mirror/tree/select-compat-1
> >
> > Ok, sounds good to me. I've had a (very brief) look and have one
> > suggestion: instead of open-coding the compat vs native mode
> > in multiple places like
> >
> > if (!in_compat_syscall())
> >      return copy_from_user(fdset, ufdset, FDS_BYTES(nr)) ? -EFAULT : 0;
> > else
> >      return compat_get_bitmap(fdset, ufdset, nr);
> >
> > maybe move this into a separate function and call that where needed.
> >
> > I've done this for the get_bitmap() function in my series at
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/commit/?h=compat-alloc-user-space-7&id=b1b23ebb12b635654a2060df49455167a142c5d2
> >
> > The definition is slightly differrent for cpumask, nodemask and fd_set,
> > so we'd need to try out the best way to structure the code to end
> > up with the most readable version, but it should be possible when
> > there are only three callers (and duplicating the function would
> > be the end of the world either)
>
> For fd_set there is only a single caller for each direction. Do you
> prefer helpers even so?
>
> For sigmask, with three callers, something along the lines of this?
>
>   @@ -1138,10 +1135,7 @@ static int do_ppoll(struct pollfd __user
> *ufds, unsigned int nfds,
>                           return -EINVAL;
>           }
>
>   -       if (!in_compat_syscall())
>   -               ret = set_user_sigmask(sigmask, sigsetsize);
>   -       else
>   -               ret = set_compat_user_sigmask(sigmask, sigsetsize);
>   +       ret = set_maybe_compat_user_sigmask(sigmask, sigsetsize);
>           if (ret)
>                   return ret;
>
>   --- a/include/linux/compat.h
>   +++ b/include/linux/compat.h
>   @@ -942,6 +942,17 @@ static inline bool in_compat_syscall(void) {
> return false; }
>
>   +static inline int set_maybe_compat_user_sigmask(const void __user *sigmask,
>   +                                               size_t sigsetsize)
>   +{
>   +#if defined CONFIG_COMPAT
>   +       if (unlikely(in_compat_syscall()))
>   +               return set_compat_user_sigmask(sigmask, sigsetsize);
>   +#endif
>   +
>   +       return set_user_sigmask(sigmask, sigsetsize);
>   +}

set_user_sigmask is the only open-coded variant that is used more than once.

Because it is used in both select.c and eventpoll.c, a helper would
have to live in compat.h. This then needs a new dependency on
sched_signal.h.

So given that this is a simple branch, it might just make logic more
complex, instead of less. I can add this change in a separate patch on
top of the original three, to judge whether it is worthwhile.