On Thu, Jan 25, 2024 at 06:36:30PM -0800, Joe Damato wrote:

On Thu, Jan 25, 2024 at 04:23:58PM -0800, Greg Kroah-Hartman wrote:

quoted

On Thu, Jan 25, 2024 at 04:11:28PM -0800, Joe Damato wrote:

quoted

On Thu, Jan 25, 2024 at 03:21:46PM -0800, Greg Kroah-Hartman wrote:

quoted

On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:

quoted

+struct epoll_params {
+	u64 busy_poll_usecs;
+	u16 busy_poll_budget;
+
+	/* for future fields */
+	u8 data[118];
+} EPOLL_PACKED;

variables that cross the user/kernel boundry need to be __u64, __u16,
and __u8 here.

I'll make that change for the next version, thank you.

quoted

And why 118?

I chose this arbitrarily. I figured that a 128 byte struct would support 16
u64s in the event that other fields needed to be added in the future. 118
is what was left after the existing fields. There's almost certainly a
better way to do this - or perhaps it is unnecessary as per your other
message.

I am not sure if leaving extra space in the struct is a recommended
practice for ioctls or not - I thought I noticed some code that did and
some that didn't in the kernel so I err'd on the side of leaving the space
and probably did it in the worst way possible.

It's not really a good idea unless you know exactly what you are going
to do with it.  Why not just have a new ioctl if you need new
information in the future?  That's simpler, right?

Sure, that makes sense to me. I'll remove it in the v4 alongside the other
changes you've requested.

Fwiw, we do support extensible ioctls since they encode the size. Take a
look at kernel/seccomp.c. It's a clean extensible interface built on top
of the copy_struct_from_user() pattern we added for system calls
(openat(), clone3() etc.):

static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
                                 unsigned long arg)
{
        struct seccomp_filter *filter = file->private_data;
        void __user *buf = (void __user *)arg;

        /* Fixed-size ioctls */
        switch (cmd) {
        case SECCOMP_IOCTL_NOTIF_RECV:
                return seccomp_notify_recv(filter, buf);
        case SECCOMP_IOCTL_NOTIF_SEND:
                return seccomp_notify_send(filter, buf);
        case SECCOMP_IOCTL_NOTIF_ID_VALID_WRONG_DIR:
        case SECCOMP_IOCTL_NOTIF_ID_VALID:
                return seccomp_notify_id_valid(filter, buf);
        case SECCOMP_IOCTL_NOTIF_SET_FLAGS:
                return seccomp_notify_set_flags(filter, arg);
        }

        /* Extensible Argument ioctls */
#define EA_IOCTL(cmd)   ((cmd) & ~(IOC_INOUT | IOCSIZE_MASK))
        switch (EA_IOCTL(cmd)) {
        case EA_IOCTL(SECCOMP_IOCTL_NOTIF_ADDFD):
                return seccomp_notify_addfd(filter, buf, _IOC_SIZE(cmd));
        default:
                return -EINVAL;
        }
}

static long seccomp_notify_addfd(struct seccomp_filter *filter,
                                 struct seccomp_notif_addfd __user *uaddfd,
                                 unsigned int size)
{
        struct seccomp_notif_addfd addfd;
        struct seccomp_knotif *knotif;
        struct seccomp_kaddfd kaddfd;
        int ret;

        BUILD_BUG_ON(sizeof(addfd) < SECCOMP_NOTIFY_ADDFD_SIZE_VER0);
        BUILD_BUG_ON(sizeof(addfd) != SECCOMP_NOTIFY_ADDFD_SIZE_LATEST);

        if (size < SECCOMP_NOTIFY_ADDFD_SIZE_VER0 || size >= PAGE_SIZE)
                return -EINVAL;

        ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
        if (ret)
                return ret;