Mutex Hang on GC in lockd for 2.6.22-14

DISCUSSION Jonathan Couper 2 participants 4 messages Jan 21, 2008 5 messages not in the archive

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jonathan Couper

author

2008-01-21 05:39 UTC #3 lore raw

Heya NFS Experts,

I'm pretty darn sure I've found (and am currently testing a fix for) a
mutex bug in lockd that hangs NFS server pretty reliably. I have no idea how this bug has gone unnoticed for so long, since this is the current Gutsy Gibbon kernel!

That makes me think that maybe I'm mistaken. But my system definitely
hangs, and the code definitely appears to attempt to lock twice on a non-recursive mutex.

All the details are:

https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/181996

I'd LOVE to get my name into the Linux Kernel. Please tell me I've
found a real bug!

J.

Quoted 26 lines · Neil Brown

Quoted 8 lines · On Monday January 21, [email protected] wrote:
Heya,
I'm pretty darn sure I've found (and am currently testing a fix for) a
mutex bug in lockd that hangs NFS server pretty reliably. I have no idea how this bug has gone unnoticed for so long, since this is the current Gutsy Gibbon kernel!
That makes me think that maybe I'm mistaken. But my system definitely
hangs, and the code definitely appears to attempt to lock twice on a non-recursive mutex.
Quoted 8 lines
All the details are:
https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/181996
I'd LOVE to get my name into the Linux Kernel. Please tell me I've
found a real bug!
	J.
I'll probably have a look, but please post this to
[email protected]
other people have interest in, and knowledge of, lockd. Not just me.
Thanks,
NeilBrown

A message that is not in the archive (sent off-list or not yet received): [email protected]

J. Bruce Fields

2008-01-21 18:31 UTC #5 lore raw

Quoted 7 lines · Jonathan Couper

Heya NFS Experts,
I'm pretty darn sure I've found (and am currently testing a fix for) a
mutex bug in lockd that hangs NFS server pretty reliably. I have no idea how this bug has gone unnoticed for so long, since this is the current Gutsy Gibbon kernel!

Quoted 7 lines

That makes me think that maybe I'm mistaken. But my system definitely
hangs, and the code definitely appears to attempt to lock twice on a non-recursive mutex.
All the details are:
https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/181996

Hm; from:

https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/181996/comments/2

"...and on the server side I will now see TWO "[lockd]" processes where before I saw one."

That at least should be prevented by Jeff Layton's recent patches, which you can get from

git://linux-nfs.org/~bfields/linux.git nfs-server-stable

(Quick git tutorial; you can get that with

apt-get install git-core
git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
cd linux-2.6
git remote add bfields git://linux-nfs.org/~bfields/linux.git
git fetch bfields
git checkout bfields/nfs-server-stable

)

But, hm, I'm confused by your description in:

	https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/181996/comments/10

The double-lock you're identifying is from:

nlmsvc_lock() taking f_mutex, then calling
nlmsvc_create_block(), which calls
nlmsvc_lookup_host()

but nlmsvc_create_block() doesn't call nlmsvc_lookup_host(). Oh, I see--it used to in v2.6.22; the commit below fixed it. (And note if you apply that you should also apply a6d85430424d44e946e0946bfaad607115510989 "NLM: Fix a memory leak in nlmsvc_testlock"

Quoted 2 lines

I'd LOVE to get my name into the Linux Kernel. Please tell me I've
found a real bug!

Alas! So the secret is to test the most recent kernels, as those have the most unfound bugs....

--b.

commit 255129d1e9ca0ed3d69d5517fae3e03d7ab4b806

Author: Trond Myklebust <[email protected]>
Date:   Tue Sep 25 15:55:03 2007 -0400

NLM: Fix a circular lock dependency in lockd

The problem is that the garbage collector for the 'host' structures nlm_gc_hosts(), holds nlm_host_mutex while calling down to nlmsvc_mark_resources, which, eventually takes the file->f_mutex.

We cannot therefore call nlmsvc_lookup_host() from within nlmsvc_create_block, since the caller will already hold file->f_mutex, so the attempt to grab nlm_host_mutex may deadlock.

Fix the problem by calling nlmsvc_lookup_host() outside the file->f_mutex.

Signed-off-by Trond Myklebust <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

fs/lockd/svclock.c +18 ~~-11~~

index a21e4bc..d098c7a 100644
@@ -171,19 +171,14 @@ found:
  * GRANTED_RES message by cookie, without having to rely on the client's IP
  * address. --okir
  */
-static inline struct nlm_block *
-nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_file *file,
-		struct nlm_lock *lock, struct nlm_cookie *cookie)
+static struct nlm_block *
+nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
+		    struct nlm_file *file, struct nlm_lock *lock,
+		    struct nlm_cookie *cookie)
 {
 	struct nlm_block	*block;
-	struct nlm_host		*host;
 	struct nlm_rqst		*call = NULL;
 
-	/* Create host handle for callback */
-	host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
-	if (host == NULL)
-		return NULL;
-
 	call = nlm_alloc_call(host);
 	if (call == NULL)
 		return NULL;

@@ -366,6 +361,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 			struct nlm_lock *lock, int wait, struct nlm_cookie *cookie)
 {
 	struct nlm_block	*block = NULL;
+	struct nlm_host		*host;
 	int			error;
 	__be32			ret;

@@ -377,6 +373,10 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 				(long long)lock->fl.fl_end,
 				wait);
 
+	/* Create host handle for callback */
+	host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
+	if (host == NULL)
+		return nlm_lck_denied_nolocks;
 
 	/* Lock file against concurrent access */
 	mutex_lock(&file->f_mutex);

@@ -385,7 +385,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 	 */
 	block = nlmsvc_lookup_block(file, lock);
 	if (block == NULL) {
-		block = nlmsvc_create_block(rqstp, file, lock, cookie);
+		block = nlmsvc_create_block(rqstp, nlm_get_host(host), file,
+				lock, cookie);
 		ret = nlm_lck_denied_nolocks;
 		if (block == NULL)
 			goto out;

@@ -449,6 +450,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 out:
 	mutex_unlock(&file->f_mutex);
 	nlmsvc_release_block(block);
+	nlm_release_host(host);
 	dprintk("lockd: nlmsvc_lock returned %u\n", ret);
 	return ret;
 }

@@ -477,10 +479,15 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 
 	if (block == NULL) {
 		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
+		struct nlm_host	*host;
 
 		if (conf == NULL)
 			return nlm_granted;
-		block = nlmsvc_create_block(rqstp, file, lock, cookie);
+		/* Create host handle for callback */
+		host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
+		if (host == NULL)
+			return nlm_lck_denied_nolocks;
+		block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
 		if (block == NULL) {
 			kfree(conf);
 			return nlm_granted;

Jonathan Couper

author

2008-01-21 20:49 UTC #6 lore raw

Heya,

What's the path for that getting rolled back into the 2.6.22 chain? In
generally I'm happy with my 2.6.22 kernel. Is Jeff submitting those
fixes to the 2.6.22 release at some stage, do you know?

J.

Quoted 167 lines · J. Bruce Fields

Quoted 6 lines · Jonathan Couper
Heya NFS Experts,
I'm pretty darn sure I've found (and am currently testing a fix for) a
mutex bug in lockd that hangs NFS server pretty reliably. I have no idea how this bug has gone unnoticed for so long, since this is the current Gutsy Gibbon kernel!
Quoted 6 lines
That makes me think that maybe I'm mistaken. But my system definitely
hangs, and the code definitely appears to attempt to lock twice on a non-recursive mutex.
All the details are:
https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/181996
Hm; from:
https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/181996/comments/2
"...and on the server side I will now see TWO "[lockd]" processes where before I saw one."
That at least should be prevented by Jeff Layton's recent patches, which you can get from
git://linux-nfs.org/~bfields/linux.git nfs-server-stable
(Quick git tutorial; you can get that with
apt-get install git-core
git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
cd linux-2.6
git remote add bfields git://linux-nfs.org/~bfields/linux.git
git fetch bfields
git checkout bfields/nfs-server-stable
)
But, hm, I'm confused by your description in:
	https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/181996/comments/10
The double-lock you're identifying is from:
nlmsvc_lock() taking f_mutex, then calling
nlmsvc_create_block(), which calls
nlmsvc_lookup_host()
but nlmsvc_create_block() doesn't call nlmsvc_lookup_host(). Oh, I see--it used to in v2.6.22; the commit below fixed it. (And note if you apply that you should also apply a6d85430424d44e946e0946bfaad607115510989 "NLM: Fix a memory leak in nlmsvc_testlock"
Quoted 2 lines
I'd LOVE to get my name into the Linux Kernel. Please tell me I've
found a real bug!
Alas! So the secret is to test the most recent kernels, as those have the most unfound bugs....
--b.
commit 255129d1e9ca0ed3d69d5517fae3e03d7ab4b806
Author: Trond Myklebust <[email protected]>
Date:   Tue Sep 25 15:55:03 2007 -0400
NLM: Fix a circular lock dependency in lockd
The problem is that the garbage collector for the 'host' structures nlm_gc_hosts(), holds nlm_host_mutex while calling down to nlmsvc_mark_resources, which, eventually takes the file->f_mutex.
We cannot therefore call nlmsvc_lookup_host() from within nlmsvc_create_block, since the caller will already hold file->f_mutex, so the attempt to grab nlm_host_mutex may deadlock.
Fix the problem by calling nlmsvc_lookup_host() outside the file->f_mutex.
Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
fs/lockd/svclock.c +18 ~~-12~~
index a21e4bc..d098c7a 100644
@@ -171,19 +171,14 @@ found:
  * GRANTED_RES message by cookie, without having to rely on the client's IP
  * address. --okir
  */
-static inline struct nlm_block *
-nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_file *file,
-		struct nlm_lock *lock, struct nlm_cookie *cookie)
+static struct nlm_block *
+nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
+		    struct nlm_file *file, struct nlm_lock *lock,
+		    struct nlm_cookie *cookie)
 {
 	struct nlm_block	*block;
-	struct nlm_host		*host;
 	struct nlm_rqst		*call = NULL;
 
-	/* Create host handle for callback */
-	host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
-	if (host == NULL)
-		return NULL;
-
 	call = nlm_alloc_call(host);
 	if (call == NULL)
 		return NULL;
@@ -366,6 +361,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 			struct nlm_lock *lock, int wait, struct nlm_cookie *cookie)
 {
 	struct nlm_block	*block = NULL;
+	struct nlm_host		*host;
 	int			error;
 	__be32			ret;
 
@@ -377,6 +373,10 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 				(long long)lock->fl.fl_end,
 				wait);
 
+	/* Create host handle for callback */
+	host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
+	if (host == NULL)
+		return nlm_lck_denied_nolocks;
 
 	/* Lock file against concurrent access */
 	mutex_lock(&file->f_mutex);
@@ -385,7 +385,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 	 */
 	block = nlmsvc_lookup_block(file, lock);
 	if (block == NULL) {
-		block = nlmsvc_create_block(rqstp, file, lock, cookie);
+		block = nlmsvc_create_block(rqstp, nlm_get_host(host), file,
+				lock, cookie);
 		ret = nlm_lck_denied_nolocks;
 		if (block == NULL)
 			goto out;
@@ -449,6 +450,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 out:
 	mutex_unlock(&file->f_mutex);
 	nlmsvc_release_block(block);
+	nlm_release_host(host);
 	dprintk("lockd: nlmsvc_lock returned %u\n", ret);
 	return ret;
 }
@@ -477,10 +479,15 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 
 	if (block == NULL) {
 		struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
+		struct nlm_host	*host;
 
 		if (conf == NULL)
 			return nlm_granted;
-		block = nlmsvc_create_block(rqstp, file, lock, cookie);
+		/* Create host handle for callback */
+		host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
+		if (host == NULL)
+			return nlm_lck_denied_nolocks;
+		block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
 		if (block == NULL) {
 			kfree(conf);
 			return nlm_granted;
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

J. Bruce Fields

2008-01-21 21:03 UTC #8 lore raw

Quoted 5 lines · Jonathan Couper

Heya,
What's the path for that getting rolled back into the 2.6.22 chain? In
generally I'm happy with my 2.6.22 kernel. Is Jeff submitting those
fixes to the 2.6.22 release at some stage, do you know?

There appear to be two separate problems here with two fixes:

- the possibility of multiple lockd's at once, fixed by Jeff's patches.
- the double-lock problem, fixed by Trond's two patches.

It sounds like the latter may actually be what you're more in need of? They're probably easier to backport in any case.

I don't know if those are in 2.6.22, or if 2.6.22 is still open. I'd add pointers to those two patches to the bugzilla report for the ubuntu people to look at.

--b.