autofs4: fix race in unhashed dentry code

PATCH In review Jeff Mahoney 2 participants 6 messages Apr 11, 2007

Share & tools

Jeff Mahoney

author

2007-04-11 15:05 UTC #0 lore raw

PATCH 1/1 autofs4: fix race in unhashed dentry code

Commit f50b6f8691cae2e0064c499dd3ef3f31142987f0 introduced a race in autofs4 between autofs_lookup_unhashed() and autofs_dentry_release().

autofs_dentry_release() ends up clearing the ->dentry and ->inode members of autofs_info before removing it from the rehash list. The list is protected by the rehash lock in both functions, but since autofs_dentry_release() starts tearing the autofs_info struct down before removing it from the list, autofs_lookup_unhashed() can get a autofs_info with a NULL dentry.

This patch moves the clearing of ->dentry and ->inode after the removal from the rehash list.

Signed-off-by Jeff Mahoney <[email protected]>

---

 fs/autofs4/root.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/autofs4/root.c +3 -3

@@ -470,9 +470,6 @@ void autofs4_dentry_release(struct dentr
 	if (inf) {
 		struct autofs_sb_info *sbi = autofs4_sbi(de->d_sb);
 
-		inf->dentry = NULL;
-		inf->inode = NULL;
-
 		if (sbi) {
 			spin_lock(&sbi->rehash_lock);
 			if (!list_empty(&inf->rehash))

@@ -480,6 +477,9 @@ void autofs4_dentry_release(struct dentr
 			spin_unlock(&sbi->rehash_lock);
 		}
 
+		inf->dentry = NULL;
+		inf->inode = NULL;
+
 		autofs4_free_ino(inf);
 	}
 }

Signature · 2 lines

Jeff Mahoney
SUSE Labs

Ian Kent

Acked-by on v1 1/1

2007-04-11 17:51 UTC #1 lore raw

Quoted 11 lines · Jeff Mahoney

Commit f50b6f8691cae2e0064c499dd3ef3f31142987f0 introduced a race in autofs4 between autofs_lookup_unhashed() and autofs_dentry_release().
autofs_dentry_release() ends up clearing the ->dentry and ->inode members of autofs_info before removing it from the rehash list. The list is protected by the rehash lock in both functions, but since autofs_dentry_release() starts tearing the autofs_info struct down before removing it from the list, autofs_lookup_unhashed() can get a autofs_info with a NULL dentry.

Quoted 2 lines

This patch moves the clearing of ->dentry and ->inode after the removal from the rehash list.

Oh .. excellent, I had a bug report but I just couldn't see it for looking.

You've made my day.
Thanks heaps
Ian

Quoted 2 lines

Signed-off-by Jeff Mahoney <[email protected]>

Acked-by Ian Kent <[email protected]>

Quoted 29 lines

---

 fs/autofs4/root.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/autofs4/root.c +3 -3

@@ -470,9 +470,6 @@ void autofs4_dentry_release(struct dentr
 	if (inf) {
 		struct autofs_sb_info *sbi = autofs4_sbi(de->d_sb);
 
-		inf->dentry = NULL;
-		inf->inode = NULL;
-
 		if (sbi) {
 			spin_lock(&sbi->rehash_lock);
 			if (!list_empty(&inf->rehash))

@@ -480,6 +477,9 @@ void autofs4_dentry_release(struct dentr
 			spin_unlock(&sbi->rehash_lock);
 		}
 
+		inf->dentry = NULL;
+		inf->inode = NULL;
+
 		autofs4_free_ino(inf);
 	}
 }

Ian Kent

2007-04-11 18:02 UTC #2 lore raw

Quoted 12 lines · Ian Kent

Quoted 11 lines · Jeff Mahoney
Commit f50b6f8691cae2e0064c499dd3ef3f31142987f0 introduced a race in autofs4 between autofs_lookup_unhashed() and autofs_dentry_release().
autofs_dentry_release() ends up clearing the ->dentry and ->inode members of autofs_info before removing it from the rehash list. The list is protected by the rehash lock in both functions, but since autofs_dentry_release() starts tearing the autofs_info struct down before removing it from the list, autofs_lookup_unhashed() can get a autofs_info with a NULL dentry.

Quoted 5 lines

Quoted 2 lines
This patch moves the clearing of ->dentry and ->inode after the removal from the rehash list.
Oh .. excellent, I had a bug report but I just couldn't see it for looking.

Maybe I've been a bit hasty with the celebration.
It looks like I've got a bigger locking problem here.
If autofs4_dentry_release waits on the rehash lock and autofs4_lookup_unhashed reclaims it then the info struct and the dentry go away unconditionally as the release is called just prior to freeing the dentry memory, right?.

But you've certainly identified the problem with it. Let me think about it a little while.

Quoted 38 lines

You've made my day.
Thanks heaps
Ian

Quoted 2 lines

Signed-off-by Jeff Mahoney <[email protected]>

Acked-by Ian Kent <[email protected]>

Quoted 29 lines

---

 fs/autofs4/root.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/autofs4/root.c +3 -3

@@ -470,9 +470,6 @@ void autofs4_dentry_release(struct dentr
 	if (inf) {
 		struct autofs_sb_info *sbi = autofs4_sbi(de->d_sb);
 
-		inf->dentry = NULL;
-		inf->inode = NULL;
-
 		if (sbi) {
 			spin_lock(&sbi->rehash_lock);
 			if (!list_empty(&inf->rehash))

@@ -480,6 +477,9 @@ void autofs4_dentry_release(struct dentr
 			spin_unlock(&sbi->rehash_lock);
 		}
 
+		inf->dentry = NULL;
+		inf->inode = NULL;
+
 		autofs4_free_ino(inf);
 	}
 }

Ian Kent

2007-04-11 18:10 UTC #3 lore raw

Quoted 19 lines · Ian Kent

Quoted 12 lines · Ian Kent
Quoted 11 lines · Jeff Mahoney
Commit f50b6f8691cae2e0064c499dd3ef3f31142987f0 introduced a race in autofs4 between autofs_lookup_unhashed() and autofs_dentry_release().
autofs_dentry_release() ends up clearing the ->dentry and ->inode members of autofs_info before removing it from the rehash list. The list is protected by the rehash lock in both functions, but since autofs_dentry_release() starts tearing the autofs_info struct down before removing it from the list, autofs_lookup_unhashed() can get a autofs_info with a NULL dentry.
Quoted 5 lines
Quoted 2 lines
This patch moves the clearing of ->dentry and ->inode after the removal from the rehash list.
Oh .. excellent, I had a bug report but I just couldn't see it for looking.

Quoted 6 lines

Maybe I've been a bit hasty with the celebration.
It looks like I've got a bigger locking problem here.
If autofs4_dentry_release waits on the rehash lock and autofs4_lookup_unhashed reclaims it then the info struct and the dentry go away unconditionally as the release is called just prior to freeing the dentry memory, right?.

No I'm wrong, dentry_iput holds the dcache locks till it sets d_inode to NULL, the point of the d_inode check in autofs4_lookup_unhashed.

My day is still made.

Quoted 38 lines

You've made my day.
Thanks heaps
Ian

Quoted 2 lines

Signed-off-by Jeff Mahoney <[email protected]>

Acked-by Ian Kent <[email protected]>

Quoted 29 lines

---

 fs/autofs4/root.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/autofs4/root.c +3 -3

@@ -470,9 +470,6 @@ void autofs4_dentry_release(struct dentr
 	if (inf) {
 		struct autofs_sb_info *sbi = autofs4_sbi(de->d_sb);
 
-		inf->dentry = NULL;
-		inf->inode = NULL;
-
 		if (sbi) {
 			spin_lock(&sbi->rehash_lock);
 			if (!list_empty(&inf->rehash))

@@ -480,6 +477,9 @@ void autofs4_dentry_release(struct dentr
 			spin_unlock(&sbi->rehash_lock);
 		}
 
+		inf->dentry = NULL;
+		inf->inode = NULL;
+
 		autofs4_free_ino(inf);
 	}
 }

Jeff Mahoney

author

2007-04-11 18:13 UTC #4 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 25 lines · Ian Kent

Quoted 15 lines · Ian Kent
Quoted 14 lines · Ian Kent
Quoted 13 lines · Jeff Mahoney
Commit f50b6f8691cae2e0064c499dd3ef3f31142987f0 introduced a race in autofs4 between autofs_lookup_unhashed() and autofs_dentry_release().
autofs_dentry_release() ends up clearing the ->dentry and ->inode members of autofs_info before removing it from the rehash list. The list is protected by the rehash lock in both functions, but since autofs_dentry_release() starts tearing the autofs_info struct down before removing it from the list, autofs_lookup_unhashed() can get a autofs_info with a NULL dentry.
This patch moves the clearing of ->dentry and ->inode after the removal from the rehash list.
Quoted 8 lines
Quoted 2 lines
Oh .. excellent, I had a bug report but I just couldn't see it for looking.
Maybe I've been a bit hasty with the celebration.
It looks like I've got a bigger locking problem here.
If autofs4_dentry_release waits on the rehash lock and autofs4_lookup_unhashed reclaims it then the info struct and the dentry go away unconditionally as the release is called just prior to freeing the dentry memory, right?.

Quoted 5 lines

No I'm wrong, dentry_iput holds the dcache locks till it sets d_inode to NULL, the point of the d_inode check in autofs4_lookup_unhashed.
My day is still made.

Ok, that was quick. I thought it was ok, but was about to dig back in to make sure. :)

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFGHSVMLPWxlyuTD7IRArOcAKCGwXJaqObc3ee3W810zv5CU2h/MgCfbLUd 4dR++d9OFimkdWGYcmJvWlE=
=+Tlm
-----END PGP SIGNATURE-----

Ian Kent

2007-04-11 18:15 UTC #5 lore raw

Quoted 29 lines · Jeff Mahoney

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 25 lines · Ian Kent
Quoted 23 lines · Ian Kent
Quoted 12 lines · Ian Kent
Quoted 11 lines · Jeff Mahoney
Commit f50b6f8691cae2e0064c499dd3ef3f31142987f0 introduced a race in autofs4 between autofs_lookup_unhashed() and autofs_dentry_release().
autofs_dentry_release() ends up clearing the ->dentry and ->inode members of autofs_info before removing it from the rehash list. The list is protected by the rehash lock in both functions, but since autofs_dentry_release() starts tearing the autofs_info struct down before removing it from the list, autofs_lookup_unhashed() can get a autofs_info with a NULL dentry.
Quoted 4 lines
Quoted 2 lines
This patch moves the clearing of ->dentry and ->inode after the removal from the rehash list.
Oh .. excellent, I had a bug report but I just couldn't see it for looking.
Maybe I've been a bit hasty with the celebration.
It looks like I've got a bigger locking problem here.
If autofs4_dentry_release waits on the rehash lock and autofs4_lookup_unhashed reclaims it then the info struct and the dentry go away unconditionally as the release is called just prior to freeing the dentry memory, right?.

Quoted 8 lines

Quoted 5 lines
No I'm wrong, dentry_iput holds the dcache locks till it sets d_inode to NULL, the point of the d_inode check in autofs4_lookup_unhashed.
My day is still made.
Ok, that was quick. I thought it was ok, but was about to dig back in to make sure. :)

Ya .. I'll think about it some more but it looks good. Thanks again.

Quoted 14 lines

- -Jeff
- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org
iD8DBQFGHSVMLPWxlyuTD7IRArOcAKCGwXJaqObc3ee3W810zv5CU2h/MgCfbLUd 4dR++d9OFimkdWGYcmJvWlE=
=+Tlm
-----END PGP SIGNATURE-----