Re: [PATCH v2 05/17] locks: generic_delete_lease doesn't need a file_lock at all

[NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3698 bytes --]

On Mon, 12 Jan 2015 18:25:00 -0500 Jeff Layton <jeff.layton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
wrote:

> On Tue, 13 Jan 2015 12:03:43 +1300
> NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org> wrote:
> 
> > On Thu,  4 Sep 2014 08:38:31 -0400 Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
> > wrote:
> > 
> > > Ensure that it's OK to pass in a NULL file_lock double pointer on
> > > a F_UNLCK request and convert the vfs_setlease F_UNLCK callers to
> > > do just that.
> > > 
> > > Finally, turn the BUG_ON in generic_setlease into a WARN_ON_ONCE
> > > with an error return. That's a problem we can handle without
> > > crashing the box if it occurs.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
> > > Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> > > ---
> > >  fs/locks.c                      | 34 ++++++++++++++--------------------
> > >  fs/nfsd/nfs4state.c             |  2 +-
> > >  include/trace/events/filelock.h | 14 +++++++-------
> > >  3 files changed, 22 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index 4031324e6cca..1289b74fffbf 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -1637,22 +1637,23 @@ out:
> > >  	return error;
> > >  }
> > >  
> > > -static int generic_delete_lease(struct file *filp, struct file_lock **flp)
> > > +static int generic_delete_lease(struct file *filp)
> > >  {
> > > +	int error = -EAGAIN;
> > >  	struct file_lock *fl, **before;
> > >  	struct dentry *dentry = filp->f_path.dentry;
> > >  	struct inode *inode = dentry->d_inode;
> > >  
> > > -	trace_generic_delete_lease(inode, *flp);
> > > -
> > >  	for (before = &inode->i_flock;
> > >  			((fl = *before) != NULL) && IS_LEASE(fl);
> > >  			before = &fl->fl_next) {
> > > -		if (fl->fl_file != filp)
> > > -			continue;
> > > -		return (*flp)->fl_lmops->lm_change(before, F_UNLCK);
> > > +		if (fl->fl_file == filp)
> > > +			break;
> > >  	}
> > > -	return -EAGAIN;
> > > +	trace_generic_delete_lease(inode, fl);
> > > +	if (fl)
> > > +		error = fl->fl_lmops->lm_change(before, F_UNLCK);
> > > +	return error;
> > >  }
> > 
> > Hi Jeff,
> >  I have a report of a crash in 3.18 because fl->fl_lmops is NULL in the above.
> >    https://bugzilla.suse.com/show_bug.cgi?id=912569
> > 
> > I assume this happens because a file_lock is found which is not IS_LEASE.
> > When that happens, the loop will abort, but fl will not be NULL.
> > As non-LEASE locks have  a NULL fl_lmops, we crash.
> > 
> > I would be inclined to put the code back the way it was, and just move the
> > trace_generic_delete_lease call.
> > 
> > Alternately we could make it
> > 
> >   if (fl && IS_LEASE(fl))
> >           error = fl->fl_lmops-> .....
> > 
> > What do you think?
> > 
> > NeilBrown
> 
> Doh! Well spotted...
> 
> Either fix sounds fine as long as we don't make generic_delete_lease
> require a "flp" arg again. IOW, if you do make the code work similarly
> to how it did before, then we should do:
> 
>     return fl->fl_lmops->lm_change(before, F_UNLCK);
> 
> ...rather than trying to use the ops from a completely different struct
> file_lock argument that's passed in.
> 
> FWIW, I have an overhaul of the locking code that is queued for v3.20
> that will also fix this (as we'll be moving all of the different locks
> to separate lists), but we'll obviously need to queue up a patch for
> stable for this in the interim.
> 
> Thanks!


As you are going to re-write it all I won't try to make it elegant, just a
simple fix.  I'll post shortly.

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]