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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> @@ -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 <[email protected]>
Thanks.
--
tejun
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks!
--
tejun
Tejun Heo <[email protected]> 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 <[email protected]>
>
> Acked-by: Tejun Heo <[email protected]>
Thanks, David.
Queued both patches for next tree.
Miklos