[PATCH] autofs4: fix race in unhashed dentry code

[PATCH] autofs4: fix race in unhashed dentry code

[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.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>

---

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

--- a/fs/autofs4/root.c	2007-04-11 09:41:44.000000000 -0400
+++ b/fs/autofs4/root.c	2007-04-11 10:54:37.000000000 -0400
@@ -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
SUSE Labs

Re: [PATCH] autofs4: fix race in unhashed dentry code

[Ian Kent]

On Wed, 2007-04-11 at 11:05 -0400, Jeff Mahoney wrote:
>  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.

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

You've made my day.
Thanks heaps
Ian

> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
Acked-by: Ian Kent <raven@themaw.net>

> 
> ---
> 
>  fs/autofs4/root.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> --- a/fs/autofs4/root.c	2007-04-11 09:41:44.000000000 -0400
> +++ b/fs/autofs4/root.c	2007-04-11 10:54:37.000000000 -0400
> @@ -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);
>  	}
>  }
> 

Re: [PATCH] autofs4: fix race in unhashed dentry code

[Ian Kent]

On Thu, 2007-04-12 at 01:49 +0800, Ian Kent wrote:
> On Wed, 2007-04-11 at 11:05 -0400, Jeff Mahoney wrote:
> >  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.
> 
> 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.

> 
> You've made my day.
> Thanks heaps
> Ian
> 
> > 
> > Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> Acked-by: Ian Kent <raven@themaw.net>
> 
> > 
> > ---
> > 
> >  fs/autofs4/root.c |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > --- a/fs/autofs4/root.c	2007-04-11 09:41:44.000000000 -0400
> > +++ b/fs/autofs4/root.c	2007-04-11 10:54:37.000000000 -0400
> > @@ -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);
> >  	}
> >  }
> > 

Re: [PATCH] autofs4: fix race in unhashed dentry code

[Ian Kent]

On Thu, 2007-04-12 at 02:00 +0800, Ian Kent wrote:
> On Thu, 2007-04-12 at 01:49 +0800, Ian Kent wrote:
> > On Wed, 2007-04-11 at 11:05 -0400, Jeff Mahoney wrote:
> > >  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.
> > 
> > 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?.

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.

> > 
> > You've made my day.
> > Thanks heaps
> > Ian
> > 
> > > 
> > > Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> > Acked-by: Ian Kent <raven@themaw.net>
> > 
> > > 
> > > ---
> > > 
> > >  fs/autofs4/root.c |    6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > --- a/fs/autofs4/root.c	2007-04-11 09:41:44.000000000 -0400
> > > +++ b/fs/autofs4/root.c	2007-04-11 10:54:37.000000000 -0400
> > > @@ -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);
> > >  	}
> > >  }
> > > 

Re: [PATCH] autofs4: fix race in unhashed dentry code

[Jeff Mahoney]

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

Ian Kent wrote:
> On Thu, 2007-04-12 at 02:00 +0800, Ian Kent wrote:
>> On Thu, 2007-04-12 at 01:49 +0800, Ian Kent wrote:
>>> On Wed, 2007-04-11 at 11:05 -0400, Jeff Mahoney wrote:
>>>>  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.
>>> 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?.
> 
> 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-----

Re: [PATCH] autofs4: fix race in unhashed dentry code

[Ian Kent]

On Wed, 2007-04-11 at 14:13 -0400, Jeff Mahoney wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Ian Kent wrote:
> > On Thu, 2007-04-12 at 02:00 +0800, Ian Kent wrote:
> >> On Thu, 2007-04-12 at 01:49 +0800, Ian Kent wrote:
> >>> On Wed, 2007-04-11 at 11:05 -0400, Jeff Mahoney wrote:
> >>>>  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.
> >>> 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?.
> > 
> > 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.

> 
> - -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-----