Re: [PATCH v18 2/5] random: add vgetrandom_alloc() syscall

[Jason A. Donenfeld]

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