lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 26 Sep 2020 20:30:03 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Russell King <linux@...linux.org.uk>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>
Subject: Re: [PATCH v2 5/9] ARM: oabi-compat: rework epoll_wait/epoll_pwait emulation

On Sat, Sep 19, 2020 at 7:32 AM Christoph Hellwig <hch@...radead.org> wrote:
>
> > index 855aa7cc9b8e..156880943c16 100644
> > --- a/arch/arm/include/asm/syscall.h
> > +++ b/arch/arm/include/asm/syscall.h
> > @@ -28,6 +28,17 @@ static inline int syscall_get_nr(struct task_struct *task,
> >       return task_thread_info(task)->syscall & ~__NR_OABI_SYSCALL_BASE;
> >  }
> >
> > +static inline bool __in_oabi_syscall(struct task_struct *task)
> > +{
> > +     return IS_ENABLED(CONFIG_OABI_COMPAT) &&
> > +             (task_thread_info(task)->syscall & __NR_OABI_SYSCALL_BASE);
> > +}
> > +
> > +static inline bool in_oabi_syscall(void)
> > +{
> > +     return __in_oabi_syscall(current);
> > +}
> > +
>
> Maybe split these infrastructure additions into a separate helper?

Sorry, I'm not following what you mean by this. Both of the above
are pretty minimal helpers already, in what way could they be split
further?

> So after you argued for this variant I still have minor nitpicks:
>
> I alway find positive ifdefs better where possible, e.g.
>
> #if defined(CONFIG_ARM) && defined(CONFIG_OABI_COMPAT)
> external declaration here
> #else
> the real thing
> #endif

Ok.

> but I still find the fact that the native case goes into the arch
> helper a little weird.

Would you prefer something like this:

static inline struct epoll_event __user *
epoll_put_uevent(__poll_t revents, __u64 data,
                 struct epoll_event __user *uevent)
{
#if defined(CONFIG_ARM) && defined(CONFIG_OABI_COMPAT)
        /* ARM OABI has an incompatible struct layout and needs a
special handler */
        extern struct epoll_event __user *
        epoll_oabi_put_uevent(__poll_t revents, __u64 data,
                              struct epoll_event __user *uevent);

        if (in_oabi_syscall())
                return epoll_oabi_put_uevent(revents, data, uevent);
#endif
        if (__put_user(revents, &uevent->events) ||
            __put_user(data, &uevent->data))
                return NULL;

        return uevent+1;
}

That would keep the native case in one place, but also mix in
more architecture specific stuff into the common source location,
which again seems worse to me.

     Arnd

Powered by blists - more mailing lists