[PATCH] lockd: fix race in nlm_release()

[PATCH] lockd: fix race in nlm_release()

[J. Bruce Fields]

From: J. Bruce Fields <bfields@citi.umich.edu>

The sm_count is decremented to zero but left on the nsm_handles list.
So in the space between decrementing sm_count and acquiring nsm_mutex,
it is possible for another task to find this nsm_handle, increment the
use count and then enter nsm_release itself.

Thus there's nothing to prevent the nsm being freed before we acquire
nsm_mutex here.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/lockd/host.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

Am I missing something here?--b.

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index c3f1194..960911c 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -529,12 +529,10 @@ nsm_release(struct nsm_handle *nsm)
 {
 	if (!nsm)
 		return;
+	mutex_lock(&nsm_mutex);
 	if (atomic_dec_and_test(&nsm->sm_count)) {
-		mutex_lock(&nsm_mutex);
-		if (atomic_read(&nsm->sm_count) == 0) {
-			list_del(&nsm->sm_link);
-			kfree(nsm);
-		}
-		mutex_unlock(&nsm_mutex);
+		list_del(&nsm->sm_link);
+		kfree(nsm);
 	}
+	mutex_unlock(&nsm_mutex);
 }
-- 
1.5.4.rc2.60.gb2e62

Re: [PATCH] lockd: fix race in nlm_release()

[Trond Myklebust]

On Wed, 2008-02-20 at 14:11 -0500, J. Bruce Fields wrote:
> From: J. Bruce Fields <bfields@citi.umich.edu>
> 
> The sm_count is decremented to zero but left on the nsm_handles list.
> So in the space between decrementing sm_count and acquiring nsm_mutex,
> it is possible for another task to find this nsm_handle, increment the
> use count and then enter nsm_release itself.
> 
> Thus there's nothing to prevent the nsm being freed before we acquire
> nsm_mutex here.
> 
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  fs/lockd/host.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 
> Am I missing something here?--b.
> 
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index c3f1194..960911c 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -529,12 +529,10 @@ nsm_release(struct nsm_handle *nsm)
>  {
>  	if (!nsm)
>  		return;
> +	mutex_lock(&nsm_mutex);
>  	if (atomic_dec_and_test(&nsm->sm_count)) {
> -		mutex_lock(&nsm_mutex);
> -		if (atomic_read(&nsm->sm_count) == 0) {
> -			list_del(&nsm->sm_link);
> -			kfree(nsm);
> -		}
> -		mutex_unlock(&nsm_mutex);
> +		list_del(&nsm->sm_link);
> +		kfree(nsm);
>  	}
> +	mutex_unlock(&nsm_mutex);
>  }

It would be nice to get rid of that mutex. That should really be either
a spinlock or an rcu-protected list...

Trond

Re: [PATCH] lockd: fix race in nlm_release()

[J. Bruce Fields]

On Wed, Feb 20, 2008 at 02:24:26PM -0500, Trond Myklebust wrote:
> 
> On Wed, 2008-02-20 at 14:11 -0500, J. Bruce Fields wrote:
> > From: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > The sm_count is decremented to zero but left on the nsm_handles list.
> > So in the space between decrementing sm_count and acquiring nsm_mutex,
> > it is possible for another task to find this nsm_handle, increment the
> > use count and then enter nsm_release itself.
> > 
> > Thus there's nothing to prevent the nsm being freed before we acquire
> > nsm_mutex here.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > ---
> >  fs/lockd/host.c |   10 ++++------
> >  1 files changed, 4 insertions(+), 6 deletions(-)
> > 
> > Am I missing something here?--b.
> > 
> > diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> > index c3f1194..960911c 100644
> > --- a/fs/lockd/host.c
> > +++ b/fs/lockd/host.c
> > @@ -529,12 +529,10 @@ nsm_release(struct nsm_handle *nsm)
> >  {
> >  	if (!nsm)
> >  		return;
> > +	mutex_lock(&nsm_mutex);
> >  	if (atomic_dec_and_test(&nsm->sm_count)) {
> > -		mutex_lock(&nsm_mutex);
> > -		if (atomic_read(&nsm->sm_count) == 0) {
> > -			list_del(&nsm->sm_link);
> > -			kfree(nsm);
> > -		}
> > -		mutex_unlock(&nsm_mutex);
> > +		list_del(&nsm->sm_link);
> > +		kfree(nsm);
> >  	}
> > +	mutex_unlock(&nsm_mutex);
> >  }
> 
> It would be nice to get rid of that mutex. That should really be either
> a spinlock or an rcu-protected list...

OK, I'll look into doing that next.

If you've got any other suggestions while I'm in the general area, I'm
all ears.

--b.

Re: [PATCH] lockd: fix race in nlm_release()

[Trond Myklebust]

On Wed, 2008-02-20 at 14:27 -0500, J. Bruce Fields wrote:
> On Wed, Feb 20, 2008 at 02:24:26PM -0500, Trond Myklebust wrote:
> > 
> > On Wed, 2008-02-20 at 14:11 -0500, J. Bruce Fields wrote:
> > > From: J. Bruce Fields <bfields@citi.umich.edu>
> > > 
> > > The sm_count is decremented to zero but left on the nsm_handles list.
> > > So in the space between decrementing sm_count and acquiring nsm_mutex,
> > > it is possible for another task to find this nsm_handle, increment the
> > > use count and then enter nsm_release itself.
> > > 
> > > Thus there's nothing to prevent the nsm being freed before we acquire
> > > nsm_mutex here.
> > > 
> > > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > > ---
> > >  fs/lockd/host.c |   10 ++++------
> > >  1 files changed, 4 insertions(+), 6 deletions(-)
> > > 
> > > Am I missing something here?--b.
> > > 
> > > diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> > > index c3f1194..960911c 100644
> > > --- a/fs/lockd/host.c
> > > +++ b/fs/lockd/host.c
> > > @@ -529,12 +529,10 @@ nsm_release(struct nsm_handle *nsm)
> > >  {
> > >  	if (!nsm)
> > >  		return;
> > > +	mutex_lock(&nsm_mutex);
> > >  	if (atomic_dec_and_test(&nsm->sm_count)) {
> > > -		mutex_lock(&nsm_mutex);
> > > -		if (atomic_read(&nsm->sm_count) == 0) {
> > > -			list_del(&nsm->sm_link);
> > > -			kfree(nsm);
> > > -		}
> > > -		mutex_unlock(&nsm_mutex);
> > > +		list_del(&nsm->sm_link);
> > > +		kfree(nsm);
> > >  	}
> > > +	mutex_unlock(&nsm_mutex);
> > >  }
> > 
> > It would be nice to get rid of that mutex. That should really be either
> > a spinlock or an rcu-protected list...
> 
> OK, I'll look into doing that next.
> 
> If you've got any other suggestions while I'm in the general area, I'm
> all ears.

Just the usual plea to replace the host->h_server flag with 2 separate
lists: one list of client nlm_hosts, and one list of server
nlm_hosts :-)

...Oh and a minor optimisation: If we're using a loopback mount, I don't
think we'll ever need to monitor 'localhost' :-)

Re: [PATCH] lockd: fix race in nlm_release()

[Chuck Lever]

On Feb 20, 2008, at 2:48 PM, Trond Myklebust wrote:
> On Wed, 2008-02-20 at 14:27 -0500, J. Bruce Fields wrote:
>> On Wed, Feb 20, 2008 at 02:24:26PM -0500, Trond Myklebust wrote:
>>>
>>> On Wed, 2008-02-20 at 14:11 -0500, J. Bruce Fields wrote:
>>>> From: J. Bruce Fields <bfields@citi.umich.edu>
>>>>
>>>> The sm_count is decremented to zero but left on the nsm_handles  
>>>> list.
>>>> So in the space between decrementing sm_count and acquiring  
>>>> nsm_mutex,
>>>> it is possible for another task to find this nsm_handle,  
>>>> increment the
>>>> use count and then enter nsm_release itself.
>>>>
>>>> Thus there's nothing to prevent the nsm being freed before we  
>>>> acquire
>>>> nsm_mutex here.
>>>>
>>>> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>>>> ---
>>>>  fs/lockd/host.c |   10 ++++------
>>>>  1 files changed, 4 insertions(+), 6 deletions(-)
>>>>
>>>> Am I missing something here?--b.
>>>>
>>>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>>>> index c3f1194..960911c 100644
>>>> --- a/fs/lockd/host.c
>>>> +++ b/fs/lockd/host.c
>>>> @@ -529,12 +529,10 @@ nsm_release(struct nsm_handle *nsm)
>>>>  {
>>>>  	if (!nsm)
>>>>  		return;
>>>> +	mutex_lock(&nsm_mutex);
>>>>  	if (atomic_dec_and_test(&nsm->sm_count)) {
>>>> -		mutex_lock(&nsm_mutex);
>>>> -		if (atomic_read(&nsm->sm_count) == 0) {
>>>> -			list_del(&nsm->sm_link);
>>>> -			kfree(nsm);
>>>> -		}
>>>> -		mutex_unlock(&nsm_mutex);
>>>> +		list_del(&nsm->sm_link);
>>>> +		kfree(nsm);
>>>>  	}
>>>> +	mutex_unlock(&nsm_mutex);
>>>>  }
>>>
>>> It would be nice to get rid of that mutex. That should really be  
>>> either
>>> a spinlock or an rcu-protected list...
>>
>> OK, I'll look into doing that next.
>>
>> If you've got any other suggestions while I'm in the general area,  
>> I'm
>> all ears.
>
> Just the usual plea to replace the host->h_server flag with 2 separate
> lists: one list of client nlm_hosts, and one list of server
> nlm_hosts :-)

I have no objection to that, but my NLM IPv6 patches will be  
significantly affected by such a change right at this point.  Can we  
hold off until the IPv6 work is integrated, or make this change part  
of the IPv6 work itself?

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] lockd: fix race in nlm_release()

[J. Bruce Fields]

On Wed, Feb 20, 2008 at 05:10:24PM -0500, Chuck Lever wrote:
> On Feb 20, 2008, at 2:48 PM, Trond Myklebust wrote:
>> Just the usual plea to replace the host->h_server flag with 2 separate
>> lists: one list of client nlm_hosts, and one list of server
>> nlm_hosts :-)
>
> I have no objection to that, but my NLM IPv6 patches will be  
> significantly affected by such a change right at this point.  Can we  
> hold off until the IPv6 work is integrated, or make this change part of 
> the IPv6 work itself?

Just speak up before I try to merge it, and we'll work something out....

--b.

Re: [PATCH] lockd: fix race in nlm_release()

[J. Bruce Fields]

On Wed, Feb 20, 2008 at 02:48:38PM -0500, Trond Myklebust wrote:
> 
> On Wed, 2008-02-20 at 14:27 -0500, J. Bruce Fields wrote:
> > On Wed, Feb 20, 2008 at 02:24:26PM -0500, Trond Myklebust wrote:
> > > 
> > > On Wed, 2008-02-20 at 14:11 -0500, J. Bruce Fields wrote:
> > > > From: J. Bruce Fields <bfields@citi.umich.edu>
> > > > 
> > > > The sm_count is decremented to zero but left on the nsm_handles list.
> > > > So in the space between decrementing sm_count and acquiring nsm_mutex,
> > > > it is possible for another task to find this nsm_handle, increment the
> > > > use count and then enter nsm_release itself.
> > > > 
> > > > Thus there's nothing to prevent the nsm being freed before we acquire
> > > > nsm_mutex here.
> > > > 
> > > > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > > > ---
> > > >  fs/lockd/host.c |   10 ++++------
> > > >  1 files changed, 4 insertions(+), 6 deletions(-)
> > > > 
> > > > Am I missing something here?--b.
> > > > 
> > > > diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> > > > index c3f1194..960911c 100644
> > > > --- a/fs/lockd/host.c
> > > > +++ b/fs/lockd/host.c
...
> > If you've got any other suggestions while I'm in the general area, I'm
> > all ears.
> 
> Just the usual plea to replace the host->h_server flag with 2 separate
> lists: one list of client nlm_hosts, and one list of server
> nlm_hosts :-)

OK, I'm looking at that.

> ...Oh and a minor optimisation: If we're using a loopback mount, I don't
> think we'll ever need to monitor 'localhost' :-)

I assumed one of the only uses for loopback mounts was for testing and
development, in which case it's better to keep the loopback behavior
similar to non-loopback behavior, even if it's a little silly.  No?

--b.

Re: [PATCH] lockd: fix race in nlm_release()

[Peter Staubach]

J. Bruce Fields wrote:
> On Wed, Feb 20, 2008 at 02:48:38PM -0500, Trond Myklebust wrote:
>   
>> On Wed, 2008-02-20 at 14:27 -0500, J. Bruce Fields wrote:
>>     
>>> On Wed, Feb 20, 2008 at 02:24:26PM -0500, Trond Myklebust wrote:
>>>       
>>>> On Wed, 2008-02-20 at 14:11 -0500, J. Bruce Fields wrote:
>>>>         
>>>>> From: J. Bruce Fields <bfields@citi.umich.edu>
>>>>>
>>>>> The sm_count is decremented to zero but left on the nsm_handles list.
>>>>> So in the space between decrementing sm_count and acquiring nsm_mutex,
>>>>> it is possible for another task to find this nsm_handle, increment the
>>>>> use count and then enter nsm_release itself.
>>>>>
>>>>> Thus there's nothing to prevent the nsm being freed before we acquire
>>>>> nsm_mutex here.
>>>>>
>>>>> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>>>>> ---
>>>>>  fs/lockd/host.c |   10 ++++------
>>>>>  1 files changed, 4 insertions(+), 6 deletions(-)
>>>>>
>>>>> Am I missing something here?--b.
>>>>>
>>>>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>>>>> index c3f1194..960911c 100644
>>>>> --- a/fs/lockd/host.c
>>>>> +++ b/fs/lockd/host.c
>>>>>           
> ...
>   
>>> If you've got any other suggestions while I'm in the general area, I'm
>>> all ears.
>>>       
>> Just the usual plea to replace the host->h_server flag with 2 separate
>> lists: one list of client nlm_hosts, and one list of server
>> nlm_hosts :-)
>>     
>
> OK, I'm looking at that.
>
>   
>> ...Oh and a minor optimisation: If we're using a loopback mount, I don't
>> think we'll ever need to monitor 'localhost' :-)
>>     
>
> I assumed one of the only uses for loopback mounts was for testing and
> development, in which case it's better to keep the loopback behavior
> similar to non-loopback behavior, even if it's a little silly.  No?

We have encountered a number of customers, who are trying to deploy
in a cluster architecture, who mount over loopback.  We have tried
to convince them that this is a bad idea and/or that they should use
something like bind mounts instead, but they insist.

We try not to look too surprised when they encounter issues...  :-)

       ps