Re: [PATCH] fix recursive nlm_file_mutex deadlock

[Trond Myklebust <trond.myklebust@fys.uio.no>]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On Thu, 2006-08-10 at 11:40 -0400, Trond Myklebust wrote:
> On Thu, 2006-08-10 at 11:24 -0400, Wendy Cheng wrote:
> 
> > We simply can't call nlm_release_file() as long as it requires
> > nlm_file_mutex (no matter where you move the locking line) within
> > nlm_traverse_files() code path. I'm ok with atomic variable though.
> 
> As long as we can guarantee that file->f_count doesn't drop to zero (and
> we can make that guarantee in nlm_traverse_file()), then the code is
> correct.

Doh! There is no reason whatsoever to be holding nlm_file_mutex across
the call to nlm_inspect_file. As you said, the only thing it does is
protect the list, and we don't care about that.

The simplest fix is just to do something like the following.

Cheers,
  Trond

[-- Attachment #2: linux-2.6.18-009-fix_nlm_traverse_files_deadlock.dif --]
[-- Type: message/rfc822, Size: 1653 bytes --]

From: Trond Myklebust <Trond.Myklebust@netapp.com>
Subject: No Subject
Date: Thu, 10 Aug 2006 12:01:51 -0400
Message-ID: <1155225711.10547.49.camel@localhost>

nlm_traverse_files() is not allowed to hold the nlm_file_mutex while calling
nlm_inspect file, since it may end up calling nlm_release_file() when
releaseing the blocks.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/lockd/svcsubs.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 2a4df9b..01b4db9 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -237,19 +237,22 @@ static int
 nlm_traverse_files(struct nlm_host *host, int action)
 {
 	struct nlm_file	*file, **fp;
-	int		i;
+	int i, ret = 0;
 
 	mutex_lock(&nlm_file_mutex);
 	for (i = 0; i < FILE_NRHASH; i++) {
 		fp = nlm_files + i;
 		while ((file = *fp) != NULL) {
+			file->f_count++;
+			mutex_unlock(&nlm_file_mutex);
+
 			/* Traverse locks, blocks and shares of this file
 			 * and update file->f_locks count */
-			if (nlm_inspect_file(host, file, action)) {
-				mutex_unlock(&nlm_file_mutex);
-				return 1;
-			}
+			if (nlm_inspect_file(host, file, action))
+				ret = 1;
 
+			mutex_lock(&nlm_file_mutex);
+			file->f_count--;
 			/* No more references to this file. Let go of it. */
 			if (!file->f_blocks && !file->f_locks
 			 && !file->f_shares && !file->f_count) {
@@ -262,7 +265,7 @@ nlm_traverse_files(struct nlm_host *host
 		}
 	}
 	mutex_unlock(&nlm_file_mutex);
-	return 0;
+	return ret;
 }
 
 /*

[-- Attachment #3: Type: text/plain, Size: 373 bytes --]

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs