On Thu, Feb 18, 2010 at 09:16:03AM +0000, Steven Whitehouse wrote:
> I'm not sure what more I can say here.... this is a sysfs file store
> function and one of the reasons for using it is that sysfs looks after
> the ref counting for you.
>
> Even aside from that, if you don't have a reference to the lockspace,
> then the dereference that is done to discover the lockspace name would
> be invalid, since the structure might have already been freed before the
> reference is obtained.
>
> You could also compare with with the other store and show functions in
> that same file and notice that none of them try to grab a reference to
> the lockspace in that way. So if this is required, then it must be
> required for those functions too.
>
> Either way there is something not quite right here and having studied
> the code in some detail, I'm pretty sure this is the correct fix,
I guess you didn't see this oops in your tests. Can you show that the
situation in this commit is no longer possible?
commit e2de7f565521a76fbbb927f701c5a1d381c71a93
Author: Patrick Caulfield <pcaul...@redhat.com>
Date: Mon Nov 6 08:53:28 2006 +0000
[DLM] fix oops in kref_put when removing a lockspace
Now that the lockspace struct is freed when the last sysfs object is
released
this patch prevents use of that lockspace by sysfs. We attempt to re-get the
lockspace from the lockspace list and fail the request if it has been
removed.
Signed-Off-By: Patrick Caulfield <pcaul...@redhat.com>
Signed-off-by: Steven Whitehouse <swhit...@redhat.com>
diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
index 499ee11..f8842ca 100644
--- a/fs/dlm/lockspace.c
+++ b/fs/dlm/lockspace.c
@@ -43,6 +43,10 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const
char *buf, size_t len)
ssize_t ret = len;
int n = simple_strtol(buf, NULL, 0);
+ ls = dlm_find_lockspace_local(ls->ls_local_handle);
+ if (!ls)
+ return -EINVAL;
+
switch (n) {
case 0:
dlm_ls_stop(ls);
@@ -53,6 +57,7 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const
char *buf, size_t len)
default:
ret = -EINVAL;
}
+ dlm_put_lockspace(ls);
return ret;
}
>
> Steve.
>
> > > Signed-off-by: Steven Whitehouse <swhit...@redhat.com>
> > > ---
> > > fs/dlm/lockspace.c | 6 +-----
> > > 1 files changed, 1 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
> > > index 26a8bd4..ce0fdf5 100644
> > > --- a/fs/dlm/lockspace.c
> > > +++ b/fs/dlm/lockspace.c
> > > @@ -37,10 +37,6 @@ static ssize_t dlm_control_store(struct dlm_ls *ls,
> > > const char *buf, size_t len)
> > > ssize_t ret = len;
> > > int n = simple_strtol(buf, NULL, 0);
> > >
> > > - ls = dlm_find_lockspace_local(ls->ls_local_handle);
> > > - if (!ls)
> > > - return -EINVAL;
> > > -
> > > switch (n) {
> > > case 0:
> > > dlm_ls_stop(ls);
> > > @@ -51,7 +47,7 @@ static ssize_t dlm_control_store(struct dlm_ls *ls,
> > > const char *buf, size_t len)
> > > default:
> > > ret = -EINVAL;
> > > }
> > > - dlm_put_lockspace(ls);
> > > +
> > > return ret;
> > > }
> > >
> > > --
> > > 1.6.2.5
>
