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

Hey again,

On Thu, Nov 24, 2022 at 01:24:42PM +0100, Jason A. Donenfeld wrote:

Hi Florian,

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

quoted

* 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?

Looks like set_mempolicy, get_mempoliy, and migrate_pages pass an
unsigned long pointer and I don't see any compat stuff around it:

    SYSCALL_DEFINE3(set_mempolicy, int, mode, const unsigned long __user *, nmask,
                    unsigned long, maxnode)
    
    SYSCALL_DEFINE5(get_mempolicy, int __user *, policy,
                    unsigned long __user *, nmask, unsigned long, maxnode,
                    unsigned long, addr, unsigned long, flags)

    SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
                    const unsigned long __user *, old_nodes,
                    const unsigned long __user *, new_nodes)
     

In contrast sched_setaffinity and get_robust_list take a unsigned long
pointer and does have a compat wrapper:

    SYSCALL_DEFINE3(sched_setaffinity, pid_t, pid, unsigned int, len,
                    unsigned long __user *, user_mask_ptr)

    SYSCALL_DEFINE3(get_robust_list, int, pid,
                    struct robust_list_head __user * __user *, head_ptr,
                    size_t __user *, len_ptr)

Jason