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

[Jeff Layton]

On Tue, 13 Jan 2015 12:03:43 +1300
NeilBrown <neilb@xxxxxxx> wrote:

> On Thu,  4 Sep 2014 08:38:31 -0400 Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > ---
> >  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!
-- 
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>

Attachment: pgp9wIK9sAoU3.pgp
Description: OpenPGP digital signature