From: Dave Martin <Dave.Martin@arm.com>
To: Will Deacon <will@kernel.org>
Cc: linux-arch@vger.kernel.org, linux-man@vger.kernel.org,
"Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>,
linux-arm-kernel@lists.infradead.org,
Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH 13/14] prctl.2: Add SVE prctls (arm64)
Date: Mon, 18 May 2020 17:37:43 +0100 [thread overview]
Message-ID: <20200518163742.GC21779@arm.com> (raw)
In-Reply-To: <20200513211153.GB28594@willie-the-truck>
On Wed, May 13, 2020 at 10:11:54PM +0100, Will Deacon wrote:
> On Wed, May 13, 2020 at 03:02:00PM +0100, Dave Martin wrote:
> > On Wed, May 13, 2020 at 01:01:12PM +0200, Michael Kerrisk (man-pages) wrote:
> > > On 5/13/20 12:46 PM, Dave Martin wrote:
> > > > On Wed, May 13, 2020 at 09:43:52AM +0100, Will Deacon wrote:
> > > >> On Tue, May 12, 2020 at 05:36:58PM +0100, Dave Martin wrote:
> > > > glibc explicitly has
> > > >
> > > > extern int prctl (int __option, ...);
> > > >
> > > > (and nobody has to write _exit(0, 0, 0, 0, 0, 0) after all.)
> > > >
> > > > Is there some agreed rationale for requiring redundant arguments to be
> > > > supplied explicitly as zero? For now there are likely to be few users
> > > > of this, so we _might_ get away with changing the behaviour here if it's
> > > > considered important enough.
> > >
> > > See above.
> >
> > So there is no bulletproof rationale for either approach, but the main
> > concern is inconsistency? Have I understood that right?
>
> I think it's all just an extension of "make sure unused parameters are 0"
> idiom which allows those bits to be safely repurposed for flags and things
> later on without the worry of existing applications getting away with
> passing junk.
I'd say that the explicit zeroing may give a false sense of safety, but
I sympathise with the intent.
At least, I think the explicit zeroing means that any resulting bugs are
more likely to be fixable in userspace.
> > I'll propose to get that written down in the kernel source somewhere
> > if so.
>
> That would be a really good idea, actually!
>
> > (From my end, the pros and cons of the two approaches seem superficial
> > but the inconsistency is indeed annoying. For PR_SVE_SET_VL, I think
> > the first example I looked at didn't zero the trailing arguments, so I
> > didn't either... but it's been upstream for several releases, so most
> > likely we're stuck with it.)
>
> FWIW, I wasn't blaming you for this. Just that these oversights aren't
> always apparent when reviewing patches, but become more clear when
> reviewing the documentation.
I'll have a think, so long as nobody implies that the SVE prctls are
"wrong" ;)
Adding comments in the code about how the implementation of those prctls
can and can't safely be extended would be sensible though. I'll try to
address that at some point.
> > > > There is no forwards compatibility problem with this prctl though,
> > > > because there are spare bits in arg2 which can "turn on" additional
> > > > args if needed.
> > > >
> > > > Also, it's implausible that PR_SVE_GET_VL will ever want an argument.
> > > >
> > > > There are still 2 billion unallocated prctl numbers, so new prctls can
> > > > always be added if there's ever a need to work around these limitations,
> > > > but it seems extremely unlikely.
>
> Oh, there are ways out, but had I noticed this during code review it
> would've been very easy just to enforce zero for the other args and be done
> with it.
Ack
> > > >>> +If
> > > >>> +.B PR_SVE_VL_INHERIT
> > > >>> +is also included in
> > > >>> +.IR arg2 ,
> > > >>> +it takes effect
> > > >>> +.I after
> > > >>> +this deferred change.
> > > >>
> > > >> I find this a bit hard to follow, since it's not clear to me whether the
> > > >> INHERIT flag is effectively set before or after the next execve(). In other
> > > >> words, if both PR_SVE_SET_VL_ONEXEC and PR_SVE_VL_INHERIT are specified,
> > > >> is the vector length preserved or reset on the next execve()?
> > > >
> > > > It makes no difference, because the ONEXEC handling takes priority over
> > > > the INHERIT handling. But either way INHERIT is never cleared by execve()
> > > > and will apply at subsequent execs().
> > > >
> > > > Explaining all this properly seems out of scope here. Maybe this should
> > > > be trimmed down rather than elaborated? Or perhaps just explain it in
> > > > terms of what the kernel does instead of futile attempts to make it
> > > > intuitive?
>
> Hmm, if we don't explain it in the man page then we should at least point
> people to somewhere where they can get the gory details, because I think
> they're necessary in order to use the prctl() request correctly. I'm still
> not confident that I understand the semantics of setting both
> PR_SVE_SET_VL_ONEXEC and PR_SVE_VL_INHERIT without reading the code, which
> may change.
>
> (I concede on all the spelling/grammar discussions ;)
Ultimately I aim to add another page, but for now would it be sufficient
to refer to Documentation/?
Cheers
---Dave
next prev parent reply other threads:[~2020-05-18 16:37 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-12 16:36 [PATCH 00/14] prctl.2 man page updates for Linux 5.6 Dave Martin
2020-05-12 16:36 ` [PATCH 01/14] prctl.2: tfix clarify that prctl can apply to threads Dave Martin
2020-05-13 8:30 ` Michael Kerrisk (man-pages)
2020-05-12 16:36 ` [PATCH 02/14] prctl.2: Add health warning Dave Martin
2020-05-13 10:10 ` Michael Kerrisk (man-pages)
2020-05-13 11:13 ` Dave Martin
2020-05-13 11:40 ` Michael Kerrisk (man-pages)
2020-05-13 11:41 ` Dave Martin
2020-05-12 16:36 ` [PATCH 03/14] prctl.2: tfix mis-description of thread ID values in procfs Dave Martin
2020-05-13 8:36 ` Michael Kerrisk (man-pages)
2020-05-12 16:36 ` [PATCH 04/14] prctl.2: srcfix add comments for navigation Dave Martin
2020-05-13 10:09 ` Michael Kerrisk (man-pages)
2020-05-13 10:56 ` Dave Martin
2020-05-13 11:03 ` Michael Kerrisk (man-pages)
2020-05-13 11:15 ` Dave Martin
2020-05-13 11:48 ` Michael Kerrisk (man-pages)
2020-05-13 11:51 ` Dave Martin
2020-05-12 16:36 ` [PATCH 05/14] prctl.2: tfix listing order of prctls Dave Martin
2020-05-13 10:10 ` Michael Kerrisk (man-pages)
2020-05-13 11:21 ` Dave Martin
2020-05-13 11:31 ` Michael Kerrisk (man-pages)
2020-05-13 11:45 ` Dave Martin
2020-05-12 16:36 ` [PATCH 06/14] prctl.2: ffix quotation mark tweaks Dave Martin
2020-05-13 10:11 ` Michael Kerrisk (man-pages)
2020-05-13 11:39 ` Dave Martin
2020-05-13 11:46 ` Michael Kerrisk (man-pages)
2020-05-13 11:51 ` Dave Martin
2020-05-12 16:36 ` [PATCH 07/14] prctl.2: Document removal of Intel MPX prctls Dave Martin
2020-05-12 16:41 ` Dave Hansen
2020-05-13 10:11 ` Michael Kerrisk (man-pages)
2020-05-12 16:36 ` [PATCH 08/14] prctl.2: Work around bogus constant "maxsig" in PR_SET_PDEATHSIG Dave Martin
2020-05-13 10:30 ` Michael Kerrisk (man-pages)
2020-05-12 16:36 ` [PATCH 09/14] prctl.2: tfix minor punctuation in SPECULATION_CTRL prctls Dave Martin
2020-05-13 10:31 ` Michael Kerrisk (man-pages)
2020-05-12 16:36 ` [PATCH 10/14] prctl.2: Add PR_SPEC_INDIRECT_BRANCH for " Dave Martin
2020-05-13 11:21 ` Michael Kerrisk (man-pages)
2020-05-13 11:49 ` Dave Martin
2020-05-13 12:06 ` Michael Kerrisk (man-pages)
2020-05-13 13:53 ` Dave Martin
2020-05-12 16:36 ` [PATCH 11/14] prctl.2: Add PR_SPEC_DISABLE_NOEXEC " Dave Martin
2020-05-12 16:36 ` [PATCH 12/14] prctl.2: Clarify the unsupported hardware case of EINVAL Dave Martin
2020-05-13 10:48 ` Michael Kerrisk (man-pages)
2020-05-12 16:36 ` [PATCH 13/14] prctl.2: Add SVE prctls (arm64) Dave Martin
2020-05-13 8:43 ` Will Deacon
2020-05-13 10:46 ` Dave Martin
2020-05-13 11:01 ` Michael Kerrisk (man-pages)
2020-05-13 14:02 ` Dave Martin
2020-05-13 21:11 ` Will Deacon
2020-05-18 16:37 ` Dave Martin [this message]
2020-05-26 14:45 ` Dave Martin
2020-05-12 16:36 ` [PATCH 14/14] prctl.2: Add PR_PAC_RESET_KEYS (arm64) Dave Martin
2020-05-13 7:25 ` Will Deacon
2020-05-13 14:36 ` Dave Martin
2020-05-13 21:00 ` Will Deacon
2020-05-18 16:11 ` Dave Martin
2020-05-18 16:29 ` Will Deacon
2020-05-13 11:28 ` [PATCH 00/14] prctl.2 man page updates for Linux 5.6 Michael Kerrisk (man-pages)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200518163742.GC21779@arm.com \
--to=dave.martin@arm.com \
--cc=catalin.marinas@arm.com \
--cc=linux-arch@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-man@vger.kernel.org \
--cc=mtk.manpages@gmail.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).