On Wed, Aug 17, 2022 at 11:26 PM Greg KH gregkh@linuxfoundation.org wrote:
On Wed, Aug 17, 2022 at 02:47:25PM -0700, Axel Rasmussen wrote:
+static int userfaultfd_dev_open(struct inode *inode, struct file *file) +{
return 0;If your open does nothing, no need to list it here at all, right?
+}
+static long userfaultfd_dev_ioctl(struct file *file, unsigned int cmd, unsigned long flags) +{
if (cmd != USERFAULTFD_IOC_NEW)return -EINVAL;return new_userfaultfd(flags);+}
+static const struct file_operations userfaultfd_dev_fops = {
.open = userfaultfd_dev_open,.unlocked_ioctl = userfaultfd_dev_ioctl,.compat_ioctl = userfaultfd_dev_ioctl,Why do you need to set compat_ioctl? Shouldn't it just default to the existing one?
I took some more time looking at this today, and I think it actually has to be the way it is.
I didn't find anywhere we noticed compat_ioctl unset, and default to the "normal" one (e.g. see the compat ioctl syscall definition in fs/ioctl.c). It looks to me like it really does need some value. It's common to use compat_ptr_ioctl for this, but since we're interpreting the arg as a scalar not as a pointer, doing that here would be incorrect.
It looks like there are other existing examples that do it the same way, e.g. seccomp_notify_ops in linux/seccomp.c.
And why is this a device node at all? Shouldn't the syscall handle all of this (to be honest, I didn't read anything but the misc code, sorry.)
thanks,
greg k-h