On Thu, 4 Sep 2014 08:38:31 -0400 Jeff Layton <[email protected]> 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 <[email protected]> > Reviewed-by: Christoph Hellwig <[email protected]> > --- > 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
pgpzCubSAFRgc.pgp
Description: OpenPGP digital signature
