linux-kernel - Re: [RFCv2 03/10] x86: Introduce userspace API to handle per-thread features

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 linux-cve-announce 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]

Message-ID: <9e59305039f2c8077ee087313d1df5ff06028cfe.camel@intel.com>
Date:   Sat, 14 May 2022 23:06:58 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "aryabinin@...tuozzo.com" <aryabinin@...tuozzo.com>,
        "dvyukov@...gle.com" <dvyukov@...gle.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "glider@...gle.com" <glider@...gle.com>
Subject: Re: [RFCv2 03/10] x86: Introduce userspace API to handle per-thread
 features

On Sat, 2022-05-14 at 10:37 +0200, Thomas Gleixner wrote:
> On Fri, May 13 2022 at 23:50, Edgecombe, Rick P wrote:
> > On Sat, 2022-05-14 at 02:09 +0300, Kirill A. Shutemov wrote:
> > > On Fri, May 13, 2022 at 05:34:12PM +0000, Edgecombe, Rick P
> > > wrote:
> > > > On Fri, 2022-05-13 at 16:09 +0200, Alexander Potapenko wrote:
> > > > > > +
> > > > > > +       /* Handle ARCH_THREAD_FEATURE_ENABLE */
> > > > > > +
> > > > > > +       task->thread.features |= features;
> > > > > > +out:
> > > > > > +       return task->thread.features;
> > > > > 
> > > > > Isn't arch_prctl() supposed to return 0 on success?
> > > > 
> > > > Hmm, good point. Maybe we'll need a struct to pass info in and
> > > > out.
> > > 
> > > But values >0 are unused. I don't see why can't we use them.
> > 
> > Hmm, I don't know what it would break since it is a new "code"
> > argument. But the man page says:
> > "On success, arch_prctl() returns 0; on error, -1 is returned, and
> > errno is set to indicate the error."
> > 
> > So just change the man pages?
> > "On success, arch_prctl() returns positive values; on error, -1 is
> > returned, and errno is set to indicate the error."
> 
> Why?
> 
>         prctl(GET, &out)
> 
> is the pattern used all over the place.

It seems better to me, but we also need to pass something in.

The idea of the "enable" operation is that userspace would pass in all
the features that it wants in one call, and then find out back what was
successfully enabled. So unlike the other arch_prctl()s, it wants to
pass something in AND get a result in one arch_prctl() call. It doesn't
need to check what is supported ahead of time. Since these enabling
operations can fail (OOM, etc), userspace has to handle unexpected per-
feature failure anyway. So it just blindly asks for what it wants.

Any objections to having it write back the result in the same
structure?

Otherwise, the option that used to be used here was a "status"
arch_prctl(), which was called separately to find out what actually got
enabled after an "enable" call. That fit with the GET/SET semantics
already in place.

I guess we could also get rid of the batch enabling idea, and just have
one "enable" call per feature too. But then it is syscall heavy.

Any chance you could weigh in on this?