Re: [PATCH 2/2] dlm: Remove obsolete lockspace lookup

[David Teigland <teigland@redhat.com>]

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 <pcaulfie@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 <pcaulfie@redhat.com>
    Signed-off-by: Steven Whitehouse <swhiteho@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 <swhiteho@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
>