[PATCH] hiddev: Add 32bit ioctl compatibilty - Kernel

This is a discussion on [PATCH] hiddev: Add 32bit ioctl compatibilty - Kernel ; The hiddev driver currently lacks 32bit ioctl compatibility, so if you're running with a 64bit kernel and 32bit userspace, it won't work. I'm pretty sure that the only thing missing is a compat_ioctl implementation as all structs have fixed size ...

+ Reply to Thread
Results 1 to 7 of 7

Thread: [PATCH] hiddev: Add 32bit ioctl compatibilty

  1. [PATCH] hiddev: Add 32bit ioctl compatibilty

    The hiddev driver currently lacks 32bit ioctl compatibility, so
    if you're running with a 64bit kernel and 32bit userspace, it won't
    work.

    I'm pretty sure that the only thing missing is a compat_ioctl
    implementation as all structs have fixed size fields.

    With this change I can use revoco to configure my MX Revolution mouse.

    Signed-off-by: Philip Langdale

    --- linux-2.6.23/drivers/hid/usbhid/hiddev.c 2007-10-09 13:31:38.000000000 -0700
    +++ linux-phil/drivers/hid/usbhid/hiddev.c 2007-10-12 15:02:15.000000000 -0700
    @@ -34,6 +34,7 @@
    #include
    #include
    #include
    +#include
    #include "usbhid.h"

    #ifdef CONFIG_USB_DYNAMIC_MINORS
    @@ -738,6 +738,14 @@
    return -EINVAL;
    }

    +#ifdef CONFIG_COMPAT
    +static long hiddev_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
    +{
    + struct inode *inode = file->f_path.dentry->d_inode;
    + return hiddev_ioctl(inode, file, cmd, compat_ptr(arg));
    +}
    +#endif
    +
    static const struct file_operations hiddev_fops = {
    .owner = THIS_MODULE,
    .read = hiddev_read,
    @@ -747,6 +754,9 @@
    .release = hiddev_release,
    .ioctl = hiddev_ioctl,
    .fasync = hiddev_fasync,
    +#ifdef CONFIG_COMPAT
    + .compat_ioctl = hiddev_compat_ioctl,
    +#endif
    };

    static struct usb_class_driver hiddev_class = {
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] hiddev: Add 32bit ioctl compatibilty

    On Fri, Oct 12, 2007 at 04:51:00PM -0700, Philip Langdale wrote:
    > The hiddev driver currently lacks 32bit ioctl compatibility, so
    > if you're running with a 64bit kernel and 32bit userspace, it won't
    > work.
    >
    > I'm pretty sure that the only thing missing is a compat_ioctl
    > implementation as all structs have fixed size fields.
    >
    > +static long hiddev_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
    > +{
    > + struct inode *inode = file->f_path.dentry->d_inode;
    > + return hiddev_ioctl(inode, file, cmd, compat_ptr(arg));
    > +}


    Just how many instances of that sucker do we need? It's nothing but

    struct inode *inode = file->f_path.dentry->d_inode;
    return file->f_op->ioctl(inode, file, cmd, compat_ptr(arg));
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] hiddev: Add 32bit ioctl compatibilty

    Al Viro wrote:
    >>
    >> +static long hiddev_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
    >> +{
    >> + struct inode *inode = file->f_path.dentry->d_inode;
    >> + return hiddev_ioctl(inode, file, cmd, compat_ptr(arg));
    >> +}

    >
    > Just how many instances of that sucker do we need? It's nothing but
    >
    > struct inode *inode = file->f_path.dentry->d_inode;
    > return file->f_op->ioctl(inode, file, cmd, compat_ptr(arg));
    >


    So, I don't actually know what you're looking for, but of the 140 occasions
    that .compat_ioctl is implemented in Linus' tree, I can't find another one
    that actually uses this form. So, writing a shared implementation doesn't pick
    off any low hanging fruit. Now, it's possible that some of the other implementations
    could be reduced to this form - but for now, it seems the answer to your question
    is 'one' in either case. :-)

    --phil
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. [PATCH] compat_ioctl: introduce generic_compat_ioctl helper

    Many drivers use only compatible ioctl numbers. In order to
    avoid having to write a special compat_ioctl handler for each
    of them or listing every ioctl number in fs/compat_ioctl.c,
    let's introduce a generic handler that simply calls the
    driver specific f_op->unlocked_ioctl() or f_op->ioctl() handler.

    Signed-off-by: Arnd Bergman
    ---
    On Saturday 13 October 2007, Al Viro wrote:
    > Just how many instances of that sucker do we need? It's nothing but
    >
    > struct inode *inode = file->f_path.dentry->d_inode;
    > return file->f_op->ioctl(inode, file, cmd, compat_ptr(arg));


    Is this what you had in mind? I've been meaning to do something like
    it for some time, but had forgotten about it.

    I guess we can kill a significant amount of COMPATIBLE_IOCTL
    lines in fs/compat_ioctl.c with this.

    Index: linux-2.6/fs/compat_ioctl.c
    ================================================== =================
    --- linux-2.6.orig/fs/compat_ioctl.c
    +++ linux-2.6/fs/compat_ioctl.c
    @@ -1877,6 +1877,30 @@ lp_timeout_trans(unsigned int fd, unsign
    return sys_ioctl(fd, cmd, (unsigned long)tn);
    }

    +/*
    + * Helper function for device drivers that only have COMPATIBLE_IOCTL
    + * numbers. This can be used as f_op->compat_ioctl without any #ifdef
    + * and will simply call the native ioctl handler with the argument
    + * pointer.
    + * Don't use for drivers that interpret arg as an unsigned long instead
    + * of a pointer.
    + */
    +long generic_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
    +{
    + int ret = -ENOTTY;
    + arg = (unsigned long)compat_ptr(arg);
    +
    + if (file->f_op->unlocked_ioctl)
    + ret = file->f_op->unlocked_ioctl(file, cmd, arg);
    + else if (file->f_op->ioctl) {
    + lock_kernel();
    + ret = file->f_op->ioctl(file->f_dentry->d_inode, file, cmd, arg);
    + unlock_kernel();
    + }
    +
    + return ret;
    +}
    +EXPORT_SYMBOL_GPL(generic_compat_ioctl);

    typedef int (*ioctl_trans_handler_t)(unsigned int, unsigned int,
    unsigned long, struct file *);
    Index: linux-2.6/include/linux/fs.h
    ================================================== =================
    --- linux-2.6.orig/include/linux/fs.h
    +++ linux-2.6/include/linux/fs.h
    @@ -1807,6 +1807,12 @@ extern void do_generic_mapping_read(stru
    extern int generic_segment_checks(const struct iovec *iov,
    unsigned long *nr_segs, size_t *count, int access_flags);

    +#ifdef CONFIG_COMPAT
    +extern long generic_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
    +#else
    +#define generic_compat_ioctl (long (*)(struct file *, unsigned int, unsigned long))NULL
    +#endif
    +
    /* fs/splice.c */
    extern ssize_t generic_file_splice_read(struct file *, loff_t *,
    struct pipe_inode_info *, size_t, unsigned int);
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. [PATCH] hiddev: simplify 32bit ioctl compatibilty

    hiddev has both entries in fs/compat_ioctl.c and its own
    compat_ioctl() handler. Remove both and use the new generic_compat_ioctl
    helper instead.

    Signed-off-by: Arnd Bergmann
    Index: linux-2.6/drivers/hid/hidraw.c
    ================================================== =================
    --- linux-2.6.orig/drivers/hid/hidraw.c
    +++ linux-2.6/drivers/hid/hidraw.c
    @@ -268,6 +268,7 @@ static const struct file_operations hidr
    .open = hidraw_open,
    .release = hidraw_release,
    .ioctl = hidraw_ioctl,
    + .compat_ioctl = generic_compat_ioctl,
    };

    void hidraw_report_event(struct hid_device *hid, u8 *data, int len)
    Index: linux-2.6/drivers/hid/usbhid/hiddev.c
    ================================================== =================
    --- linux-2.6.orig/drivers/hid/usbhid/hiddev.c
    +++ linux-2.6/drivers/hid/usbhid/hiddev.c
    @@ -739,14 +739,6 @@ inval:
    return -EINVAL;
    }

    -#ifdef CONFIG_COMPAT
    -static long hiddev_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
    -{
    - struct inode *inode = file->f_path.dentry->d_inode;
    - return hiddev_ioctl(inode, file, cmd, compat_ptr(arg));
    -}
    -#endif
    -
    static const struct file_operations hiddev_fops = {
    .owner = THIS_MODULE,
    .read = hiddev_read,
    @@ -756,9 +748,7 @@ static const struct file_operations hidd
    .release = hiddev_release,
    .ioctl = hiddev_ioctl,
    .fasync = hiddev_fasync,
    -#ifdef CONFIG_COMPAT
    - .compat_ioctl = hiddev_compat_ioctl,
    -#endif
    + .compat_ioctl = generic_compat_ioctl,
    };

    static struct usb_class_driver hiddev_class = {
    Index: linux-2.6/fs/compat_ioctl.c
    ================================================== =================
    --- linux-2.6.orig/fs/compat_ioctl.c
    +++ linux-2.6/fs/compat_ioctl.c
    @@ -102,8 +102,6 @@
    #include
    #include

    -#include
    -
    #include
    #include
    #include
    @@ -2575,23 +2573,6 @@ COMPATIBLE_IOCTL(SIOCSIWPOWER)
    COMPATIBLE_IOCTL(SIOCGIWPOWER)
    COMPATIBLE_IOCTL(SIOCSIWAUTH)
    COMPATIBLE_IOCTL(SIOCGIWAUTH)
    -/* hiddev */
    -COMPATIBLE_IOCTL(HIDIOCGVERSION)
    -COMPATIBLE_IOCTL(HIDIOCAPPLICATION)
    -COMPATIBLE_IOCTL(HIDIOCGDEVINFO)
    -COMPATIBLE_IOCTL(HIDIOCGSTRING)
    -COMPATIBLE_IOCTL(HIDIOCINITREPORT)
    -COMPATIBLE_IOCTL(HIDIOCGREPORT)
    -COMPATIBLE_IOCTL(HIDIOCSREPORT)
    -COMPATIBLE_IOCTL(HIDIOCGREPORTINFO)
    -COMPATIBLE_IOCTL(HIDIOCGFIELDINFO)
    -COMPATIBLE_IOCTL(HIDIOCGUSAGE)
    -COMPATIBLE_IOCTL(HIDIOCSUSAGE)
    -COMPATIBLE_IOCTL(HIDIOCGUCODE)
    -COMPATIBLE_IOCTL(HIDIOCGFLAG)
    -COMPATIBLE_IOCTL(HIDIOCSFLAG)
    -COMPATIBLE_IOCTL(HIDIOCGCOLLECTIONINDEX)
    -COMPATIBLE_IOCTL(HIDIOCGCOLLECTIONINFO)
    /* dvb */
    COMPATIBLE_IOCTL(AUDIO_STOP)
    COMPATIBLE_IOCTL(AUDIO_PLAY)
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH] compat_ioctl: introduce generic_compat_ioctl helper

    On Sat, Oct 20, 2007 at 05:50:57PM +0200, Arnd Bergmann wrote:
    > Many drivers use only compatible ioctl numbers. In order to
    > avoid having to write a special compat_ioctl handler for each
    > of them or listing every ioctl number in fs/compat_ioctl.c,
    > let's introduce a generic handler that simply calls the
    > driver specific f_op->unlocked_ioctl() or f_op->ioctl() handler.


    At least for the unlocked_ioctl case this is not nessecary because
    the driver an simply set both the unlocked_ioctl and compat_ioctl
    handlers to the same function. For the drivers not using unlocked_ioctl
    yet a function like this makes sense in theory, but I'd prefer to
    just switch them to ->unlocked_ioctl.

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH] compat_ioctl: introduce generic_compat_ioctl helper

    On Saturday 20 October 2007, Christoph Hellwig wrote:
    > At least for the unlocked_ioctl case this is not nessecary because
    > the driver an simply set both the unlocked_ioctl and compat_ioctl
    > handlers to the same function. *For the drivers not using unlocked_ioctl
    > yet a function like this makes sense in theory, but I'd prefer to
    > just switch them to ->unlocked_ioctl.
    >


    There is still the problem with s390 compat_ptr() conversion, a driver
    using the same handler for both may interpret a pointer with the high
    bit set as out of range.

    Arnd <><
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread