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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).