Re: [PATCH] lockd: fix race in nlm_release()
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 - 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
[PATCH] lockd: fix race in nlm_release()
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
-
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
Re: [PATCH] lockd: fix race in nlm_release()
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
-
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
Re: [PATCH] lockd: fix race in nlm_release()
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.
-
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
Re: [PATCH] lockd: fix race in nlm_release()
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' :-)
-
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
Re: [PATCH] lockd: fix race in nlm_release()
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
-
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
Re: [PATCH] lockd: fix race in nlm_release()
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. - 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
