linux-kernel - Re: [patch 23/28] i2c: Remove big kernel lock from i2cdev

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 10 Oct 2009 19:04:36 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Vincent Sanders <vince@...tec.co.uk>,
	John Kacur <jkacur@...hat.com>,
	Jonathan Corbet <corbet@....net>,
	Christoph Hellwig <hch@...radead.org>
Subject: Re: [patch 23/28] i2c: Remove big kernel lock from i2cdev_open

Hi Thomas,

On Sat, 10 Oct 2009 15:37:17 -0000, Thomas Gleixner wrote:
> The BKL is held over a kmalloc so cannot protect anything beyond that.
> The two calls before the kmalloc have their own locking.
> Improve device open function by removing the now unnecessary ret variable
> 
> Signed-off-by: Vincent Sanders <vince@...tec.co.uk>
> LKML-Reference: <1255175172-2666-1-git-send-email-vince@...tec.co.uk>
> Cc: Jean Delvare <khali@...ux-fr.org>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  drivers/i2c/i2c-dev.c |   22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> Index: linux-2.6-tip/drivers/i2c/i2c-dev.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/i2c/i2c-dev.c
> +++ linux-2.6-tip/drivers/i2c/i2c-dev.c
> @@ -34,7 +34,6 @@
>  #include <linux/list.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-dev.h>
> -#include <linux/smp_lock.h>
>  #include <linux/jiffies.h>
>  #include <asm/uaccess.h>
>  
> @@ -445,20 +444,14 @@ static int i2cdev_open(struct inode *ino
>  	struct i2c_client *client;
>  	struct i2c_adapter *adap;
>  	struct i2c_dev *i2c_dev;
> -	int ret = 0;
>  
> -	lock_kernel();
>  	i2c_dev = i2c_dev_get_by_minor(minor);
> -	if (!i2c_dev) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> +	if (!i2c_dev)
> +		return -ENODEV;
>  
>  	adap = i2c_get_adapter(i2c_dev->adap->nr);
> -	if (!adap) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> +	if (!adap)
> +		return -ENODEV;
>  
>  	/* This creates an anonymous i2c_client, which may later be
>  	 * pointed to some address using I2C_SLAVE or I2C_SLAVE_FORCE.
> @@ -470,8 +463,7 @@ static int i2cdev_open(struct inode *ino
>  	client = kzalloc(sizeof(*client), GFP_KERNEL);
>  	if (!client) {
>  		i2c_put_adapter(adap);
> -		ret = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
>  	snprintf(client->name, I2C_NAME_SIZE, "i2c-dev %d", adap->nr);
>  	client->driver = &i2cdev_driver;
> @@ -479,9 +471,7 @@ static int i2cdev_open(struct inode *ino
>  	client->adapter = adap;
>  	file->private_data = client;
>  
> -out:
> -	unlock_kernel();
> -	return ret;
> +	return 0;
>  }
>  
>  static int i2cdev_release(struct inode *inode, struct file *file)
> 

Looks good to me:

Acked-by: Jean Delvare <khali@...ux-fr.org>

Do you want me to pick this patch in my i2c tree, or will you take care
of pushing it upstream? If you're not going to push it quickly, I'd
prefer the first solution, to avoid conflicts.

(As a side note, i2c-dev would deserve a partial rewrite as it doesn't
integrate into the standard device driver model, and certainly has a
number of race conditions. But this is well beyond the scope of your
patch.)

Thanks,
-- 
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/