| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20080523093545.175c769c@hyperion.delvare>
Date: Fri, 23 May 2008 09:35:45 +0200
From: Jean Delvare <khali@...ux-fr.org>
To: Alan Cox <alan@...rguk.ukuu.org.uk>
Cc: i2c@...sensors.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c: Push ioctl BKL down into the i2c code
Hi Alan,
On Thu, 22 May 2008 22:23:27 +0100, Alan Cox wrote:
> Signed-off-by: Alan Cox <alan@...hat.com>
>
Description of what the patch does and why it is needed, please. I
can't apply it without that. My first impression is a patch making the
code bigger and more complex with no obvious benefit ;)
Bonus points for a diffstat of the patch.
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index d34c14c..16cf0f2 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -34,7 +34,8 @@
> #include <linux/list.h>
> #include <linux/i2c.h>
> #include <linux/i2c-dev.h>
> -#include <asm/uaccess.h>
> +#include <linux/smp_lock.h>
> +#include <linux/uaccess.h>
>
> static struct i2c_driver i2cdev_driver;
>
> @@ -266,8 +267,9 @@ static noinline int i2cdev_ioctl_rdrw(struct i2c_client *client,
> kfree(rdwr_pa);
> return res;
> }
> -
> + lock_kernel();
> res = i2c_transfer(client->adapter, rdwr_pa, rdwr_arg.nmsgs);
> + unlock_kernel();
> while (i-- > 0) {
> if (res >= 0 && (rdwr_pa[i].flags & I2C_M_RD)) {
> if (copy_to_user(data_ptrs[i], rdwr_pa[i].buf,
> @@ -320,11 +322,15 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
>
> if ((data_arg.size == I2C_SMBUS_QUICK) ||
> ((data_arg.size == I2C_SMBUS_BYTE) &&
> - (data_arg.read_write == I2C_SMBUS_WRITE)))
> + (data_arg.read_write == I2C_SMBUS_WRITE))) {
> /* These are special: we do not use data */
> - return i2c_smbus_xfer(client->adapter, client->addr,
> + lock_kernel();
> + res =i2c_smbus_xfer(client->adapter, client->addr,
Coding style.
> client->flags, data_arg.read_write,
> data_arg.command, data_arg.size, NULL);
> + unlock_kernel();
> + return res;
> + }
>
> if (data_arg.data == NULL) {
> dev_dbg(&client->adapter->dev,
> @@ -355,8 +361,10 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
> if (data_arg.read_write == I2C_SMBUS_READ)
> temp.block[0] = I2C_SMBUS_BLOCK_MAX;
> }
> + lock_kernel();
> res = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> data_arg.read_write, data_arg.command, data_arg.size, &temp);
> + unlock_kernel();
> if (!res && ((data_arg.size == I2C_SMBUS_PROC_CALL) ||
> (data_arg.size == I2C_SMBUS_BLOCK_PROC_CALL) ||
> (data_arg.read_write == I2C_SMBUS_READ))) {
> @@ -366,11 +374,12 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
> return res;
> }
>
> -static int i2cdev_ioctl(struct inode *inode, struct file *file,
> - unsigned int cmd, unsigned long arg)
> +static long i2cdev_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> {
> struct i2c_client *client = (struct i2c_client *)file->private_data;
> unsigned long funcs;
> + int ret = 0;
>
> dev_dbg(&client->adapter->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n",
> cmd, arg);
> @@ -388,28 +397,37 @@ static int i2cdev_ioctl(struct inode *inode, struct file *file,
> * the PEC flag already set, the i2c-dev driver won't see
> * (or use) this setting.
> */
> + lock_kernel();
> if ((arg > 0x3ff) ||
> (((client->flags & I2C_M_TEN) == 0) && arg > 0x7f))
> - return -EINVAL;
> - if (cmd == I2C_SLAVE && i2cdev_check_addr(client->adapter, arg))
> - return -EBUSY;
> - /* REVISIT: address could become busy later */
> - client->addr = arg;
> - return 0;
> + ret = -EINVAL;
> + else if (cmd == I2C_SLAVE &&
> + i2cdev_check_addr(client->adapter, arg))
> + ret = -EBUSY;
> + else
> + /* REVISIT: address could become busy later */
> + client->addr = arg;
No unlock?
> + return ret;
> case I2C_TENBIT:
> + lock_kernel();
> if (arg)
> client->flags |= I2C_M_TEN;
> else
> client->flags &= ~I2C_M_TEN;
> + unlock_kernel();
> return 0;
> case I2C_PEC:
> + lock_kernel();
> if (arg)
> client->flags |= I2C_CLIENT_PEC;
> else
> client->flags &= ~I2C_CLIENT_PEC;
> + unlock_kernel();
> return 0;
> case I2C_FUNCS:
> + lock_kernel();
> funcs = i2c_get_functionality(client->adapter);
> + unlock_kernel();
> return put_user(funcs, (unsigned long __user *)arg);
>
> case I2C_RDWR:
> @@ -419,10 +437,14 @@ static int i2cdev_ioctl(struct inode *inode, struct file *file,
> return i2cdev_ioctl_smbus(client, arg);
>
> case I2C_RETRIES:
> + lock_kernel();
> client->adapter->retries = arg;
> + unlock_kernel();
> break;
> case I2C_TIMEOUT:
> + lock_kernel();
> client->adapter->timeout = arg;
> + unlock_kernel();
> break;
> default:
> /* NOTE: returning a fault code here could cause trouble
> @@ -487,7 +509,7 @@ static const struct file_operations i2cdev_fops = {
> .llseek = no_llseek,
> .read = i2cdev_read,
> .write = i2cdev_write,
> - .ioctl = i2cdev_ioctl,
> + .unlocked_ioctl = i2cdev_ioctl,
> .open = i2cdev_open,
> .release = i2cdev_release,
> };
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists