From: Mark Lord <kernel@teksavvy.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: [PATCH] i8k: fix ioctl handing (was Re: Linux 3.3-rc5)
Date: Thu, 01 Mar 2012 10:26:24 -0500 [thread overview]
Message-ID: <4F4F9520.9070309@teksavvy.com> (raw)
In-Reply-To: <4F4F8BEE.3040806@teksavvy.com>
[-- Attachment #1: Type: text/plain, Size: 4218 bytes --]
On 12-03-01 09:47 AM, Mark Lord wrote:
> On 12-02-28 01:30 PM, Linus Torvalds wrote:
>> On Mon, Feb 27, 2012 at 7:23 AM, Mark Lord <kernel@teksavvy.com> wrote:
..
>> Does this (with your compat_ioctl wrapper addition) also work for you?
>
>
> It's close (after adding a missing left-paren), but not 100% working yet.
> In particular, this command fails to get valid data: i8kctl bios
>
> ioctl(3, I8K_BIOS_VERSION, 0xbfc543c8) = -1 EINVAL (Invalid argument)
Oh.. that's due to just plain broken code in i8k.c.
My original patch had a fix for that too:
- switch (cmd) {
- case I8K_BIOS_VERSION:
- val = i8k_get_bios_version();
+ switch (_IOC_NR(cmd)) {
+ case (_IOC_NR(I8K_BIOS_VERSION)):
+ {
+ int i;
+ for (val = 0, i = 0; i < strlen(bios_version); i++)
+ val = (val << 8) | bios_version[i];
break;
So, with your patch + bits of mine, we may have it all working.
Here's a single combined patch for further discussion.
I'm still not 100% happy with the arg/argp passing to i8ki_unlocked_ioctl() though.
Shouldn't there be a compat_ptr() macro in there someplace?
----------------snip-----------------
Fix all kinds of badness in the i8k driver's ioctl handling.
This enables it to continue to work with existing 32-bit binaries
under both 32-bit and 64-bit kernels.
It also fixes the broken I8K_BIOS_VERSION call for all situations.
Signed-off-by: Mark Lord <mlord@pobox.com>
--- old/include/linux/i8k.h
+++ linux/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
--- old/drivers/char/i8k.c 2012-03-01 09:44:03.400800231 -0500
+++ linux/drivers/char/i8k.c 2012-03-01 10:17:26.909946304 -0500
@@ -92,6 +92,23 @@
static int i8k_open_fs(struct inode *inode, struct file *file);
static long i8k_ioctl(struct file *, unsigned int, unsigned long);
+#ifdef CONFIG_COMPAT
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg);
+
+static long
+i8k_compat_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+ long ret;
+
+ mutex_lock(&i8k_mutex);
+ ret = i8k_ioctl_unlocked(fp, cmd, arg);
+ mutex_unlock(&i8k_mutex);
+
+ return ret;
+}
+#endif
+
static const struct file_operations i8k_fops = {
.owner = THIS_MODULE,
.open = i8k_open_fs,
@@ -99,6 +116,9 @@
.llseek = seq_lseek,
.release = single_release,
.unlocked_ioctl = i8k_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = i8k_compat_ioctl,
+#endif
};
struct smm_regs {
@@ -318,17 +338,21 @@
static int
i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
{
- int val = 0;
- int speed;
+ int i, speed, val = 0;
unsigned char buff[16];
int __user *argp = (int __user *)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();
+ for (val = 0, i = 0; i < strlen(bios_version); i++)
+ val = (val << 8) | bios_version[i];
break;
case I8K_MACHINE_ID:
[-- Attachment #2: 07_i8k_ioctl_fixes.patch --]
[-- Type: text/x-patch, Size: 3002 bytes --]
Fix all kinds of badness in the i8k driver's ioctl handling.
This enables it to continue to work with existing 32-bit binaries
under both 32-bit and 64-bit kernels.
It also fixes the broken I8K_BIOS_VERSION call for all situations.
Signed-off-by: Mark Lord <mlord@pobox.com>
--- old/include/linux/i8k.h
+++ linux/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
--- old/drivers/char/i8k.c 2012-03-01 09:44:03.400800231 -0500
+++ linux/drivers/char/i8k.c 2012-03-01 10:17:26.909946304 -0500
@@ -92,6 +92,23 @@
static int i8k_open_fs(struct inode *inode, struct file *file);
static long i8k_ioctl(struct file *, unsigned int, unsigned long);
+#ifdef CONFIG_COMPAT
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg);
+
+static long
+i8k_compat_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+ long ret;
+
+ mutex_lock(&i8k_mutex);
+ ret = i8k_ioctl_unlocked(fp, cmd, arg);
+ mutex_unlock(&i8k_mutex);
+
+ return ret;
+}
+#endif
+
static const struct file_operations i8k_fops = {
.owner = THIS_MODULE,
.open = i8k_open_fs,
@@ -99,6 +116,9 @@
.llseek = seq_lseek,
.release = single_release,
.unlocked_ioctl = i8k_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = i8k_compat_ioctl,
+#endif
};
struct smm_regs {
@@ -318,17 +338,21 @@
static int
i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
{
- int val = 0;
- int speed;
+ int i, speed, val = 0;
unsigned char buff[16];
int __user *argp = (int __user *)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();
+ for (val = 0, i = 0; i < strlen(bios_version); i++)
+ val = (val << 8) | bios_version[i];
break;
case I8K_MACHINE_ID:
next prev parent reply other threads:[~2012-03-01 15:26 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-25 20:34 Linux 3.3-rc5 Linus Torvalds
2012-02-27 15:23 ` Mark Lord
2012-02-28 18:30 ` Linus Torvalds
2012-02-28 19:29 ` Andreas Schwab
2012-02-28 19:39 ` Linus Torvalds
2012-03-01 14:47 ` Mark Lord
2012-03-01 15:26 ` Mark Lord [this message]
2012-03-02 3:25 ` [PATCH] i8k: fix ioctl handing (was Re: Linux 3.3-rc5) Linus Torvalds
2012-03-01 16:22 ` Linux 3.3-rc5 Linus Torvalds
2012-03-02 0:17 ` Mark Lord
2012-03-02 0:21 ` Mark Lord
2012-03-02 0:31 ` Linus Torvalds
2012-03-02 3:48 ` Mark Lord
2012-02-27 21:59 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F4F9520.9070309@teksavvy.com \
--to=kernel@teksavvy.com \
--cc=linux-kernel@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.