[lkml]   [2012]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patch in this message
    SubjectRe: Linux 3.3-rc5
    On Mon, Feb 27, 2012 at 7:23 AM, Mark Lord <> wrote:
    > The i8k driver is still b0rked for COMPAT use in linux-3.2.xx,
    > and I don't think my patch got picked up by anyone for 3.3 yet:

    I really don't like your patch.

    It basically compares the ioctl argument by dropping almost all bits.
    That looks completely wrong. If I read the patch correctly, it will
    now match ioctl's that have *nothing* to do with the i8k ioctls, that
    just happen to have a few bits in common.

    I also think that your explanation is wrong. The problem is not the
    use of _IOR() at all, the problem is that the data structure given
    *to* the _IOR() macro is complete grabage. For example:

    #define I8K_GET_SPEED _IOWR('i', 0x85, size_t)

    that "size_t" there is just crap. It's wrong. The ioctl doesn't write
    a size_t (which is different sizes on 32-bit and 64-bit), it actually
    writes an "int" (which happens to be the same size in both compat and

    So the 64-bit code - totally incorrectly - has been compiled with an
    ioctl number that implies a 64-bit argument.

    EVERY SINGLE IOCTL that is defined for the i8k driver is crap. There
    are two that were "int", but they are marked as "broken" because the
    data structure *they* write isn't actually "int". But those two are
    the ones that just happen to work in both 32-bit and 64-bit mode,
    because at least the size of the (incorrect) data structure is the

    The ones that have "size_t" aren't marked broken, but those are the
    *really* broken ones due to the wrong data structure choice that has a
    size that now is different in 32-bit and 64-bit mode.

    Quite frankly, I think the right solution is to fix the kernel
    interface to the right type (int) that is the same. But because we
    don't want to change the user interface, let's make the kernel
    *accept* the 8-byte entity and just change it into a 4-byte size, and
    leave the user-space visible ioctl numbers the same broken crap - it's
    not like the other ioctl numbers had the right size *either*...

    IOW, have something like the attached in the ioctl handler (and then
    we need to also add the compat handler, as in your patch).

    Does this (with your compat_ioctl wrapper addition) also work for you?

    drivers/char/i8k.c | 4 ++++
    include/linux/i8k.h | 22 ++++++++++++++++------
    2 files changed, 20 insertions(+), 6 deletions(-)

    diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
    index 40cc0cf2ded6..e4b1613543ed 100644
    --- a/drivers/char/i8k.c
    +++ b/drivers/char/i8k.c
    @@ -326,6 +326,10 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
    if (!argp)
    return -EINVAL;

    + /* We had some bad 64-bit confusion */
    + if ((arg >> _IOC_SIZESHIFT) & _IOC_SIZEMASK) == 8)
    + arg -= 4 << _IOC_SIZESHIFT;
    switch (cmd) {
    case I8K_BIOS_VERSION:
    val = i8k_get_bios_version();
    diff --git a/include/linux/i8k.h b/include/linux/i8k.h
    index 1c45ba505115..650f4015c89c 100644
    --- a/include/linux/i8k.h
    +++ b/include/linux/i8k.h
    @@ -20,14 +20,24 @@
    #define I8K_PROC "/proc/i8k"
    #define I8K_PROC_FMT "1.0"

    + * For hysterical raisins we expose the wrong size to user space
    + * in the ioctl numbering. The actual real data size is "int".
    + */
    +#ifdef __KERNEL__
    +#define borked_i8k_arg int
    +#define borked_i8k_arg size_t
    #define I8K_BIOS_VERSION _IOR ('i', 0x80, int) /* broken: meant 4 bytes */
    #define I8K_MACHINE_ID _IOR ('i', 0x81, int) /* broken: meant 16 bytes */
    -#define I8K_POWER_STATUS _IOR ('i', 0x82, size_t)
    -#define I8K_FN_STATUS _IOR ('i', 0x83, size_t)
    -#define I8K_GET_TEMP _IOR ('i', 0x84, size_t)
    -#define I8K_GET_SPEED _IOWR('i', 0x85, size_t)
    -#define I8K_GET_FAN _IOWR('i', 0x86, size_t)
    -#define I8K_SET_FAN _IOWR('i', 0x87, size_t)
    +#define I8K_POWER_STATUS _IOR ('i', 0x82, borked_i8k_arg)
    +#define I8K_FN_STATUS _IOR ('i', 0x83, borked_i8k_arg)
    +#define I8K_GET_TEMP _IOR ('i', 0x84, borked_i8k_arg)
    +#define I8K_GET_SPEED _IOWR('i', 0x85, borked_i8k_arg)
    +#define I8K_GET_FAN _IOWR('i', 0x86, borked_i8k_arg)
    +#define I8K_SET_FAN _IOWR('i', 0x87, borked_i8k_arg)

    #define I8K_FAN_LEFT 1
    #define I8K_FAN_RIGHT 0
     \ /
      Last update: 2012-02-28 19:33    [W:0.025 / U:10.344 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site