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
