Re: [PATCH v6 1/3] random: add vgetrandom_alloc() syscall

Hi Florian,

On Thu, Nov 24, 2022 at 01:15:24PM +0100, Florian Weimer wrote:

* Jason A. Donenfeld:

quoted

Hi Florian,

On Thu, Nov 24, 2022 at 06:25:39AM +0100, Florian Weimer wrote:

quoted

* Jason A. Donenfeld:

quoted

Hi Florian,

On Wed, Nov 23, 2022 at 11:46:58AM +0100, Florian Weimer wrote:

quoted

* Jason A. Donenfeld:

quoted

+ * The vgetrandom() function in userspace requires an opaque state, which this
+ * function provides to userspace, by mapping a certain number of special pages
+ * into the calling process. It takes a hint as to the number of opaque states
+ * desired, and returns the number of opaque states actually allocated, the
+ * size of each one in bytes, and the address of the first state.
+ */
+SYSCALL_DEFINE3(vgetrandom_alloc, unsigned long __user *, num,
+		unsigned long __user *, size_per_each, unsigned int, flags)

I think you should make this __u64, so that you get a consistent
userspace interface on all architectures, without the need for compat
system calls.

That would be quite unconventional. Most syscalls that take lengths do
so with the native register size (`unsigned long`, `size_t`), rather
than u64. If you can point to a recent trend away from this by
indicating some commits that added new syscalls with u64, I'd be happy
to be shown otherwise. But AFAIK, that's not the way it's done.

See clone3 and struct clone_args.

The struct is one thing. But actually, clone3 takes a `size_t`:

    SYSCALL_DEFINE2(clone3, struct clone_args __user *, uargs, size_t, size)

I take from this that I too should use `size_t` rather than `unsigned
long.` And it doesn't seem like there's any compat clone3.

But vgetrandom_alloc does not use unsigned long, but unsigned long *.
You need to look at the contents for struct clone_args for comparison.

Ah! I see what you mean; that's a good point. The usual register
clearing thing isn't going to happen because these are addresses.

I still am somewhat hesitant, though, because `size_t` is really the
"proper" type to be used. Maybe the compat syscall thing is just a
necessary evil?

The other direction would be making this a u32, since 640k ought to be
enough for anybody and such, but maybe that'd be a mistake too.

So I'm not sure. Anybody else on the list with experience adding
syscalls have an opinion?

Jason