From: J. Bruce Fields <[email protected]>
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 <[email protected]>
---
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
On Wed, 2008-02-20 at 14:11 -0500, J. Bruce Fields wrote:
> From: J. Bruce Fields <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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' :-)
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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.
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.
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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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