Skip to contentSkip to conversation

configfs: add missing mutex_unlock()

PATCH In review Akinobu Mita 3 participants 4 messages Mar 4, 2007 – Mar 5, 2007 configfs

lore
Share & tools
lore

Akinobu Mita

author
#0 lore raw

PATCH 1/1 configfs: add missing mutex_unlock()

From: Akinobu Mita <[email protected]>
Subject: [PATCH] configfs: add missing mutex_unlock()

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

Signed-off-by Akinobu Mita <[email protected]>
Cc Joel Becker <[email protected]>
---
 fs/configfs/dir.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/configfs/dir.c +3 -1
Index: 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);
#1 lore raw
Quoted 11 lines · On Sunday 04 March 2007 14:38:12 Akinobu Mita wrote:
(quoted diff) +0 -0
@@ -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 <><
author
#2 lore raw
Quoted 16 lines · Arnd Bergmann
Quoted 11 lines · On Sunday 04 March 2007 14:38:12 Akinobu Mita wrote:
(quoted diff) +0 -0
@@ -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).

fs/configfs/dir.c +12 -11
Index: 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;
 }
#3 lore raw

First, thank you Akinobu for discovering the bug.

Quoted 6 lines · Akinobu Mita
Quoted 3 lines · Arnd Bergmann

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

fs/configfs/dir.c +12 -15
index 1814ba4..489c265 100644
@@ -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;
Signature · 10 lines
"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: [email protected]
Phone: (650) 506-8127