Skip to contentSkip to conversation

eCryptFS: fix missed mutex_unlock

PATCH In review Cyrill Gorcunov 3 participants 5 messages May 18, 2008 – May 21, 2008 Ecrypt file system

lore
Share & tools
lore

Cyrill Gorcunov

author
#0 lore raw

PATCH 1/1 eCryptFS: fix missed mutex_unlock

---

Ingo, could you please apply it and test? Actually I really doubt if it help with the locking problem you pointed. There are two procedures in miscrev.c - ecryptfs_miscdev_poll() and ecryptfs_miscdev_read() which takes/releases mutexes in a bit strange way... investigating, but this patch is needed anyway.

fs/ecryptfs/crypto.c +1 -0
Index: linux-2.6.git/fs/ecryptfs/crypto.c
===================================================================
@@ -1903,6 +1903,7 @@ int ecryptfs_get_tfm_and_mutex_for_ciphe
 		if (rc) {
 			printk(KERN_ERR "Error adding new key_tfm to list; "
 					"rc = [%d]\n", rc);
+			mutex_unlock(&key_tfm_list_mutex);
 			goto out;
 		}
 	}
#1 lore raw
Quoted 13 lines · On Sun, 18 May 2008 18:26:11 +0400 Cyrill Gorcunov <[email protected]> wrote:

---

Ingo, could you please apply it and test? Actually I really doubt if it help with the locking problem you pointed. There are two procedures in miscrev.c - ecryptfs_miscdev_poll() and ecryptfs_miscdev_read() which takes/releases mutexes in a bit strange way... investigating, but this patch is needed anyway.

Index: linux-2.6.git/fs/ecryptfs/crypto.c
===================================================================

fs/ecryptfs/crypto.c int ecryptfs_get_tfm_and_mutex_for_ciphe · line 1909

Quoted 7 lines
fs/ecryptfs/crypto.c +1 -0
@@ -1903,6 +1903,7 @@ int ecryptfs_get_tfm_and_mutex_for_ciphe
 		if (rc) {
 			printk(KERN_ERR "Error adding new key_tfm to list; "
 					"rc = [%d]\n", rc);
+			mutex_unlock(&key_tfm_list_mutex);
 			goto out;
 		}
 	}

Better to do it this way, I think:

fs/ecryptfs/crypto.c renamed from fs/ecryptfs/crypto.c~ecryptfs-fix-missed-mutex_unlock +1 -1
@@ -1906,9 +1906,9 @@ int ecryptfs_get_tfm_and_mutex_for_ciphe
 			goto out;
 		}
 	}
-	mutex_unlock(&key_tfm_list_mutex);
 	(*tfm) = key_tfm->key_tfm;
 	(*tfm_mutex) = &key_tfm->key_tfm_mutex;
 out:
+	mutex_unlock(&key_tfm_list_mutex);
 	return rc;
 }

_

Holding the lock for an additional few instructions may not be strictly needed, but we might avoid the reintroduction of such bugs?

author
#2 lore raw
Quoted 40 lines · Andrew Morton
Quoted 13 lines · On Sun, 18 May 2008 18:26:11 +0400 Cyrill Gorcunov <[email protected]> wrote:

---

Ingo, could you please apply it and test? Actually I really doubt if it help with the locking problem you pointed. There are two procedures in miscrev.c - ecryptfs_miscdev_poll() and ecryptfs_miscdev_read() which takes/releases mutexes in a bit strange way... investigating, but this patch is needed anyway.

Index: linux-2.6.git/fs/ecryptfs/crypto.c
===================================================================

fs/ecryptfs/crypto.c 2008-05-18 17:56:12.000000000 +0400 int ecryptfs_get_tfm_and_mutex_for_ciphe · line 1909

Quoted 7 lines
fs/ecryptfs/crypto.c 2008-05-18 17:56:12.000000000 +0400 renamed from fs/ecryptfs/crypto.c 2008-05-18 16:44:20.000000000 +0400 +1 -0
@@ -1903,6 +1903,7 @@ int ecryptfs_get_tfm_and_mutex_for_ciphe
              if (rc) {
                      printk(KERN_ERR "Error adding new key_tfm to list; "
                                      "rc = [%d]\n", rc);
+                     mutex_unlock(&key_tfm_list_mutex);
                      goto out;
              }
      }

Better to do it this way, I think:

fs/ecryptfs/crypto.c renamed from fs/ecryptfs/crypto.c~ecryptfs-fix-missed-mutex_unlock +1 -1
@@ -1906,9 +1906,9 @@ int ecryptfs_get_tfm_and_mutex_for_ciphe
                       goto out;
               }
       }
-       mutex_unlock(&key_tfm_list_mutex);
       (*tfm) = key_tfm->key_tfm;
       (*tfm_mutex) = &key_tfm->key_tfm_mutex;
 out:
+       mutex_unlock(&key_tfm_list_mutex);
       return rc;
 }

_

Quoted 4 lines

Holding the lock for an additional few instructions may not be strictly needed, but we might avoid the reintroduction of such bugs?

Good idea, thanks! Could you update the patch, please?

#3 lore raw
In reply to Cyrill Gorcunov
Quoted 5 lines · Cyrill Gorcunov

Ingo, could you please apply it and test? Actually I really doubt if it help with the locking problem you pointed. There are two procedures in miscrev.c - ecryptfs_miscdev_poll() and ecryptfs_miscdev_read() which takes/releases mutexes in a bit strange way... investigating, but this patch is needed anyway.

btw., i didnt do any specific ecryptfs testing - randconfig enabled it and it got booted. So if you dont get the warning during bootup/module-load, you'll have the same test coverage i did.

Ingo

author
#4 lore raw
Quoted 9 lines · Ingo Molnar
Quoted 5 lines · Cyrill Gorcunov

Ingo, could you please apply it and test? Actually I really doubt if it help with the locking problem you pointed. There are two procedures in miscrev.c - ecryptfs_miscdev_poll() and ecryptfs_miscdev_read() which takes/releases mutexes in a bit strange way... investigating, but this patch is needed anyway.

Quoted 6 lines

btw., i didnt do any specific ecryptfs testing - randconfig enabled it and it got booted. So if you dont get the warning during bootup/module-load, you'll have the same test coverage i did.

Ingo

oh, thanks ;)