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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1266484563.2297.6.camel@localhost>
Date:	Thu, 18 Feb 2010 09:16:03 +0000
From:	Steven Whitehouse <swhiteho@...hat.com>
To:	David Teigland <teigland@...hat.com>
Cc:	cluster-devel@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] dlm: Remove obsolete lockspace lookup

Hi,

On Wed, 2010-02-17 at 15:12 -0500, David Teigland wrote:
> On Wed, Feb 17, 2010 at 09:41:35AM +0000, Steven Whitehouse wrote:
> > We don't need to look up the lockspace in this particular
> > case since we already have a pointer to it (which was being
> > dereferenced in order to do the lookup in the first place).
> 
> It'll take more to convince me that that reference from find isn't needed.
> My assumption is that I added it because it was.
> 
> Dave
> 
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,

Steve.

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/