Re: Linux 3.3-rc5

[Linus Torvalds]

On Mon, Feb 27, 2012 at 7:23 AM, Mark Lord <kernel@xxxxxxxxxxxx> 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
non-compat).

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
same.

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?

                        Linus
 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
+#else
+#define borked_i8k_arg size_t
+#endif
+
 #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