[PATCH v3 1/2] cuse: use mutex as registration lock instead of spinlocks

[PATCH v3 1/2] cuse: use mutex as registration lock instead of spinlocks

[David Herrmann]

We need to check for name-collisions during cuse-device registration. To
avoid race-conditions, this needs to be protected during the whole device
registration. Therefore, replace the spinlocks by mutexes first so we can
safely extend the locked regions to include more expensive or sleeping
code paths.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
Hi again

Sorry for the noise, but I think this time everything should work. I fixed the
name-check to check for all registered devices instead only one list.
And I split it into two patches.

Thanks for the reviews!
David

 fs/fuse/cuse.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
index ee8d550..e830dab 100644
--- a/fs/fuse/cuse.c
+++ b/fs/fuse/cuse.c
@@ -45,7 +45,6 @@
 #include <linux/miscdevice.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
-#include <linux/spinlock.h>
 #include <linux/stat.h>
 #include <linux/module.h>
 
@@ -63,7 +62,7 @@ struct cuse_conn {
 	bool			unrestricted_ioctl;
 };
 
-static DEFINE_SPINLOCK(cuse_lock);		/* protects cuse_conntbl */
+static DEFINE_MUTEX(cuse_lock);			/* protects registration */
 static struct list_head cuse_conntbl[CUSE_CONNTBL_LEN];
 static struct class *cuse_class;
 
@@ -114,14 +113,18 @@ static int cuse_open(struct inode *inode, struct file *file)
 	int rc;
 
 	/* look up and get the connection */
-	spin_lock(&cuse_lock);
+	rc = mutex_lock_interruptible(&cuse_lock);
+	if (rc)
+		return rc;
+
 	list_for_each_entry(pos, cuse_conntbl_head(devt), list)
 		if (pos->dev->devt == devt) {
 			fuse_conn_get(&pos->fc);
 			cc = pos;
 			break;
 		}
-	spin_unlock(&cuse_lock);
+
+	mutex_unlock(&cuse_lock);
 
 	/* dead? */
 	if (!cc)
@@ -377,9 +380,9 @@ static void cuse_process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 	cc->cdev = cdev;
 
 	/* make the device available */
-	spin_lock(&cuse_lock);
+	mutex_lock(&cuse_lock);
 	list_add(&cc->list, cuse_conntbl_head(devt));
-	spin_unlock(&cuse_lock);
+	mutex_unlock(&cuse_lock);
 
 	/* announce device availability */
 	dev_set_uevent_suppress(dev, 0);
@@ -520,9 +523,9 @@ static int cuse_channel_release(struct inode *inode, struct file *file)
 	int rc;
 
 	/* remove from the conntbl, no more access from this point on */
-	spin_lock(&cuse_lock);
+	mutex_lock(&cuse_lock);
 	list_del_init(&cc->list);
-	spin_unlock(&cuse_lock);
+	mutex_unlock(&cuse_lock);
 
 	/* remove device */
 	if (cc->dev)
-- 
1.8.0

[PATCH v3 2/2] cuse: do not register multiple devices with identical names

[David Herrmann]

Sysfs doesn't allow two devices with the same name, but we register a
sysfs entry for each cuse device without checking for name collisions.
This extends the registration to first check whether the name was already
registered.

To avoid race-conditions between the name-check and linking the device, we
need to protect the whole registration with a mutex.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
Hi

Changes in v2 include:
 - move mutex-conversion into a separate patch
 - check every list in the cuse_conntbl array, not only the current one

 fs/fuse/cuse.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
index e830dab..c56c84c 100644
--- a/fs/fuse/cuse.c
+++ b/fs/fuse/cuse.c
@@ -308,14 +308,14 @@ static void cuse_gendev_release(struct device *dev)
  */
 static void cuse_process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 {
-	struct cuse_conn *cc = fc_to_cc(fc);
+	struct cuse_conn *cc = fc_to_cc(fc), *pos;
 	struct cuse_init_out *arg = req->out.args[0].value;
 	struct page *page = req->pages[0];
 	struct cuse_devinfo devinfo = { };
 	struct device *dev;
 	struct cdev *cdev;
 	dev_t devt;
-	int rc;
+	int rc, i;
 
 	if (req->out.h.error ||
 	    arg->major != FUSE_KERNEL_VERSION || arg->minor < 11) {
@@ -359,15 +359,24 @@ static void cuse_process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 	dev_set_drvdata(dev, cc);
 	dev_set_name(dev, "%s", devinfo.name);
 
+	mutex_lock(&cuse_lock);
+
+	/* make sure the device-name is unique */
+	for (i = 0; i < CUSE_CONNTBL_LEN; ++i) {
+		list_for_each_entry(pos, &cuse_conntbl[i], list)
+			if (!strcmp(dev_name(pos->dev), dev_name(dev)))
+				goto err_unlock;
+	}
+
 	rc = device_add(dev);
 	if (rc)
-		goto err_device;
+		goto err_unlock;
 
 	/* register cdev */
 	rc = -ENOMEM;
 	cdev = cdev_alloc();
 	if (!cdev)
-		goto err_device;
+		goto err_unlock;
 
 	cdev->owner = THIS_MODULE;
 	cdev->ops = &cuse_frontend_fops;
@@ -380,7 +389,6 @@ static void cuse_process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 	cc->cdev = cdev;
 
 	/* make the device available */
-	mutex_lock(&cuse_lock);
 	list_add(&cc->list, cuse_conntbl_head(devt));
 	mutex_unlock(&cuse_lock);
 
@@ -394,7 +402,8 @@ out:
 
 err_cdev:
 	cdev_del(cdev);
-err_device:
+err_unlock:
+	mutex_unlock(&cuse_lock);
 	put_device(dev);
 err_region:
 	unregister_chrdev_region(devt, 1);
-- 
1.8.0

Re: [PATCH v3 2/2] cuse: do not register multiple devices with identical names

[Tejun Heo]

On Sat, Nov 17, 2012 at 12:45:48PM +0100, David Herrmann wrote:
> Sysfs doesn't allow two devices with the same name, but we register a
> sysfs entry for each cuse device without checking for name collisions.
> This extends the registration to first check whether the name was already
> registered.
> 
> To avoid race-conditions between the name-check and linking the device, we
> need to protect the whole registration with a mutex.
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks!

-- 
tejun

Re: [PATCH v3 2/2] cuse: do not register multiple devices with identical names

[Miklos Szeredi]

Tejun Heo <tj@kernel.org> writes:

> On Sat, Nov 17, 2012 at 12:45:48PM +0100, David Herrmann wrote:
>> Sysfs doesn't allow two devices with the same name, but we register a
>> sysfs entry for each cuse device without checking for name collisions.
>> This extends the registration to first check whether the name was already
>> registered.
>> 
>> To avoid race-conditions between the name-check and linking the device, we
>> need to protect the whole registration with a mutex.
>> 
>> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
>
> Acked-by: Tejun Heo <tj@kernel.org>

Thanks, David.

Queued both patches for next tree.

Miklos

Re: [PATCH v3 1/2] cuse: use mutex as registration lock instead of spinlocks

[Tejun Heo]

Hello, David.

On Sat, Nov 17, 2012 at 12:45:47PM +0100, David Herrmann wrote:
> We need to check for name-collisions during cuse-device registration. To
> avoid race-conditions, this needs to be protected during the whole device
> registration. Therefore, replace the spinlocks by mutexes first so we can
> safely extend the locked regions to include more expensive or sleeping
> code paths.
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> @@ -114,14 +113,18 @@ static int cuse_open(struct inode *inode, struct file *file)
>  	int rc;
>  
>  	/* look up and get the connection */
> -	spin_lock(&cuse_lock);
> +	rc = mutex_lock_interruptible(&cuse_lock);

Well, the above can't hurt but it doesn't help anything either given
the narrow scope of the lock.  Eh well, I guess it's okay either way.

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun