[PATCH] configfs: add missing mutex_unlock()

[PATCH] configfs: add missing mutex_unlock()

[Akinobu Mita]

From: Akinobu Mita <akinobu.mita@gmail.com>
Subject: [PATCH] configfs: add missing mutex_unlock()

Add missing mutex_unlock() on d_alloc() failure in
configfs_register_subsystem().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Joel Becker <joel.becker@oracle.com>
---
 fs/configfs/dir.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: 2.6-mm/fs/configfs/dir.c
===================================================================
--- 2.6-mm.orig/fs/configfs/dir.c
+++ 2.6-mm/fs/configfs/dir.c
@@ -1168,8 +1168,10 @@ int configfs_register_subsystem(struct c
 
 	err = -ENOMEM;
 	dentry = d_alloc(configfs_sb->s_root, &name);
-	if (!dentry)
+	if (!dentry) {
+		mutex_unlock(&configfs_sb->s_root->d_inode->i_mutex);
 		goto out_release;
+	}
 
 	d_add(dentry, NULL);
 

Re: [PATCH] configfs: add missing mutex_unlock()

[Arnd Bergmann]

On Sunday 04 March 2007 14:38:12 Akinobu Mita wrote:
> @@ -1168,8 +1168,10 @@ int configfs_register_subsystem(struct c
>  
>         err = -ENOMEM;
>         dentry = d_alloc(configfs_sb->s_root, &name);
> -       if (!dentry)
> +       if (!dentry) {
> +               mutex_unlock(&configfs_sb->s_root->d_inode->i_mutex);
>                 goto out_release;
> +       }
>  
>         d_add(dentry, NULL);

This should be changed to jump to a new exit point, before the mutex_unlock
at the end of the function. Having multiple places in the function that
release the same lock easily leads to the kind of bug you are fixing here.

	Arnd <><

Re: [PATCH] configfs: add missing mutex_unlock()

[Akinobu Mita]

On Sun, Mar 04, 2007 at 03:10:19PM +0100, Arnd Bergmann wrote:
> On Sunday 04 March 2007 14:38:12 Akinobu Mita wrote:
> > @@ -1168,8 +1168,10 @@ int configfs_register_subsystem(struct c
> >  
> >         err = -ENOMEM;
> >         dentry = d_alloc(configfs_sb->s_root, &name);
> > -       if (!dentry)
> > +       if (!dentry) {
> > +               mutex_unlock(&configfs_sb->s_root->d_inode->i_mutex);
> >                 goto out_release;
> > +       }
> >  
> >         d_add(dentry, NULL);
> 
> This should be changed to jump to a new exit point, before the mutex_unlock
> at the end of the function. Having multiple places in the function that
> release the same lock easily leads to the kind of bug you are fixing here.

Agreed. Please see the patch below (untested).

Index: 2.6-mm/fs/configfs/dir.c
===================================================================
--- 2.6-mm.orig/fs/configfs/dir.c
+++ 2.6-mm/fs/configfs/dir.c
@@ -1169,25 +1169,26 @@ int configfs_register_subsystem(struct c
 	err = -ENOMEM;
 	dentry = d_alloc(configfs_sb->s_root, &name);
 	if (!dentry)
-		goto out_release;
+		goto out_unlock;
 
 	d_add(dentry, NULL);
 
 	err = configfs_attach_group(sd->s_element, &group->cg_item,
 				    dentry);
-	if (!err)
-		dentry = NULL;
-	else
-		d_delete(dentry);
+	if (err)
+		goto out_d_delete;
 
 	mutex_unlock(&configfs_sb->s_root->d_inode->i_mutex);
 
-	if (dentry) {
-	    dput(dentry);
-out_release:
-	    unlink_group(group);
-	    configfs_release_fs();
-	}
+	return 0;
+
+out_d_delete:
+	d_delete(dentry);
+	dput(dentry);
+out_unlock:
+	mutex_unlock(&configfs_sb->s_root->d_inode->i_mutex);
+	unlink_group(group);
+	configfs_release_fs();
 
 	return err;
 }

Re: [PATCH] configfs: add missing mutex_unlock()

[Joel Becker]

	First, thank you Akinobu for discovering the bug.

On Mon, Mar 05, 2007 at 12:16:59AM +0900, Akinobu Mita wrote:
> On Sun, Mar 04, 2007 at 03:10:19PM +0100, Arnd Bergmann wrote:
> > This should be changed to jump to a new exit point, before the mutex_unlock
> > at the end of the function. Having multiple places in the function that
> > release the same lock easily leads to the kind of bug you are fixing here.
> 
> Agreed. Please see the patch below (untested).

	Jumping isn't always the solution.  The new patch, while better
than the first, still calls mutex_unlock() in two places.  The following
patch keeps it all in one code path, and keeps all dentry modification
underneath a single mutex_lock()/unlock() pair.  What do you think?  I'm
building it to test as we speak.

Joel

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 1814ba4..489c265 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1142,25 +1142,22 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
 
 	err = -ENOMEM;
 	dentry = d_alloc(configfs_sb->s_root, &name);
-	if (!dentry)
-		goto out_release;
-
-	d_add(dentry, NULL);
+	if (dentry) {
+		d_add(dentry, NULL);
 
-	err = configfs_attach_group(sd->s_element, &group->cg_item,
-				    dentry);
-	if (!err)
-		dentry = NULL;
-	else
-		d_delete(dentry);
+		err = configfs_attach_group(sd->s_element, &group->cg_item,
+					    dentry);
+		if (err) {
+			d_delete(dentry);
+			dput(dentry);
+		}
+	}
 
 	mutex_unlock(&configfs_sb->s_root->d_inode->i_mutex);
 
-	if (dentry) {
-	    dput(dentry);
-out_release:
-	    unlink_group(group);
-	    configfs_release_fs();
+	if (err) {
+		unlink_group(group);
+		configfs_release_fs();
 	}
 
 	return err;

-- 

"Nobody loves me,
 Nobody seems to care.
 Troubles and worries, people,
 You know I've had my share."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127