On Mon, 12 Jan 2015 18:25:00 -0500 Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
wrote:
> 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!
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:
pgpfsaE6pZTRj.pgp
Description: OpenPGP digital signature
