Date: Fri, 28 Jun 2024 16:09:01 +0200
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Aleksa Sarai <cyphar@...har.com>, linux-kernel@...r.kernel.org,
	patches@...ts.linux.dev, linux-crypto@...r.kernel.org,
	linux-api@...r.kernel.org, x86@...nel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Adhemerval Zanella Netto <adhemerval.zanella@...aro.org>,
	Carlos O'Donell <carlos@...hat.com>,
	Florian Weimer <fweimer@...hat.com>, Arnd Bergmann <arnd@...db.de>,
	Jann Horn <jannh@...gle.com>,
	Christian Brauner <brauner@...nel.org>,
	David Hildenbrand <dhildenb@...hat.com>
Subject: Re: [PATCH v18 2/5] random: add vgetrandom_alloc() syscall

On Fri, Jun 28, 2024 at 03:56:12PM +0200, Thomas Gleixner wrote:
> Jason!
> 
> On Thu, Jun 20 2024 at 14:18, Jason A. Donenfeld wrote:
> > On Wed, Jun 19, 2024 at 07:13:26PM -0700, Aleksa Sarai wrote:
> >> Then again, I guess since libc is planned to be the primary user,
> >> creating a new syscall in a decade if necessary is probably not that big
> >> of an issue.
> >
> > I'm not sure going the whole big struct thing is really necessary, and
> > for an additional reason: this is only meant to be used with the vDSO
> > function, which is also coupled with the kernel. It doesn't return
> > information that's made to be used (or allowed to be used) anywhere
> > else. So both the vdso code and the syscall code are part of the same
> > basic thing that will evolve together. So I'm not convinced extensible
> > struct really makes sense for this, as neat as it is.
> >
> > If there's wide consensus that it's desirable, in contrast to what I'm
> > saying, I'm not vehemently opposed to it and could do it, but it just
> > seems like massive overkill and not at all necessary. Things are
> > intentionally as simple and straightforward as can be.
> 
> Right, but the problem is that this is a syscall, so people are free to
> explore it even without the vdso part. Now when you want to change it
> later then you are caught in the no-regression trap.
> 
> So making it extensible with backwards compability in place (add the
> unused flag field and check for 0) will allow you to expand without
> breaking users.

Okay, so it sounds like you're also in camp-struct. I guess let's do it
then. This opens up a few questions, but I think we can get them sorted.
Right now this version of the patch has this signature:

  void *vgetrandom_alloc(unsigned int *num, unsigned int *size_per_each,
                         unsigned long addr, unsigned int flags);

The semantics are currently:

- [in] unsigned int num - desired number of states
- [in] unsigned long addr - reserved, nothing
- [in] unsigned int flags - reserved, nothing
- [out] unsigned int num - actual number of states
- [out] unsigned int size_per_each - size of each state
- [out] void* return value - the allocated thing

Following Aleksa's suggestion, we keep the `[out] void* return value` as
a return value, but move all the other into a struct:

    void *vgetrandom_alloc(struct vgetrandom_args *arg, size_t size);

So now the struct can become:

    struct vgetrandom_args {
        [in] u64 flags;
        [in/out] u32 num;
        [out] u32 size_per_each;
    }

Alternatively, this now opens the possibility to incorporate Eric's
suggestion of also returning the number of allocated bytes, which is
perhaps definitely to deal with, but I didn't do because I wanted
symmetry in the argument list. So doing that, now we have:

    struct vgetrandom_args {
        [in] u64 flags;
        [in/out] u32 num;
        [out] u32 size_per_each;
        [out] u64 bytes_allocated;
    }

Does that seem reasonable? There's a little bit of mixing of ins and
outs within the struct, and the return value is still a return value,
rather than a `[out] void *ret` inside of the struct. But maybe that's
fine. Also I used u32 there for the two smaller arguments, but maybe
that's silly and we should go straight to u64?

Anyway, how does that look to you?

Jason

Powered by blists - more mailing lists