v2:
- cleanups and fixes in response to HCH's review
- make security_file_set_fowner a void return operation
- consolidate "nosetlease" routines
- remove "deadlock protection" from generic_add_lease routine
- add return value to lm_break op to allow for direct lease removal
The internal "API" for handling leases has a lot of problems. The main
one is that on success it can return a pointer to a lease that sits on
the inode's i_flock list. That pointer is only guaranteed to be valid
until the i_lock is dropped, which makes it a bit dangerous to use.
Also, the i_lock is held over much too much of the code, which
precludes any hope of ever adding proper support for leases to
distributed filesystems.
This patchset is a cleanup and overhaul of the internal lease API, and a
few stray locking-related patches. It fixes a number of problems in that
code and makes an attempt at making that API more sane to use.
The only real consumer of that API is knfsd, but this should make it
easier for others to do so, reduce and clarify the spinlocking involved
in handling leases, and get us a step closer toward allowing lease
implementations that can block.
I'm targeting this work for v3.18. Review would be welcome...
Jeff Layton (17):
locks: consolidate "nolease" routines
security: make security_file_set_fowner, f_setown and __f_setown void
return
locks: close potential race in lease_get_mtime
nfsd: fix potential lease memory leak in nfs4_setlease
locks: generic_delete_lease doesn't need a file_lock at all
locks: clean up vfs_setlease kerneldoc comments
nfsd: don't keep a pointer to the lease in nfs4_file
locks: plumb a "priv" pointer into the setlease routines
locks: define a lm_setup handler for leases
locks: move i_lock acquisition into generic_*_lease handlers
locks: move freeing of leases outside of i_lock
locks: update Documentation/filesystems with new setlease semantics
locks: remove i_have_this_lease check from __break_lease
locks: __break_lease cleanup in preparation of allowing direct removal
of leases
locks: give lm_break a return value
locks: set fl_owner for leases to filp instead of current->files
locks: clean up comments over fl_owner_t definition
Documentation/filesystems/Locking | 11 +-
Documentation/filesystems/vfs.txt | 7 +-
drivers/net/tun.c | 4 +-
drivers/tty/tty_io.c | 3 +-
fs/cifs/cifsfs.c | 7 +-
fs/fcntl.c | 21 +--
fs/gfs2/file.c | 22 +--
fs/libfs.c | 18 +++
fs/locks.c | 297 ++++++++++++++++++++------------------
fs/nfs/file.c | 13 +-
fs/nfs/internal.h | 1 -
fs/nfs/nfs4file.c | 2 +-
fs/nfsd/nfs4state.c | 44 +++---
fs/nfsd/state.h | 1 -
fs/notify/dnotify/dnotify.c | 8 +-
include/linux/fs.h | 41 ++++--
include/linux/security.h | 8 +-
include/trace/events/filelock.h | 14 +-
net/socket.c | 3 +-
security/capability.c | 4 +-
security/security.c | 2 +-
security/selinux/hooks.c | 4 +-
security/smack/smack_lsm.c | 3 +-
23 files changed, 269 insertions(+), 269 deletions(-)
--
1.9.3
GFS2 and NFS have setlease routines that always just return -EINVAL.
Turn that into a generic routine that can live in fs/libfs.c.
Cc: Trond Myklebust <[email protected]>
Cc: <[email protected]>
Cc: Steven Whitehouse <[email protected]>
Cc: <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/gfs2/file.c | 22 +---------------------
fs/libfs.c | 16 ++++++++++++++++
fs/nfs/file.c | 13 +------------
fs/nfs/internal.h | 1 -
fs/nfs/nfs4file.c | 2 +-
include/linux/fs.h | 1 +
6 files changed, 20 insertions(+), 35 deletions(-)
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 26b3f952e6b1..2c02478a86b0 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -913,26 +913,6 @@ out_uninit:
#ifdef CONFIG_GFS2_FS_LOCKING_DLM
/**
- * gfs2_setlease - acquire/release a file lease
- * @file: the file pointer
- * @arg: lease type
- * @fl: file lock
- *
- * We don't currently have a way to enforce a lease across the whole
- * cluster; until we do, disable leases (by just returning -EINVAL),
- * unless the administrator has requested purely local locking.
- *
- * Locking: called under i_lock
- *
- * Returns: errno
- */
-
-static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl)
-{
- return -EINVAL;
-}
-
-/**
* gfs2_lock - acquire/release a posix lock on a file
* @file: the file pointer
* @cmd: either modify or retrieve lock state, possibly wait
@@ -1069,7 +1049,7 @@ const struct file_operations gfs2_file_fops = {
.flock = gfs2_flock,
.splice_read = generic_file_splice_read,
.splice_write = iter_file_splice_write,
- .setlease = gfs2_setlease,
+ .setlease = simple_nosetlease,
.fallocate = gfs2_fallocate,
};
diff --git a/fs/libfs.c b/fs/libfs.c
index 88e3e00e2eca..29012a303ef8 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1075,3 +1075,19 @@ struct inode *alloc_anon_inode(struct super_block *s)
return inode;
}
EXPORT_SYMBOL(alloc_anon_inode);
+
+/**
+ * simple_nosetlease - generic helper for prohibiting leases
+ * @filp: file pointer
+ * @arg: type of lease to obtain
+ * @flp: new lease supplied for insertion
+ *
+ * Generic helper for filesystems that do not wish to allow leases to be set.
+ * All arguments are ignored and it just returns -EINVAL.
+ */
+int
+simple_nosetlease(struct file *filp, long arg, struct file_lock **flp)
+{
+ return -EINVAL;
+}
+EXPORT_SYMBOL(simple_nosetlease);
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 524dd80d1898..8c4048ecdad1 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -891,17 +891,6 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
}
EXPORT_SYMBOL_GPL(nfs_flock);
-/*
- * There is no protocol support for leases, so we have no way to implement
- * them correctly in the face of opens by other clients.
- */
-int nfs_setlease(struct file *file, long arg, struct file_lock **fl)
-{
- dprintk("NFS: setlease(%pD2, arg=%ld)\n", file, arg);
- return -EINVAL;
-}
-EXPORT_SYMBOL_GPL(nfs_setlease);
-
const struct file_operations nfs_file_operations = {
.llseek = nfs_file_llseek,
.read = new_sync_read,
@@ -918,6 +907,6 @@ const struct file_operations nfs_file_operations = {
.splice_read = nfs_file_splice_read,
.splice_write = iter_file_splice_write,
.check_flags = nfs_check_flags,
- .setlease = nfs_setlease,
+ .setlease = simple_nosetlease,
};
EXPORT_SYMBOL_GPL(nfs_file_operations);
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9056622d2230..94d922ebb5ac 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -346,7 +346,6 @@ int nfs_file_release(struct inode *, struct file *);
int nfs_lock(struct file *, int, struct file_lock *);
int nfs_flock(struct file *, int, struct file_lock *);
int nfs_check_flags(int);
-int nfs_setlease(struct file *, long, struct file_lock **);
/* inode.c */
extern struct workqueue_struct *nfsiod_workqueue;
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index a816f0627a6c..3e987ad9ae25 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -131,5 +131,5 @@ const struct file_operations nfs4_file_operations = {
.splice_read = nfs_file_splice_read,
.splice_write = iter_file_splice_write,
.check_flags = nfs_check_flags,
- .setlease = nfs_setlease,
+ .setlease = simple_nosetlease,
};
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 458f733c96bd..435e3d9ec5cf 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2599,6 +2599,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping,
struct page *page, void *fsdata);
extern int always_delete_dentry(const struct dentry *);
extern struct inode *alloc_anon_inode(struct super_block *);
+extern int simple_nosetlease(struct file *, long, struct file_lock **);
extern const struct dentry_operations simple_dentry_operations;
extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags);
--
1.9.3
On Thu, Sep 04, 2014 at 08:38:34AM -0400, Jeff Layton wrote:
> In later patches, we're going to add a new lock_manager_operation to
> finish setting up the lease while still holding the i_lock. To do
> this, we'll need to pass a little bit of info in the fcntl setlease
> case (primarily an fasync structure). Plumb the extra pointer into
> there in advance of that.
>
> We declare this pointer as a void ** to make it clear that this is
> private info, and that the caller isn't required to set this unless
> the lm_setup specifically requires it.
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Thu, 4 Sep 2014 10:50:43 -0700
Christoph Hellwig <[email protected]> wrote:
> Looks good, but I'd rather merge it into the patch that changes the
> semantics.
>
> Reviewed-by: Christoph Hellwig <[email protected]>
Fair enough. I'll merge the changes to the Documentation in with the
patches that change them.
--
Jeff Layton <[email protected]>
On Thu, Sep 04, 2014 at 08:38:43AM -0400, Jeff Layton wrote:
> The contents of an fl_owner_t have morphed a bit over the years, fix up
> the comments to account for the changes.
Instead of listing all the owners I'd just put something in like:
/* legacy typedef, will go away soon */
On Thu, 4 Sep 2014 10:53:34 -0700
Christoph Hellwig <[email protected]> wrote:
> On Thu, Sep 04, 2014 at 08:38:43AM -0400, Jeff Layton wrote:
> > The contents of an fl_owner_t have morphed a bit over the years, fix up
> > the comments to account for the changes.
>
> Instead of listing all the owners I'd just put something in like:
>
> /* legacy typedef, will go away soon */
For some definition of "soon" ;)? How about:
/* legacy typedef, should eventually go away */
I'll change it to read that and probably squash this change in with
another patch.
--
Jeff Layton <[email protected]>
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;
}
/**
@@ -1682,13 +1683,15 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
time_out_leases(inode);
- BUG_ON(!(*flp)->fl_lmops->lm_break);
-
switch (arg) {
case F_UNLCK:
- return generic_delete_lease(filp, flp);
+ return generic_delete_lease(filp);
case F_RDLCK:
case F_WRLCK:
+ if (!(*flp)->fl_lmops->lm_break) {
+ WARN_ON_ONCE(1);
+ return -ENOLCK;
+ }
return generic_add_lease(filp, arg, flp);
default:
return -EINVAL;
@@ -1744,15 +1747,6 @@ int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
}
EXPORT_SYMBOL_GPL(vfs_setlease);
-static int do_fcntl_delete_lease(struct file *filp)
-{
- struct file_lock fl, *flp = &fl;
-
- lease_init(filp, F_UNLCK, flp);
-
- return vfs_setlease(filp, F_UNLCK, &flp);
-}
-
static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
{
struct file_lock *fl, *ret;
@@ -1809,7 +1803,7 @@ out_unlock:
int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
{
if (arg == F_UNLCK)
- return do_fcntl_delete_lease(filp);
+ return vfs_setlease(filp, F_UNLCK, NULL);
return do_fcntl_add_lease(fd, filp, arg);
}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 29fac18d9102..0cd252916e1a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -683,7 +683,7 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
if (!fp->fi_lease)
return;
if (atomic_dec_and_test(&fp->fi_delegees)) {
- vfs_setlease(fp->fi_deleg_file, F_UNLCK, &fp->fi_lease);
+ vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
fp->fi_lease = NULL;
fput(fp->fi_deleg_file);
fp->fi_deleg_file = NULL;
diff --git a/include/trace/events/filelock.h b/include/trace/events/filelock.h
index 59d11c22f076..a0d008070962 100644
--- a/include/trace/events/filelock.h
+++ b/include/trace/events/filelock.h
@@ -53,15 +53,15 @@ DECLARE_EVENT_CLASS(filelock_lease,
),
TP_fast_assign(
- __entry->fl = fl;
+ __entry->fl = fl ? fl : NULL;
__entry->s_dev = inode->i_sb->s_dev;
__entry->i_ino = inode->i_ino;
- __entry->fl_next = fl->fl_next;
- __entry->fl_owner = fl->fl_owner;
- __entry->fl_flags = fl->fl_flags;
- __entry->fl_type = fl->fl_type;
- __entry->fl_break_time = fl->fl_break_time;
- __entry->fl_downgrade_time = fl->fl_downgrade_time;
+ __entry->fl_next = fl ? fl->fl_next : NULL;
+ __entry->fl_owner = fl ? fl->fl_owner : NULL;
+ __entry->fl_flags = fl ? fl->fl_flags : 0;
+ __entry->fl_type = fl ? fl->fl_type : 0;
+ __entry->fl_break_time = fl ? fl->fl_break_time : 0;
+ __entry->fl_downgrade_time = fl ? fl->fl_downgrade_time : 0;
),
TP_printk("fl=0x%p dev=0x%x:0x%x ino=0x%lx fl_next=0x%p fl_owner=0x%p fl_flags=%s fl_type=%s fl_break_time=%lu fl_downgrade_time=%lu",
--
1.9.3
In later patches, we're going to add a new lock_manager_operation to
finish setting up the lease while still holding the i_lock. To do
this, we'll need to pass a little bit of info in the fcntl setlease
case (primarily an fasync structure). Plumb the extra pointer into
there in advance of that.
We declare this pointer as a void ** to make it clear that this is
private info, and that the caller isn't required to set this unless
the lm_setup specifically requires it.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/cifs/cifsfs.c | 7 ++++---
fs/libfs.c | 4 +++-
fs/locks.c | 32 ++++++++++++++++++++------------
fs/nfsd/nfs4state.c | 4 ++--
include/linux/fs.h | 12 ++++++------
5 files changed, 35 insertions(+), 24 deletions(-)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 889b98455750..9d7996e8e793 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -813,7 +813,8 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int whence)
return generic_file_llseek(file, offset, whence);
}
-static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
+static int
+cifs_setlease(struct file *file, long arg, struct file_lock **lease, void **priv)
{
/*
* Note that this is called by vfs setlease with i_lock held to
@@ -829,7 +830,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
if (arg == F_UNLCK ||
((arg == F_RDLCK) && CIFS_CACHE_READ(CIFS_I(inode))) ||
((arg == F_WRLCK) && CIFS_CACHE_WRITE(CIFS_I(inode))))
- return generic_setlease(file, arg, lease);
+ return generic_setlease(file, arg, lease, priv);
else if (tlink_tcon(cfile->tlink)->local_lease &&
!CIFS_CACHE_READ(CIFS_I(inode)))
/*
@@ -840,7 +841,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
* knows that the file won't be changed on the server by anyone
* else.
*/
- return generic_setlease(file, arg, lease);
+ return generic_setlease(file, arg, lease, priv);
else
return -EAGAIN;
}
diff --git a/fs/libfs.c b/fs/libfs.c
index 29012a303ef8..171d2846f2a3 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1081,12 +1081,14 @@ EXPORT_SYMBOL(alloc_anon_inode);
* @filp: file pointer
* @arg: type of lease to obtain
* @flp: new lease supplied for insertion
+ * @priv: private data for lm_setup operation
*
* Generic helper for filesystems that do not wish to allow leases to be set.
* All arguments are ignored and it just returns -EINVAL.
*/
int
-simple_nosetlease(struct file *filp, long arg, struct file_lock **flp)
+simple_nosetlease(struct file *filp, long arg, struct file_lock **flp,
+ void **priv)
{
return -EINVAL;
}
diff --git a/fs/locks.c b/fs/locks.c
index 81a4628f5594..7162a5836dba 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1297,7 +1297,6 @@ int lease_modify(struct file_lock **before, int arg)
}
return 0;
}
-
EXPORT_SYMBOL(lease_modify);
static bool past_time(unsigned long then)
@@ -1543,7 +1542,8 @@ check_conflicting_open(const struct dentry *dentry, const long arg)
return ret;
}
-static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
+static int
+generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv)
{
struct file_lock *fl, **before, **my_before = NULL, *lease;
struct dentry *dentry = filp->f_path.dentry;
@@ -1630,11 +1630,14 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp
smp_mb();
error = check_conflicting_open(dentry, arg);
if (error)
- locks_unlink_lock(before);
+ goto out_unlink;
out:
if (is_deleg)
mutex_unlock(&inode->i_mutex);
return error;
+out_unlink:
+ locks_unlink_lock(before);
+ goto out;
}
static int generic_delete_lease(struct file *filp)
@@ -1661,13 +1664,15 @@ static int generic_delete_lease(struct file *filp)
* @filp: file pointer
* @arg: type of lease to obtain
* @flp: input - file_lock to use, output - file_lock inserted
+ * @priv: private data for lm_setup
*
* The (input) flp->fl_lmops->lm_break function is required
* by break_lease().
*
* Called with inode->i_lock held.
*/
-int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
+int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
+ void **priv)
{
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
@@ -1692,19 +1697,20 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
WARN_ON_ONCE(1);
return -ENOLCK;
}
- return generic_add_lease(filp, arg, flp);
+ return generic_add_lease(filp, arg, flp, priv);
default:
return -EINVAL;
}
}
EXPORT_SYMBOL(generic_setlease);
-static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
+static int
+__vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv)
{
if (filp->f_op->setlease)
- return filp->f_op->setlease(filp, arg, lease);
+ return filp->f_op->setlease(filp, arg, lease, priv);
else
- return generic_setlease(filp, arg, lease);
+ return generic_setlease(filp, arg, lease, priv);
}
/**
@@ -1712,6 +1718,7 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
* @filp: file pointer
* @arg: type of lease to obtain
* @lease: file_lock to use when adding a lease
+ * @priv: private info for lm_setup when adding a lease
*
* Call this to establish a lease on the file. The "lease" argument is not
* used for F_UNLCK requests and may be NULL. For commands that set or alter
@@ -1720,13 +1727,14 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
* stack trace).
*/
-int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
+int
+vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv)
{
struct inode *inode = file_inode(filp);
int error;
spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, lease);
+ error = __vfs_setlease(filp, arg, lease, priv);
spin_unlock(&inode->i_lock);
return error;
@@ -1751,7 +1759,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
}
ret = fl;
spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, &ret);
+ error = __vfs_setlease(filp, arg, &ret, NULL);
if (error)
goto out_unlock;
if (ret == fl)
@@ -1789,7 +1797,7 @@ out_unlock:
int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
{
if (arg == F_UNLCK)
- return vfs_setlease(filp, F_UNLCK, NULL);
+ return vfs_setlease(filp, F_UNLCK, NULL, NULL);
return do_fcntl_add_lease(fd, filp, arg);
}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d0a6e8e022a2..d964a41c9151 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -683,7 +683,7 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
if (!fp->fi_deleg_file)
return;
if (atomic_dec_and_test(&fp->fi_delegees)) {
- vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
+ vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL, NULL);
fput(fp->fi_deleg_file);
fp->fi_deleg_file = NULL;
}
@@ -3788,7 +3788,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
}
fl->fl_file = filp;
ret = fl;
- status = vfs_setlease(filp, fl->fl_type, &ret);
+ status = vfs_setlease(filp, fl->fl_type, &fl, NULL);
if (status) {
locks_free_lock(fl);
goto out_fput;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 96528f73dda4..f1870eb67b02 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -982,8 +982,8 @@ extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl);
extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);
extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int type);
extern void lease_get_mtime(struct inode *, struct timespec *time);
-extern int generic_setlease(struct file *, long, struct file_lock **);
-extern int vfs_setlease(struct file *, long, struct file_lock **);
+extern int generic_setlease(struct file *, long, struct file_lock **, void **priv);
+extern int vfs_setlease(struct file *, long, struct file_lock **, void **);
extern int lease_modify(struct file_lock **, int);
#else /* !CONFIG_FILE_LOCKING */
static inline int fcntl_getlk(struct file *file, unsigned int cmd,
@@ -1100,13 +1100,13 @@ static inline void lease_get_mtime(struct inode *inode, struct timespec *time)
}
static inline int generic_setlease(struct file *filp, long arg,
- struct file_lock **flp)
+ struct file_lock **flp, void **priv)
{
return -EINVAL;
}
static inline int vfs_setlease(struct file *filp, long arg,
- struct file_lock **lease)
+ struct file_lock **lease, void **priv)
{
return -EINVAL;
}
@@ -1494,7 +1494,7 @@ struct file_operations {
int (*flock) (struct file *, int, struct file_lock *);
ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, loff_t *, size_t, unsigned int);
ssize_t (*splice_read)(struct file *, loff_t *, struct pipe_inode_info *, size_t, unsigned int);
- int (*setlease)(struct file *, long, struct file_lock **);
+ int (*setlease)(struct file *, long, struct file_lock **, void **);
long (*fallocate)(struct file *file, int mode, loff_t offset,
loff_t len);
int (*show_fdinfo)(struct seq_file *m, struct file *f);
@@ -2599,7 +2599,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping,
struct page *page, void *fsdata);
extern int always_delete_dentry(const struct dentry *);
extern struct inode *alloc_anon_inode(struct super_block *);
-extern int simple_nosetlease(struct file *, long, struct file_lock **);
+extern int simple_nosetlease(struct file *, long, struct file_lock **, void **);
extern const struct dentry_operations simple_dentry_operations;
extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags);
--
1.9.3
On Thu, Sep 04, 2014 at 08:38:28AM -0400, Jeff Layton wrote:
> security_file_set_fowner always returns 0, so make it f_setown and
> __f_setown void return functions and fix up the error handling in the
> callers.
All the LSMs seems to use this purely for setting up pointers, and not
having to handle errors in the locking code for it will make life a lot
simpler. I like this a lot.
Reviewed-by: Christoph Hellwig <[email protected]>
There was only one place where we still could free a file_lock while
holding the i_lock -- lease_modify. Add a new list_head argument to the
lm_change operation, pass in a private list when calling it, and fix
those callers to dispose of the list once the lock has been dropped.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/locks.c | 34 ++++++++++++++++++++++------------
fs/nfsd/nfs4state.c | 6 +++---
include/linux/fs.h | 7 ++++---
3 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 5bc61091263f..dc2e9e18f32d 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1292,7 +1292,7 @@ static void lease_clear_pending(struct file_lock *fl, int arg)
}
/* We already had a lease on this file; just change its type */
-int lease_modify(struct file_lock **before, int arg)
+int lease_modify(struct file_lock **before, int arg, struct list_head *dispose)
{
struct file_lock *fl = *before;
int error = assign_type(fl, arg);
@@ -1311,7 +1311,7 @@ int lease_modify(struct file_lock **before, int arg)
printk(KERN_ERR "locks_delete_lock: fasync == %p\n", fl->fl_fasync);
fl->fl_fasync = NULL;
}
- locks_delete_lock(before, NULL);
+ locks_delete_lock(before, dispose);
}
return 0;
}
@@ -1325,7 +1325,7 @@ static bool past_time(unsigned long then)
return time_after(jiffies, then);
}
-static void time_out_leases(struct inode *inode)
+static void time_out_leases(struct inode *inode, struct list_head *dispose)
{
struct file_lock **before;
struct file_lock *fl;
@@ -1336,9 +1336,9 @@ static void time_out_leases(struct inode *inode)
while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) {
trace_time_out_leases(inode, fl);
if (past_time(fl->fl_downgrade_time))
- lease_modify(before, F_RDLCK);
+ lease_modify(before, F_RDLCK, dispose);
if (past_time(fl->fl_break_time))
- lease_modify(before, F_UNLCK);
+ lease_modify(before, F_UNLCK, dispose);
if (fl == *before) /* lease_modify may have freed fl */
before = &fl->fl_next;
}
@@ -1373,6 +1373,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
int i_have_this_lease = 0;
bool lease_conflict = false;
int want_write = (mode & O_ACCMODE) != O_RDONLY;
+ LIST_HEAD(dispose);
new_fl = lease_alloc(NULL, want_write ? F_WRLCK : F_RDLCK);
if (IS_ERR(new_fl))
@@ -1381,7 +1382,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
spin_lock(&inode->i_lock);
- time_out_leases(inode);
+ time_out_leases(inode, &dispose);
flock = inode->i_flock;
if ((flock == NULL) || !IS_LEASE(flock))
@@ -1436,6 +1437,7 @@ restart:
locks_insert_block(flock, new_fl);
trace_break_lease_block(inode, new_fl);
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
error = wait_event_interruptible_timeout(new_fl->fl_wait,
!new_fl->fl_next, break_time);
spin_lock(&inode->i_lock);
@@ -1443,7 +1445,7 @@ restart:
locks_delete_block(new_fl);
if (error >= 0) {
if (error == 0)
- time_out_leases(inode);
+ time_out_leases(inode, &dispose);
/*
* Wait for the next conflicting lease that has not been
* broken yet
@@ -1458,6 +1460,7 @@ restart:
out:
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
locks_free_lock(new_fl);
return error;
}
@@ -1522,9 +1525,10 @@ int fcntl_getlease(struct file *filp)
struct file_lock *fl;
struct inode *inode = file_inode(filp);
int type = F_UNLCK;
+ LIST_HEAD(dispose);
spin_lock(&inode->i_lock);
- time_out_leases(file_inode(filp));
+ time_out_leases(file_inode(filp), &dispose);
for (fl = file_inode(filp)->i_flock; fl && IS_LEASE(fl);
fl = fl->fl_next) {
if (fl->fl_file == filp) {
@@ -1533,6 +1537,7 @@ int fcntl_getlease(struct file *filp)
}
}
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
return type;
}
@@ -1570,6 +1575,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
struct inode *inode = dentry->d_inode;
bool is_deleg = (*flp)->fl_flags & FL_DELEG;
int error;
+ LIST_HEAD(dispose);
lease = *flp;
trace_generic_add_lease(inode, lease);
@@ -1593,7 +1599,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
}
spin_lock(&inode->i_lock);
- time_out_leases(inode);
+ time_out_leases(inode, &dispose);
error = check_conflicting_open(dentry, arg);
if (error)
goto out;
@@ -1630,7 +1636,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
if (my_before != NULL) {
lease = *my_before;
- error = lease->fl_lmops->lm_change(my_before, arg);
+ error = lease->fl_lmops->lm_change(my_before, arg, &dispose);
if (error)
goto out;
goto out_setup;
@@ -1660,6 +1666,7 @@ out_setup:
lease->fl_lmops->lm_setup(lease, priv);
out:
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
if (is_deleg)
mutex_unlock(&inode->i_mutex);
if (!error && !my_before)
@@ -1676,8 +1683,10 @@ static int generic_delete_lease(struct file *filp)
struct file_lock *fl, **before;
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
+ LIST_HEAD(dispose);
spin_lock(&inode->i_lock);
+ time_out_leases(inode, &dispose);
for (before = &inode->i_flock;
((fl = *before) != NULL) && IS_LEASE(fl);
before = &fl->fl_next) {
@@ -1686,8 +1695,9 @@ static int generic_delete_lease(struct file *filp)
}
trace_generic_delete_lease(inode, fl);
if (fl)
- error = fl->fl_lmops->lm_change(before, F_UNLCK);
+ error = fl->fl_lmops->lm_change(before, F_UNLCK, &dispose);
spin_unlock(&inode->i_lock);
+ locks_dispose_list(&dispose);
return error;
}
@@ -2375,7 +2385,7 @@ void locks_remove_file(struct file *filp)
while ((fl = *before) != NULL) {
if (fl->fl_file == filp) {
if (IS_LEASE(fl)) {
- lease_modify(before, F_UNLCK);
+ lease_modify(before, F_UNLCK, &dispose);
continue;
}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 86eebf13b3d0..44e04d6be676 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3423,11 +3423,11 @@ static void nfsd_break_deleg_cb(struct file_lock *fl)
spin_unlock(&fp->fi_lock);
}
-static
-int nfsd_change_deleg_cb(struct file_lock **onlist, int arg)
+static int
+nfsd_change_deleg_cb(struct file_lock **onlist, int arg, struct list_head *dispose)
{
if (arg & F_UNLCK)
- return lease_modify(onlist, arg);
+ return lease_modify(onlist, arg, dispose);
else
return -EAGAIN;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9a6d56154dd5..f419f718e447 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -873,7 +873,7 @@ struct lock_manager_operations {
void (*lm_notify)(struct file_lock *); /* unblock callback */
int (*lm_grant)(struct file_lock *, int);
void (*lm_break)(struct file_lock *);
- int (*lm_change)(struct file_lock **, int);
+ int (*lm_change)(struct file_lock **, int, struct list_head *);
void (*lm_setup)(struct file_lock *, void **);
};
@@ -985,7 +985,7 @@ extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int t
extern void lease_get_mtime(struct inode *, struct timespec *time);
extern int generic_setlease(struct file *, long, struct file_lock **, void **priv);
extern int vfs_setlease(struct file *, long, struct file_lock **, void **);
-extern int lease_modify(struct file_lock **, int);
+extern int lease_modify(struct file_lock **, int, struct list_head *);
#else /* !CONFIG_FILE_LOCKING */
static inline int fcntl_getlk(struct file *file, unsigned int cmd,
struct flock __user *user)
@@ -1112,7 +1112,8 @@ static inline int vfs_setlease(struct file *filp, long arg,
return -EINVAL;
}
-static inline int lease_modify(struct file_lock **before, int arg)
+static inline int lease_modify(struct file_lock **before, int arg,
+ struct list_head *dispose)
{
return -EINVAL;
}
--
1.9.3
The contents of an fl_owner_t have morphed a bit over the years, fix up
the comments to account for the changes.
Signed-off-by: Jeff Layton <[email protected]>
---
include/linux/fs.h | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ed4e1897099c..a51be79b7dc3 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -852,11 +852,19 @@ static inline struct file *get_file(struct file *f)
#define FILE_LOCK_DEFERRED 1
/*
- * The POSIX file lock owner is determined by
- * the "struct files_struct" in the thread group
- * (or NULL for no owner - BSD locks).
+ * A generic "file lock owner" value. This is set differently for different
+ * types of locks.
*
- * Lockd stuffs a "host" pointer into this.
+ * The POSIX file lock owner is determined by the "struct files_struct" in the
+ * thread group.
+ *
+ * Flock (BSD) locks, OFD locks and leases set the fl_owner to the
+ * file description pointer.
+ *
+ * lockd stuffs a "host" pointer into this
+ *
+ * nfsd stuffs a * nfs4_file pointer into this for leases, and a lockowner
+ * pointer into it for v4 locks.
*/
typedef void *fl_owner_t;
--
1.9.3
On Fri, 5 Sep 2014 17:40:58 -0400
"J. Bruce Fields" <[email protected]> wrote:
> On Thu, Sep 04, 2014 at 08:38:33AM -0400, Jeff Layton wrote:
> > Now that we don't need to pass in an actual lease pointer to
> > vfs_setlease on unlock, we can stop tracking a pointer to the lease in
> > the nfs4_file.
> >
> > Switch all of the places that check the fi_lease to check fi_deleg_file
> > instead. We always set that at the same time so it will have the same
> > semantics.
> >
> > Cc: J. Bruce Fields <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > Reviewed-by: Christoph Hellwig <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 10 ++++------
> > fs/nfsd/state.h | 1 -
> > 2 files changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 0cd252916e1a..d0a6e8e022a2 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -680,11 +680,10 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
> > {
> > lockdep_assert_held(&state_lock);
> >
> > - if (!fp->fi_lease)
> > + if (!fp->fi_deleg_file)
> > return;
> > if (atomic_dec_and_test(&fp->fi_delegees)) {
> > vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
> > - fp->fi_lease = NULL;
> > fput(fp->fi_deleg_file);
> > fp->fi_deleg_file = NULL;
> > }
>
> Note the code in my for-3.18 branch is different here due to a couple
> patches I've already taken from you.
>
> --b.
>
Doh! Yes, I'll make sure to pull those into the topic branch where I'm
working on these and rebase on top of them. Thanks!
> > @@ -3061,8 +3060,8 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct knfsd_fh *fh)
> > INIT_LIST_HEAD(&fp->fi_stateids);
> > INIT_LIST_HEAD(&fp->fi_delegations);
> > fh_copy_shallow(&fp->fi_fhandle, fh);
> > + fp->fi_deleg_file = NULL;
> > fp->fi_had_conflict = false;
> > - fp->fi_lease = NULL;
> > fp->fi_share_deny = 0;
> > memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
> > memset(fp->fi_access, 0, sizeof(fp->fi_access));
> > @@ -3803,13 +3802,12 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
> > if (fp->fi_had_conflict)
> > goto out_unlock;
> > /* Race breaker */
> > - if (fp->fi_lease) {
> > + if (fp->fi_deleg_file) {
> > status = 0;
> > atomic_inc(&fp->fi_delegees);
> > hash_delegation_locked(dp, fp);
> > goto out_unlock;
> > }
> > - fp->fi_lease = fl;
> > fp->fi_deleg_file = filp;
> > atomic_set(&fp->fi_delegees, 1);
> > hash_delegation_locked(dp, fp);
> > @@ -3842,7 +3840,7 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
> > spin_lock(&state_lock);
> > spin_lock(&fp->fi_lock);
> > dp->dl_stid.sc_file = fp;
> > - if (!fp->fi_lease) {
> > + if (!fp->fi_deleg_file) {
> > spin_unlock(&fp->fi_lock);
> > spin_unlock(&state_lock);
> > status = nfs4_setlease(dp);
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index 4a89e00d7461..64f291a25a8c 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -477,7 +477,6 @@ struct nfs4_file {
> > atomic_t fi_access[2];
> > u32 fi_share_deny;
> > struct file *fi_deleg_file;
> > - struct file_lock *fi_lease;
> > atomic_t fi_delegees;
> > struct knfsd_fh fi_fhandle;
> > bool fi_had_conflict;
> > --
> > 1.9.3
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <[email protected]>
On Thu, Sep 04, 2014 at 08:38:39AM -0400, Jeff Layton wrote:
> I think that the intent of this code was to ensure that a process won't
> deadlock if it has one fd open with a lease on it and then breaks that
> lease by opening another fd. In that case it'll treat the __break_lease
> call as if it were non-blocking.
>
> This seems wrong -- the process could (for instance) be multithreaded
> and managing different fds via different threads. I also don't see any
> mention of this limitation in the (somewhat sketchy) documentation.
>
> Remove the check and the non-blocking behavior when i_have_this_lease
> is true.
This looks reasonable to me, but I'm always very worried about changing
userspace exposed behavior..
Now that we don't need to pass in an actual lease pointer to
vfs_setlease on unlock, we can stop tracking a pointer to the lease in
the nfs4_file.
Switch all of the places that check the fi_lease to check fi_deleg_file
instead. We always set that at the same time so it will have the same
semantics.
Cc: J. Bruce Fields <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/nfsd/nfs4state.c | 10 ++++------
fs/nfsd/state.h | 1 -
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0cd252916e1a..d0a6e8e022a2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -680,11 +680,10 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
{
lockdep_assert_held(&state_lock);
- if (!fp->fi_lease)
+ if (!fp->fi_deleg_file)
return;
if (atomic_dec_and_test(&fp->fi_delegees)) {
vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
- fp->fi_lease = NULL;
fput(fp->fi_deleg_file);
fp->fi_deleg_file = NULL;
}
@@ -3061,8 +3060,8 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct knfsd_fh *fh)
INIT_LIST_HEAD(&fp->fi_stateids);
INIT_LIST_HEAD(&fp->fi_delegations);
fh_copy_shallow(&fp->fi_fhandle, fh);
+ fp->fi_deleg_file = NULL;
fp->fi_had_conflict = false;
- fp->fi_lease = NULL;
fp->fi_share_deny = 0;
memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
memset(fp->fi_access, 0, sizeof(fp->fi_access));
@@ -3803,13 +3802,12 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
if (fp->fi_had_conflict)
goto out_unlock;
/* Race breaker */
- if (fp->fi_lease) {
+ if (fp->fi_deleg_file) {
status = 0;
atomic_inc(&fp->fi_delegees);
hash_delegation_locked(dp, fp);
goto out_unlock;
}
- fp->fi_lease = fl;
fp->fi_deleg_file = filp;
atomic_set(&fp->fi_delegees, 1);
hash_delegation_locked(dp, fp);
@@ -3842,7 +3840,7 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
spin_lock(&state_lock);
spin_lock(&fp->fi_lock);
dp->dl_stid.sc_file = fp;
- if (!fp->fi_lease) {
+ if (!fp->fi_deleg_file) {
spin_unlock(&fp->fi_lock);
spin_unlock(&state_lock);
status = nfs4_setlease(dp);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 4a89e00d7461..64f291a25a8c 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -477,7 +477,6 @@ struct nfs4_file {
atomic_t fi_access[2];
u32 fi_share_deny;
struct file *fi_deleg_file;
- struct file_lock *fi_lease;
atomic_t fi_delegees;
struct knfsd_fh fi_fhandle;
bool fi_had_conflict;
--
1.9.3
On Thu, 4 Sep 2014 10:50:14 -0700
Christoph Hellwig <[email protected]> wrote:
> On Thu, Sep 04, 2014 at 08:38:37AM -0400, Jeff Layton wrote:
> > There was only one place where we still could free a file_lock while
> > holding the i_lock -- lease_modify. Add a new list_head argument to the
> > lm_change operation, pass in a private list when calling it, and fix
> > those callers to dispose of the list once the lock has been dropped.
>
> As mentioned I don't see a real need for this, but it does look correct
> to me.
>
> Reviewed-by: Christoph Hellwig <[email protected]>
Yeah, it's not strictly necessary, but I think it simplifies the API
for potential users. We already have the infrastructure to handle
deferring file_lock removal so we might as well take advantage of it
here too.
--
Jeff Layton <[email protected]>
On Thu, Sep 04, 2014 at 08:38:33AM -0400, Jeff Layton wrote:
> Now that we don't need to pass in an actual lease pointer to
> vfs_setlease on unlock, we can stop tracking a pointer to the lease in
> the nfs4_file.
>
> Switch all of the places that check the fi_lease to check fi_deleg_file
> instead. We always set that at the same time so it will have the same
> semantics.
>
> Cc: J. Bruce Fields <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 10 ++++------
> fs/nfsd/state.h | 1 -
> 2 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0cd252916e1a..d0a6e8e022a2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -680,11 +680,10 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
> {
> lockdep_assert_held(&state_lock);
>
> - if (!fp->fi_lease)
> + if (!fp->fi_deleg_file)
> return;
> if (atomic_dec_and_test(&fp->fi_delegees)) {
> vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL);
> - fp->fi_lease = NULL;
> fput(fp->fi_deleg_file);
> fp->fi_deleg_file = NULL;
> }
Note the code in my for-3.18 branch is different here due to a couple
patches I've already taken from you.
--b.
> @@ -3061,8 +3060,8 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct knfsd_fh *fh)
> INIT_LIST_HEAD(&fp->fi_stateids);
> INIT_LIST_HEAD(&fp->fi_delegations);
> fh_copy_shallow(&fp->fi_fhandle, fh);
> + fp->fi_deleg_file = NULL;
> fp->fi_had_conflict = false;
> - fp->fi_lease = NULL;
> fp->fi_share_deny = 0;
> memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
> memset(fp->fi_access, 0, sizeof(fp->fi_access));
> @@ -3803,13 +3802,12 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
> if (fp->fi_had_conflict)
> goto out_unlock;
> /* Race breaker */
> - if (fp->fi_lease) {
> + if (fp->fi_deleg_file) {
> status = 0;
> atomic_inc(&fp->fi_delegees);
> hash_delegation_locked(dp, fp);
> goto out_unlock;
> }
> - fp->fi_lease = fl;
> fp->fi_deleg_file = filp;
> atomic_set(&fp->fi_delegees, 1);
> hash_delegation_locked(dp, fp);
> @@ -3842,7 +3840,7 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
> spin_lock(&state_lock);
> spin_lock(&fp->fi_lock);
> dp->dl_stid.sc_file = fp;
> - if (!fp->fi_lease) {
> + if (!fp->fi_deleg_file) {
> spin_unlock(&fp->fi_lock);
> spin_unlock(&state_lock);
> status = nfs4_setlease(dp);
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 4a89e00d7461..64f291a25a8c 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -477,7 +477,6 @@ struct nfs4_file {
> atomic_t fi_access[2];
> u32 fi_share_deny;
> struct file *fi_deleg_file;
> - struct file_lock *fi_lease;
> atomic_t fi_delegees;
> struct knfsd_fh fi_fhandle;
> bool fi_had_conflict;
> --
> 1.9.3
>
Some of the latter paragraphs seem ambiguous and just plain wrong.
In particular the break_lease comment makes no sense. We call
break_lease (and break_deleg) from all sorts of vfs-layer functions,
so there is clearly such a method.
Also get rid of some of the other comments about what's needed for
a full implementation.
Signed-off-by: Jeff Layton <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/locks.c | 34 ++++++++++------------------------
1 file changed, 10 insertions(+), 24 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 1289b74fffbf..81a4628f5594 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1708,30 +1708,16 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
}
/**
- * vfs_setlease - sets a lease on an open file
- * @filp: file pointer
- * @arg: type of lease to obtain
- * @lease: file_lock to use
- *
- * Call this to establish a lease on the file.
- * The (*lease)->fl_lmops->lm_break operation must be set; if not,
- * break_lease will oops!
- *
- * This will call the filesystem's setlease file method, if
- * defined. Note that there is no getlease method; instead, the
- * filesystem setlease method should call back to setlease() to
- * add a lease to the inode's lease list, where fcntl_getlease() can
- * find it. Since fcntl_getlease() only reports whether the current
- * task holds a lease, a cluster filesystem need only do this for
- * leases held by processes on this node.
- *
- * There is also no break_lease method; filesystems that
- * handle their own leases should break leases themselves from the
- * filesystem's open, create, and (on truncate) setattr methods.
- *
- * Warning: the only current setlease methods exist only to disable
- * leases in certain cases. More vfs changes may be required to
- * allow a full filesystem lease implementation.
+ * vfs_setlease - sets a lease on an open file
+ * @filp: file pointer
+ * @arg: type of lease to obtain
+ * @lease: file_lock to use when adding a lease
+ *
+ * Call this to establish a lease on the file. The "lease" argument is not
+ * used for F_UNLCK requests and may be NULL. For commands that set or alter
+ * an existing lease, the (*lease)->fl_lmops->lm_break operation must be set;
+ * if not, this function will return -ENOLCK (and generate a scary-looking
+ * stack trace).
*/
int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
--
1.9.3
On Thu, Sep 04, 2014 at 08:38:35AM -0400, Jeff Layton wrote:
> ...and move the fasync setup into it for fcntl lease calls. At the same
> time, change the semantics of how the file_lock double-pointer is
> handled. Up until now, on a successful lease return you got a pointer to
> the lock on the list. This is bad, since that pointer can no longer be
> relied on as valid once the inode->i_lock has been released.
>
> Change the code to instead just zero out the pointer if the lease we
> passed in ended up being used. Then the callers can just check to see
> if it's NULL after the call and free it if it isn't.
>
> The priv argument has the same semantics. The lm_setup function can
> zero the pointer out to signal to the caller that it should not be
> freed after the function returns.
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
security_file_set_fowner always returns 0, so make it f_setown and
__f_setown void return functions and fix up the error handling in the
callers.
Cc: [email protected]
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
drivers/net/tun.c | 4 +---
drivers/tty/tty_io.c | 3 ++-
fs/fcntl.c | 21 +++++++--------------
fs/locks.c | 2 +-
fs/notify/dnotify/dnotify.c | 8 +-------
include/linux/fs.h | 4 ++--
include/linux/security.h | 8 ++++----
net/socket.c | 3 ++-
security/capability.c | 4 ++--
security/security.c | 2 +-
security/selinux/hooks.c | 4 +---
security/smack/smack_lsm.c | 3 +--
12 files changed, 25 insertions(+), 41 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index acaaf6784179..186ce541c657 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2152,9 +2152,7 @@ static int tun_chr_fasync(int fd, struct file *file, int on)
goto out;
if (on) {
- ret = __f_setown(file, task_pid(current), PIDTYPE_PID, 0);
- if (ret)
- goto out;
+ __f_setown(file, task_pid(current), PIDTYPE_PID, 0);
tfile->flags |= TUN_FASYNC;
} else
tfile->flags &= ~TUN_FASYNC;
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 8fbad3410c75..aea3b66f7bf2 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2163,8 +2163,9 @@ static int __tty_fasync(int fd, struct file *filp, int on)
}
get_pid(pid);
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
- retval = __f_setown(filp, pid, type, 0);
+ __f_setown(filp, pid, type, 0);
put_pid(pid);
+ retval = 0;
}
out:
return retval;
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 22d1c3df61ac..99d440a4a6ba 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -98,26 +98,19 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
write_unlock_irq(&filp->f_owner.lock);
}
-int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
+void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
int force)
{
- int err;
-
- err = security_file_set_fowner(filp);
- if (err)
- return err;
-
+ security_file_set_fowner(filp);
f_modown(filp, pid, type, force);
- return 0;
}
EXPORT_SYMBOL(__f_setown);
-int f_setown(struct file *filp, unsigned long arg, int force)
+void f_setown(struct file *filp, unsigned long arg, int force)
{
enum pid_type type;
struct pid *pid;
int who = arg;
- int result;
type = PIDTYPE_PID;
if (who < 0) {
type = PIDTYPE_PGID;
@@ -125,9 +118,8 @@ int f_setown(struct file *filp, unsigned long arg, int force)
}
rcu_read_lock();
pid = find_vpid(who);
- result = __f_setown(filp, pid, type, force);
+ __f_setown(filp, pid, type, force);
rcu_read_unlock();
- return result;
}
EXPORT_SYMBOL(f_setown);
@@ -181,7 +173,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg)
if (owner.pid && !pid)
ret = -ESRCH;
else
- ret = __f_setown(filp, pid, type, 1);
+ __f_setown(filp, pid, type, 1);
rcu_read_unlock();
return ret;
@@ -302,7 +294,8 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
force_successful_syscall_return();
break;
case F_SETOWN:
- err = f_setown(filp, arg, 1);
+ f_setown(filp, arg, 1);
+ err = 0;
break;
case F_GETOWN_EX:
err = f_getown_ex(filp, arg);
diff --git a/fs/locks.c b/fs/locks.c
index d7e15a256f8f..18e87f11a25f 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1776,7 +1776,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new))
new = NULL;
- error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
+ __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
out_unlock:
spin_unlock(&inode->i_lock);
if (fl)
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index abc8cbcfe90e..caaaf9dfe353 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -346,13 +346,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out;
}
- error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
- if (error) {
- /* if we added, we must shoot */
- if (dn_mark == new_dn_mark)
- destroy = 1;
- goto out;
- }
+ __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
error = attach_dn(dn, dn_mark, id, fd, filp, mask);
/* !error means that we attached the dn to the dn_mark, so don't free it */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 435e3d9ec5cf..96528f73dda4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1139,8 +1139,8 @@ extern void fasync_free(struct fasync_struct *);
/* can be called from interrupts */
extern void kill_fasync(struct fasync_struct **, int, int);
-extern int __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
-extern int f_setown(struct file *filp, unsigned long arg, int force);
+extern void __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
+extern void f_setown(struct file *filp, unsigned long arg, int force);
extern void f_delown(struct file *filp);
extern pid_t f_getown(struct file *filp);
extern int send_sigurg(struct fown_struct *fown);
diff --git a/include/linux/security.h b/include/linux/security.h
index 623f90e5f38d..b10e7af95d3b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1559,7 +1559,7 @@ struct security_operations {
int (*file_lock) (struct file *file, unsigned int cmd);
int (*file_fcntl) (struct file *file, unsigned int cmd,
unsigned long arg);
- int (*file_set_fowner) (struct file *file);
+ void (*file_set_fowner) (struct file *file);
int (*file_send_sigiotask) (struct task_struct *tsk,
struct fown_struct *fown, int sig);
int (*file_receive) (struct file *file);
@@ -1834,7 +1834,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
unsigned long prot);
int security_file_lock(struct file *file, unsigned int cmd);
int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
-int security_file_set_fowner(struct file *file);
+void security_file_set_fowner(struct file *file);
int security_file_send_sigiotask(struct task_struct *tsk,
struct fown_struct *fown, int sig);
int security_file_receive(struct file *file);
@@ -2312,9 +2312,9 @@ static inline int security_file_fcntl(struct file *file, unsigned int cmd,
return 0;
}
-static inline int security_file_set_fowner(struct file *file)
+static inline void security_file_set_fowner(struct file *file)
{
- return 0;
+ return;
}
static inline int security_file_send_sigiotask(struct task_struct *tsk,
diff --git a/net/socket.c b/net/socket.c
index 95ee7d8682e7..769c9671847e 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1069,7 +1069,8 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
err = -EFAULT;
if (get_user(pid, (int __user *)argp))
break;
- err = f_setown(sock->file, pid, 1);
+ f_setown(sock->file, pid, 1);
+ err = 0;
break;
case FIOGETOWN:
case SIOCGPGRP:
diff --git a/security/capability.c b/security/capability.c
index a74fde6a7468..d68c57a62bcf 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -343,9 +343,9 @@ static int cap_file_fcntl(struct file *file, unsigned int cmd,
return 0;
}
-static int cap_file_set_fowner(struct file *file)
+static void cap_file_set_fowner(struct file *file)
{
- return 0;
+ return;
}
static int cap_file_send_sigiotask(struct task_struct *tsk,
diff --git a/security/security.c b/security/security.c
index e41b1a8d7644..fd301c177d04 100644
--- a/security/security.c
+++ b/security/security.c
@@ -775,7 +775,7 @@ int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
return security_ops->file_fcntl(file, cmd, arg);
}
-int security_file_set_fowner(struct file *file)
+void security_file_set_fowner(struct file *file)
{
return security_ops->file_set_fowner(file);
}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b0e940497e23..ada0d0bf3463 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3346,14 +3346,12 @@ static int selinux_file_fcntl(struct file *file, unsigned int cmd,
return err;
}
-static int selinux_file_set_fowner(struct file *file)
+static void selinux_file_set_fowner(struct file *file)
{
struct file_security_struct *fsec;
fsec = file->f_security;
fsec->fown_sid = current_sid();
-
- return 0;
}
static int selinux_file_send_sigiotask(struct task_struct *tsk,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index e6ab307ce86e..69e5635d89e5 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1390,12 +1390,11 @@ static int smack_mmap_file(struct file *file,
* Returns 0
* Further research may be required on this one.
*/
-static int smack_file_set_fowner(struct file *file)
+static void smack_file_set_fowner(struct file *file)
{
struct smack_known *skp = smk_of_current();
file->f_security = skp->smk_known;
- return 0;
}
/**
--
1.9.3
Looks good, but I'd rather merge it into the patch that changes the
semantics.
Reviewed-by: Christoph Hellwig <[email protected]>
I think that the intent of this code was to ensure that a process won't
deadlock if it has one fd open with a lease on it and then breaks that
lease by opening another fd. In that case it'll treat the __break_lease
call as if it were non-blocking.
This seems wrong -- the process could (for instance) be multithreaded
and managing different fds via different threads. I also don't see any
mention of this limitation in the (somewhat sketchy) documentation.
Remove the check and the non-blocking behavior when i_have_this_lease
is true.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/locks.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index dc2e9e18f32d..011812490c92 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1370,7 +1370,6 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
struct file_lock *new_fl, *flock;
struct file_lock *fl;
unsigned long break_time;
- int i_have_this_lease = 0;
bool lease_conflict = false;
int want_write = (mode & O_ACCMODE) != O_RDONLY;
LIST_HEAD(dispose);
@@ -1391,8 +1390,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
if (leases_conflict(fl, new_fl)) {
lease_conflict = true;
- if (fl->fl_owner == current->files)
- i_have_this_lease = 1;
+ break;
}
}
if (!lease_conflict)
@@ -1422,7 +1420,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
fl->fl_lmops->lm_break(fl);
}
- if (i_have_this_lease || (mode & O_NONBLOCK)) {
+ if (mode & O_NONBLOCK) {
trace_break_lease_noblock(inode, new_fl);
error = -EWOULDBLOCK;
goto out;
--
1.9.3
Signed-off-by: Jeff Layton <[email protected]>
---
Documentation/filesystems/Locking | 11 ++++++-----
Documentation/filesystems/vfs.txt | 7 ++++---
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index f1997e9da61f..94d93b1f8b53 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -464,15 +464,12 @@ prototypes:
size_t, unsigned int);
ssize_t (*splice_read)(struct file *, loff_t *, struct pipe_inode_info *,
size_t, unsigned int);
- int (*setlease)(struct file *, long, struct file_lock **);
+ int (*setlease)(struct file *, long, struct file_lock **, void **);
long (*fallocate)(struct file *, int, loff_t, loff_t);
};
locking rules:
- All may block except for ->setlease.
- No VFS locks held on entry except for ->setlease.
-
-->setlease has the file_list_lock held and must not sleep.
+ All may block.
->llseek() locking has moved from llseek to the individual llseek
implementations. If your fs is not using generic_file_llseek, you
@@ -496,6 +493,10 @@ components. And there are other reasons why the current interface is a mess...
->read on directories probably must go away - we should just enforce -EISDIR
in sys_read() and friends.
+->setlease operations should call generic_setlease() before or after setting
+the lease within the individual filesystem to record the result of the
+operation
+
--------------------------- dquot_operations -------------------------------
prototypes:
int (*write_dquot) (struct dquot *);
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 61d65cc65c54..af9441f32a62 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -826,7 +826,7 @@ struct file_operations {
int (*flock) (struct file *, int, struct file_lock *);
ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, size_t, unsigned int);
ssize_t (*splice_read)(struct file *, struct pipe_inode_info *, size_t, unsigned int);
- int (*setlease)(struct file *, long arg, struct file_lock **);
+ int (*setlease)(struct file *, long arg, struct file_lock **, void **);
long (*fallocate)(struct file *, int mode, loff_t offset, loff_t len);
int (*show_fdinfo)(struct seq_file *m, struct file *f);
};
@@ -895,8 +895,9 @@ otherwise noted.
splice_read: called by the VFS to splice data from file to a pipe. This
method is used by the splice(2) system call
- setlease: called by the VFS to set or release a file lock lease.
- setlease has the file_lock_lock held and must not sleep.
+ setlease: called by the VFS to set or release a file lock lease. setlease
+ implementations should call generic_setlease to record or remove
+ the lease in the inode after setting it
fallocate: called by the VFS to preallocate blocks or punch a hole.
--
1.9.3
On Thu, 4 Sep 2014 11:07:25 -0700
Christoph Hellwig <[email protected]> wrote:
> On Thu, Sep 04, 2014 at 08:38:40AM -0400, Jeff Layton wrote:
> > Eliminate an unneeded "flock" variable. We can use "fl" as a loop cursor
> > everywhere. Add a any_leases_conflict helper function as well to
> > consolidate a bit of code.
>
> Looks good,
>
> Reviewed-by: Christoph Hellwig <[email protected]>
>
> One thing that came to mind after starring at this code for a while and
> then seeing your cleanup:
>
> the sleep/wake patterns in __break_lease seem highly suboptimal, as
> we always wait for the break time on the first least found, why
> don't we simply take the max of the lease break times, and wait for
> that?
>
> I guess the case of lots of read leases just isn't common enough to
> bother..
That would make more sense. I've no objection to changing it to do that
though it's probably outside the scope of this patchset.
--
Jeff Layton <[email protected]>
On Thu, Sep 4, 2014 at 8:38 AM, Jeff Layton <[email protected]> wrote:
> GFS2 and NFS have setlease routines that always just return -EINVAL.
> Turn that into a generic routine that can live in fs/libfs.c.
>
> Cc: Trond Myklebust <[email protected]>
> Cc: <[email protected]>
> Cc: Steven Whitehouse <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/gfs2/file.c | 22 +---------------------
> fs/libfs.c | 16 ++++++++++++++++
> fs/nfs/file.c | 13 +------------
> fs/nfs/internal.h | 1 -
> fs/nfs/nfs4file.c | 2 +-
> include/linux/fs.h | 1 +
> 6 files changed, 20 insertions(+), 35 deletions(-)
Acked-by: Trond Myklebust <[email protected]>
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
Eliminate an unneeded "flock" variable. We can use "fl" as a loop cursor
everywhere. Add a any_leases_conflict helper function as well to
consolidate a bit of code.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/locks.c | 49 +++++++++++++++++++++++++------------------------
1 file changed, 25 insertions(+), 24 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 011812490c92..246ba53650f7 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1351,6 +1351,20 @@ static bool leases_conflict(struct file_lock *lease, struct file_lock *breaker)
return locks_conflict(breaker, lease);
}
+static bool
+any_leases_conflict(struct inode *inode, struct file_lock *breaker)
+{
+ struct file_lock *fl;
+
+ lockdep_assert_held(&inode->i_lock);
+
+ for (fl = inode->i_flock ; fl && IS_LEASE(fl); fl = fl->fl_next) {
+ if (leases_conflict(fl, breaker))
+ return true;
+ }
+ return false;
+}
+
/**
* __break_lease - revoke all outstanding leases on file
* @inode: the inode of the file to return
@@ -1367,10 +1381,9 @@ static bool leases_conflict(struct file_lock *lease, struct file_lock *breaker)
int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
{
int error = 0;
- struct file_lock *new_fl, *flock;
+ struct file_lock *new_fl;
struct file_lock *fl;
unsigned long break_time;
- bool lease_conflict = false;
int want_write = (mode & O_ACCMODE) != O_RDONLY;
LIST_HEAD(dispose);
@@ -1383,17 +1396,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
time_out_leases(inode, &dispose);
- flock = inode->i_flock;
- if ((flock == NULL) || !IS_LEASE(flock))
- goto out;
-
- for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
- if (leases_conflict(fl, new_fl)) {
- lease_conflict = true;
- break;
- }
- }
- if (!lease_conflict)
+ if (!any_leases_conflict(inode, new_fl))
goto out;
break_time = 0;
@@ -1403,7 +1406,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
break_time++; /* so that 0 means no break time */
}
- for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
+ for (fl = inode->i_flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
if (!leases_conflict(fl, new_fl))
continue;
if (want_write) {
@@ -1412,7 +1415,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
fl->fl_flags |= FL_UNLOCK_PENDING;
fl->fl_break_time = break_time;
} else {
- if (lease_breaking(flock))
+ if (lease_breaking(inode->i_flock))
continue;
fl->fl_flags |= FL_DOWNGRADE_PENDING;
fl->fl_downgrade_time = break_time;
@@ -1427,12 +1430,12 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
}
restart:
- break_time = flock->fl_break_time;
+ break_time = inode->i_flock->fl_break_time;
if (break_time != 0)
break_time -= jiffies;
if (break_time == 0)
break_time++;
- locks_insert_block(flock, new_fl);
+ locks_insert_block(inode->i_flock, new_fl);
trace_break_lease_block(inode, new_fl);
spin_unlock(&inode->i_lock);
locks_dispose_list(&dispose);
@@ -1442,17 +1445,15 @@ restart:
trace_break_lease_unblock(inode, new_fl);
locks_delete_block(new_fl);
if (error >= 0) {
- if (error == 0)
- time_out_leases(inode, &dispose);
/*
* Wait for the next conflicting lease that has not been
* broken yet
*/
- for (flock = inode->i_flock; flock && IS_LEASE(flock);
- flock = flock->fl_next) {
- if (leases_conflict(new_fl, flock))
- goto restart;
- }
+ if (error == 0)
+ time_out_leases(inode, &dispose);
+ if (any_leases_conflict(inode, new_fl))
+ goto restart;
+
error = 0;
}
--
1.9.3
On Thu, Sep 04, 2014 at 08:38:27AM -0400, Jeff Layton wrote:
> GFS2 and NFS have setlease routines that always just return -EINVAL.
> Turn that into a generic routine that can live in fs/libfs.c.
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
Like flock locks, leases are owned by the file description. Now that the
i_have_this_lease check in __break_lease is gone, we don't actually use
the fl_owner for leases for anything. So, it's now safe to set this more
appropriately to the same value as the fl_file.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/locks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/locks.c b/fs/locks.c
index 5a97e7c4e7f2..735b8d3fa78c 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -465,7 +465,7 @@ static int lease_init(struct file *filp, long type, struct file_lock *fl)
if (assign_type(fl, type) != 0)
return -EINVAL;
- fl->fl_owner = current->files;
+ fl->fl_owner = filp;
fl->fl_pid = current->tgid;
fl->fl_file = filp;
--
1.9.3
Now that we have a saner internal API for managing leases, we no longer
need to mandate that the inode->i_lock be held over most of the lease
code. Push it down into generic_add_lease and generic_delete_lease.
Signed-off-by: Jeff Layton <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/locks.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index a5ed4a1f73ca..5bc61091263f 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1330,6 +1330,8 @@ static void time_out_leases(struct inode *inode)
struct file_lock **before;
struct file_lock *fl;
+ lockdep_assert_held(&inode->i_lock);
+
before = &inode->i_flock;
while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) {
trace_time_out_leases(inode, fl);
@@ -1590,6 +1592,8 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
return -EINVAL;
}
+ spin_lock(&inode->i_lock);
+ time_out_leases(inode);
error = check_conflicting_open(dentry, arg);
if (error)
goto out;
@@ -1655,6 +1659,7 @@ out_setup:
if (lease->fl_lmops->lm_setup)
lease->fl_lmops->lm_setup(lease, priv);
out:
+ spin_unlock(&inode->i_lock);
if (is_deleg)
mutex_unlock(&inode->i_mutex);
if (!error && !my_before)
@@ -1672,6 +1677,7 @@ static int generic_delete_lease(struct file *filp)
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
+ spin_lock(&inode->i_lock);
for (before = &inode->i_flock;
((fl = *before) != NULL) && IS_LEASE(fl);
before = &fl->fl_next) {
@@ -1681,6 +1687,7 @@ static int generic_delete_lease(struct file *filp)
trace_generic_delete_lease(inode, fl);
if (fl)
error = fl->fl_lmops->lm_change(before, F_UNLCK);
+ spin_unlock(&inode->i_lock);
return error;
}
@@ -1694,8 +1701,6 @@ static int generic_delete_lease(struct file *filp)
*
* The (input) flp->fl_lmops->lm_break function is required
* by break_lease().
- *
- * Called with inode->i_lock held.
*/
int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
void **priv)
@@ -1712,8 +1717,6 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
if (error)
return error;
- time_out_leases(inode);
-
switch (arg) {
case F_UNLCK:
return generic_delete_lease(filp);
@@ -1750,16 +1753,10 @@ EXPORT_SYMBOL(generic_setlease);
int
vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv)
{
- struct inode *inode = file_inode(filp);
- int error;
-
- spin_lock(&inode->i_lock);
if (filp->f_op->setlease)
- error = filp->f_op->setlease(filp, arg, lease, priv);
+ return filp->f_op->setlease(filp, arg, lease, priv);
else
- error = generic_setlease(filp, arg, lease, priv);
- spin_unlock(&inode->i_lock);
- return error;
+ return generic_setlease(filp, arg, lease, priv);
}
EXPORT_SYMBOL_GPL(vfs_setlease);
--
1.9.3
On Thu, 4 Sep 2014 13:12:00 -0700
Christoph Hellwig <[email protected]> wrote:
> On Thu, Sep 04, 2014 at 02:25:35PM -0400, Trond Myklebust wrote:
> > Actually, it looks as if when you compile with !CONFIG_FILE_LOCKING,
> > then fcntl_setlease() returns the value '0' (which would be
> > "success!"). The word "confusing" only begins to describe it all.
>
> That's incorrect for sure, we should agree on a single sensible code
> for:
>
> 1) !CONFIG_FILE_LOCKING
> 2) !lease_enable
> 3) filesystem doesn't support leases.
>
Agreed. I think -ENOLCK is really better than -EINVAL.
I usually take -EINVAL to mean "you sent me something bogus". Whereas
-ENOLCK just says "locking doesn't work". -ENOLCK seems closer to the
situation in all 3 cases above.
That said, this is a user-visible change. The main userland consumer of
leases (AFAIK) is samba, so I'll take a peek at that code and run it by
them before merging anything.
--
Jeff Layton <[email protected]>
On Thu, 4 Sep 2014 08:41:51 -0400
Trond Myklebust <[email protected]> wrote:
> On Thu, Sep 4, 2014 at 8:38 AM, Jeff Layton <[email protected]> wrote:
> > GFS2 and NFS have setlease routines that always just return -EINVAL.
> > Turn that into a generic routine that can live in fs/libfs.c.
> >
> > Cc: Trond Myklebust <[email protected]>
> > Cc: <[email protected]>
> > Cc: Steven Whitehouse <[email protected]>
> > Cc: <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/gfs2/file.c | 22 +---------------------
> > fs/libfs.c | 16 ++++++++++++++++
> > fs/nfs/file.c | 13 +------------
> > fs/nfs/internal.h | 1 -
> > fs/nfs/nfs4file.c | 2 +-
> > include/linux/fs.h | 1 +
> > 6 files changed, 20 insertions(+), 35 deletions(-)
>
> Acked-by: Trond Myklebust <[email protected]>
>
Thanks. While spinning this up, I did have a momentary pause to wonder
if -ENOLCK would be a better return value here.
It would make it easier to distinguish this from from "oops, I passed
in bogus arguments". For now, I'll leave it as -EINVAL, but it's
something to consider...
--
Jeff Layton <[email protected]>
...and move the fasync setup into it for fcntl lease calls. At the same
time, change the semantics of how the file_lock double-pointer is
handled. Up until now, on a successful lease return you got a pointer to
the lock on the list. This is bad, since that pointer can no longer be
relied on as valid once the inode->i_lock has been released.
Change the code to instead just zero out the pointer if the lease we
passed in ended up being used. Then the callers can just check to see
if it's NULL after the call and free it if it isn't.
The priv argument has the same semantics. The lm_setup function can
zero the pointer out to signal to the caller that it should not be
freed after the function returns.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/locks.c | 92 ++++++++++++++++++++++++++++-------------------------
fs/nfsd/nfs4state.c | 6 ++--
include/linux/fs.h | 1 +
3 files changed, 51 insertions(+), 48 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 7162a5836dba..a5ed4a1f73ca 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -432,9 +432,27 @@ static void lease_break_callback(struct file_lock *fl)
kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG);
}
+static void
+lease_setup(struct file_lock *fl, void **priv)
+{
+ struct file *filp = fl->fl_file;
+ struct fasync_struct *fa = *priv;
+
+ /*
+ * fasync_insert_entry() returns the old entry if any. If there was no
+ * old entry, then it used "priv" and inserted it into the fasync list.
+ * Clear the pointer to indicate that it shouldn't be freed.
+ */
+ if (!fasync_insert_entry(fa->fa_fd, filp, &fl->fl_fasync, fa))
+ *priv = NULL;
+
+ __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
+}
+
static const struct lock_manager_operations lease_manager_ops = {
.lm_break = lease_break_callback,
.lm_change = lease_modify,
+ .lm_setup = lease_setup,
};
/*
@@ -1607,10 +1625,11 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
}
if (my_before != NULL) {
+ lease = *my_before;
error = lease->fl_lmops->lm_change(my_before, arg);
- if (!error)
- *flp = *my_before;
- goto out;
+ if (error)
+ goto out;
+ goto out_setup;
}
error = -EINVAL;
@@ -1631,9 +1650,15 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
error = check_conflicting_open(dentry, arg);
if (error)
goto out_unlink;
+
+out_setup:
+ if (lease->fl_lmops->lm_setup)
+ lease->fl_lmops->lm_setup(lease, priv);
out:
if (is_deleg)
mutex_unlock(&inode->i_mutex);
+ if (!error && !my_before)
+ *flp = NULL;
return error;
out_unlink:
locks_unlink_lock(before);
@@ -1661,10 +1686,11 @@ static int generic_delete_lease(struct file *filp)
/**
* generic_setlease - sets a lease on an open file
- * @filp: file pointer
- * @arg: type of lease to obtain
- * @flp: input - file_lock to use, output - file_lock inserted
- * @priv: private data for lm_setup
+ * @filp: file pointer
+ * @arg: type of lease to obtain
+ * @flp: input - file_lock to use, output - file_lock inserted
+ * @priv: private data for lm_setup (may be NULL if lm_setup
+ * doesn't require it)
*
* The (input) flp->fl_lmops->lm_break function is required
* by break_lease().
@@ -1704,29 +1730,23 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
}
EXPORT_SYMBOL(generic_setlease);
-static int
-__vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv)
-{
- if (filp->f_op->setlease)
- return filp->f_op->setlease(filp, arg, lease, priv);
- else
- return generic_setlease(filp, arg, lease, priv);
-}
-
/**
* vfs_setlease - sets a lease on an open file
- * @filp: file pointer
- * @arg: type of lease to obtain
- * @lease: file_lock to use when adding a lease
- * @priv: private info for lm_setup when adding a lease
+ * @filp: file pointer
+ * @arg: type of lease to obtain
+ * @lease: file_lock to use when adding a lease
+ * @priv: private info for lm_setup when adding a lease (may be
+ * NULL if lm_setup doesn't require it)
*
* Call this to establish a lease on the file. The "lease" argument is not
* used for F_UNLCK requests and may be NULL. For commands that set or alter
* an existing lease, the (*lease)->fl_lmops->lm_break operation must be set;
* if not, this function will return -ENOLCK (and generate a scary-looking
* stack trace).
+ *
+ * The "priv" pointer is passed directly to the lm_setup function as-is. It
+ * may be NULL if the lm_setup operation doesn't require it.
*/
-
int
vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv)
{
@@ -1734,17 +1754,18 @@ vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv)
int error;
spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, lease, priv);
+ if (filp->f_op->setlease)
+ error = filp->f_op->setlease(filp, arg, lease, priv);
+ else
+ error = generic_setlease(filp, arg, lease, priv);
spin_unlock(&inode->i_lock);
-
return error;
}
EXPORT_SYMBOL_GPL(vfs_setlease);
static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
{
- struct file_lock *fl, *ret;
- struct inode *inode = file_inode(filp);
+ struct file_lock *fl;
struct fasync_struct *new;
int error;
@@ -1757,26 +1778,9 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
locks_free_lock(fl);
return -ENOMEM;
}
- ret = fl;
- spin_lock(&inode->i_lock);
- error = __vfs_setlease(filp, arg, &ret, NULL);
- if (error)
- goto out_unlock;
- if (ret == fl)
- fl = NULL;
+ new->fa_fd = fd;
- /*
- * fasync_insert_entry() returns the old entry if any.
- * If there was no old entry, then it used 'new' and
- * inserted it into the fasync list. Clear new so that
- * we don't release it here.
- */
- if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new))
- new = NULL;
-
- __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
-out_unlock:
- spin_unlock(&inode->i_lock);
+ error = vfs_setlease(filp, arg, &fl, (void **)&new);
if (fl)
locks_free_lock(fl);
if (new)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d964a41c9151..86eebf13b3d0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3789,12 +3789,10 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
fl->fl_file = filp;
ret = fl;
status = vfs_setlease(filp, fl->fl_type, &fl, NULL);
- if (status) {
+ if (fl)
locks_free_lock(fl);
+ if (status)
goto out_fput;
- }
- if (ret != fl)
- locks_free_lock(fl);
spin_lock(&state_lock);
spin_lock(&fp->fi_lock);
/* Did the lease get broken before we took the lock? */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f1870eb67b02..9a6d56154dd5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -874,6 +874,7 @@ struct lock_manager_operations {
int (*lm_grant)(struct file_lock *, int);
void (*lm_break)(struct file_lock *);
int (*lm_change)(struct file_lock **, int);
+ void (*lm_setup)(struct file_lock *, void **);
};
struct lock_manager {
--
1.9.3
On Thu, 4 Sep 2014 13:14:24 -0700
Christoph Hellwig <[email protected]> wrote:
> On Thu, Sep 04, 2014 at 08:38:31AM -0400, Jeff Layton 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.
>
> Can we just make generic_delete_lease (maye renamed to vfs_delete_lease)
> the interface for deleting leases instead of going through a useless
> multiplex and file operation?
>
I'm not sure that change really makes sense to me at this point.
Suppose we have an exportable filesystem with a ->setlease
implementation [1]. We end up calling into it to set up a lease and it
calls generic_add_lease. If we make the change you're suggesting, then
we'll have no parallel to a ->setlease op when removing that lease.
We could of course make a ->dellease op or something, but I'd rather
not introduce that change until I've had a chance to do some other
cleanup to the file locking infrastructure.
So...I'm not opposed to doing what you suggest, but I'd rather not do it
just yet until I've gotten a little farther with some other cleanup of
how we deal with locks in general. I think it'll be easier to do that
once some other changes have gone in.
I'll post a draft patchset based on those changes "real soon now" as an
RFC. Hopefully at that point my rationale will make a bit more sense...
[1]: of course, only cifs has a non-trivial one for now and it's pretty
half-assed...
--
Jeff Layton <[email protected]>
On Thu, Sep 04, 2014 at 08:38:41AM -0400, Jeff Layton wrote:
> Christoph suggests:
>
> "Add a return value to lm_break so that the lock manager can tell the
> core code "you can delete this lease right now". That gets rid of
> the games with the timeout which require all kinds of race avoidance
> code in the users."
>
> Do that here and have the nfsd lease break routine use it when it detects
> that there was a race between setting up the lease and it being broken.
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Thu, Sep 04, 2014 at 02:25:35PM -0400, Trond Myklebust wrote:
> Actually, it looks as if when you compile with !CONFIG_FILE_LOCKING,
> then fcntl_setlease() returns the value '0' (which would be
> "success!"). The word "confusing" only begins to describe it all.
That's incorrect for sure, we should agree on a single sensible code
for:
1) !CONFIG_FILE_LOCKING
2) !lease_enable
3) filesystem doesn't support leases.
On Thu, Sep 04, 2014 at 08:38:37AM -0400, Jeff Layton wrote:
> There was only one place where we still could free a file_lock while
> holding the i_lock -- lease_modify. Add a new list_head argument to the
> lm_change operation, pass in a private list when calling it, and fix
> those callers to dispose of the list once the lock has been dropped.
As mentioned I don't see a real need for this, but it does look correct
to me.
Reviewed-by: Christoph Hellwig <[email protected]>
On Thu, Sep 04, 2014 at 08:38:40AM -0400, Jeff Layton wrote:
> Eliminate an unneeded "flock" variable. We can use "fl" as a loop cursor
> everywhere. Add a any_leases_conflict helper function as well to
> consolidate a bit of code.
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
One thing that came to mind after starring at this code for a while and
then seeing your cleanup:
the sleep/wake patterns in __break_lease seem highly suboptimal, as
we always wait for the break time on the first least found, why
don't we simply take the max of the lease break times, and wait for
that?
I guess the case of lots of read leases just isn't common enough to
bother..
It's unlikely to ever occur, but if there were already a lease set on
the file then we could end up getting back a different pointer on a
successful setlease attempt than the one we allocated. If that happens,
the one we allocated could leak.
In practice, I don't think this will happen due to the fact that we only
try to set up the lease once per nfs4_file, but this error handling is a
bit more correct given the current lease API.
Cc: J. Bruce Fields <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/nfsd/nfs4state.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fd5ff4b17292..29fac18d9102 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3774,7 +3774,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
static int nfs4_setlease(struct nfs4_delegation *dp)
{
struct nfs4_file *fp = dp->dl_stid.sc_file;
- struct file_lock *fl;
+ struct file_lock *fl, *ret;
struct file *filp;
int status = 0;
@@ -3788,11 +3788,14 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
return -EBADF;
}
fl->fl_file = filp;
- status = vfs_setlease(filp, fl->fl_type, &fl);
+ ret = fl;
+ status = vfs_setlease(filp, fl->fl_type, &ret);
if (status) {
locks_free_lock(fl);
goto out_fput;
}
+ if (ret != fl)
+ locks_free_lock(fl);
spin_lock(&state_lock);
spin_lock(&fp->fi_lock);
/* Did the lease get broken before we took the lock? */
--
1.9.3
On Thu, 4 Sep 2014 10:51:32 -0700
Christoph Hellwig <[email protected]> wrote:
> On Thu, Sep 04, 2014 at 08:38:39AM -0400, Jeff Layton wrote:
> > I think that the intent of this code was to ensure that a process won't
> > deadlock if it has one fd open with a lease on it and then breaks that
> > lease by opening another fd. In that case it'll treat the __break_lease
> > call as if it were non-blocking.
> >
> > This seems wrong -- the process could (for instance) be multithreaded
> > and managing different fds via different threads. I also don't see any
> > mention of this limitation in the (somewhat sketchy) documentation.
> >
> > Remove the check and the non-blocking behavior when i_have_this_lease
> > is true.
>
> This looks reasonable to me, but I'm always very worried about changing
> userspace exposed behavior..
>
Yeah, me too, but I think the behavior in this case is just plain
wrong. It's really hard to understand how anyone would rely on this to
avoid deadlocking, but you never know...
I want to phase this out, but I'm certainly open to doing this in a
smoother fashion if anyone has suggestions on how to do so.
Thanks,
--
Jeff Layton <[email protected]>
On Thu, Sep 4, 2014 at 8:49 AM, Jeff Layton <[email protected]> wrote:
> On Thu, 4 Sep 2014 08:41:51 -0400
> Trond Myklebust <[email protected]> wrote:
>
>> On Thu, Sep 4, 2014 at 8:38 AM, Jeff Layton <[email protected]> wrote:
>> > GFS2 and NFS have setlease routines that always just return -EINVAL.
>> > Turn that into a generic routine that can live in fs/libfs.c.
>> >
>> > Cc: Trond Myklebust <[email protected]>
>> > Cc: <[email protected]>
>> > Cc: Steven Whitehouse <[email protected]>
>> > Cc: <[email protected]>
>> > Signed-off-by: Jeff Layton <[email protected]>
>> > ---
>> > fs/gfs2/file.c | 22 +---------------------
>> > fs/libfs.c | 16 ++++++++++++++++
>> > fs/nfs/file.c | 13 +------------
>> > fs/nfs/internal.h | 1 -
>> > fs/nfs/nfs4file.c | 2 +-
>> > include/linux/fs.h | 1 +
>> > 6 files changed, 20 insertions(+), 35 deletions(-)
>>
>> Acked-by: Trond Myklebust <[email protected]>
>>
>
> Thanks. While spinning this up, I did have a momentary pause to wonder
> if -ENOLCK would be a better return value here.
>
> It would make it easier to distinguish this from from "oops, I passed
> in bogus arguments". For now, I'll leave it as -EINVAL, but it's
> something to consider...
>
Actually, it looks as if when you compile with !CONFIG_FILE_LOCKING,
then fcntl_setlease() returns the value '0' (which would be
"success!"). The word "confusing" only begins to describe it all.
Cheers
Trond
Christoph suggests:
"Add a return value to lm_break so that the lock manager can tell the
core code "you can delete this lease right now". That gets rid of
the games with the timeout which require all kinds of race avoidance
code in the users."
Do that here and have the nfsd lease break routine use it when it detects
that there was a race between setting up the lease and it being broken.
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/locks.c | 17 +++++++++++++----
fs/nfsd/nfs4state.c | 17 +++++++++--------
include/linux/fs.h | 2 +-
3 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 246ba53650f7..5a97e7c4e7f2 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -427,9 +427,11 @@ static int flock_to_posix_lock(struct file *filp, struct file_lock *fl,
}
/* default lease lock manager operations */
-static void lease_break_callback(struct file_lock *fl)
+static bool
+lease_break_callback(struct file_lock *fl)
{
kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG);
+ return false;
}
static void
@@ -1382,7 +1384,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
{
int error = 0;
struct file_lock *new_fl;
- struct file_lock *fl;
+ struct file_lock *fl, **before;
unsigned long break_time;
int want_write = (mode & O_ACCMODE) != O_RDONLY;
LIST_HEAD(dispose);
@@ -1406,7 +1408,9 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
break_time++; /* so that 0 means no break time */
}
- for (fl = inode->i_flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
+ for (before = &inode->i_flock;
+ ((fl = *before) != NULL) && IS_LEASE(fl);
+ before = &fl->fl_next) {
if (!leases_conflict(fl, new_fl))
continue;
if (want_write) {
@@ -1420,9 +1424,14 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
fl->fl_flags |= FL_DOWNGRADE_PENDING;
fl->fl_downgrade_time = break_time;
}
- fl->fl_lmops->lm_break(fl);
+ if (fl->fl_lmops->lm_break(fl))
+ locks_delete_lock(before, &dispose);
}
+ fl = inode->i_flock;
+ if (!fl || !IS_LEASE(fl))
+ goto out;
+
if (mode & O_NONBLOCK) {
trace_break_lease_noblock(inode, new_fl);
error = -EWOULDBLOCK;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 44e04d6be676..8597fbeea3bb 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3387,18 +3387,20 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
}
/* Called from break_lease() with i_lock held. */
-static void nfsd_break_deleg_cb(struct file_lock *fl)
+static bool
+nfsd_break_deleg_cb(struct file_lock *fl)
{
+ bool ret = false;
struct nfs4_file *fp = (struct nfs4_file *)fl->fl_owner;
struct nfs4_delegation *dp;
if (!fp) {
WARN(1, "(%p)->fl_owner NULL\n", fl);
- return;
+ return ret;
}
if (fp->fi_had_conflict) {
WARN(1, "duplicate break on %p\n", fp);
- return;
+ return ret;
}
/*
* We don't want the locks code to timeout the lease for us;
@@ -3410,17 +3412,16 @@ static void nfsd_break_deleg_cb(struct file_lock *fl)
spin_lock(&fp->fi_lock);
fp->fi_had_conflict = true;
/*
- * If there are no delegations on the list, then we can't count on this
- * lease ever being cleaned up. Set the fl_break_time to jiffies so that
- * time_out_leases will do it ASAP. The fact that fi_had_conflict is now
- * true should keep any new delegations from being hashed.
+ * If there are no delegations on the list, then return true
+ * so that the lease code will go ahead and delete it.
*/
if (list_empty(&fp->fi_delegations))
- fl->fl_break_time = jiffies;
+ ret = true;
else
list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
nfsd_break_one_deleg(dp);
spin_unlock(&fp->fi_lock);
+ return ret;
}
static int
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f419f718e447..ed4e1897099c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -872,7 +872,7 @@ struct lock_manager_operations {
void (*lm_put_owner)(struct file_lock *);
void (*lm_notify)(struct file_lock *); /* unblock callback */
int (*lm_grant)(struct file_lock *, int);
- void (*lm_break)(struct file_lock *);
+ bool (*lm_break)(struct file_lock *);
int (*lm_change)(struct file_lock **, int, struct list_head *);
void (*lm_setup)(struct file_lock *, void **);
};
--
1.9.3
lease_get_mtime is called without the i_lock held, so there's no
guarantee about the stability of the list. Between the time when we
assign "flock" and then dereference it to check whether it's a lease
and for write, the lease could be freed.
Ensure that that doesn't occur by taking the i_lock before trying
to check the lease.
Cc: J. Bruce Fields <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/locks.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 18e87f11a25f..4031324e6cca 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1456,8 +1456,18 @@ EXPORT_SYMBOL(__break_lease);
*/
void lease_get_mtime(struct inode *inode, struct timespec *time)
{
- struct file_lock *flock = inode->i_flock;
- if (flock && IS_LEASE(flock) && (flock->fl_type == F_WRLCK))
+ bool has_lease = false;
+ struct file_lock *flock;
+
+ if (inode->i_flock) {
+ spin_lock(&inode->i_lock);
+ flock = inode->i_flock;
+ if (flock && IS_LEASE(flock) && (flock->fl_type == F_WRLCK))
+ has_lease = true;
+ spin_unlock(&inode->i_lock);
+ }
+
+ if (has_lease)
*time = current_fs_time(inode->i_sb);
else
*time = inode->i_mtime;
--
1.9.3
On Thu, Sep 04, 2014 at 08:38:31AM -0400, Jeff Layton 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.
Can we just make generic_delete_lease (maye renamed to vfs_delete_lease)
the interface for deleting leases instead of going through a useless
multiplex and file operation?
On 7 October 2014 20:17, Christoph Hellwig <[email protected]> wrote:
> On Tue, Oct 07, 2014 at 08:11:42PM +0300, Dmitry Kasatkin wrote:
>> If file_set_fowner op is now type of "void", how you can actually
>> return the value?
>> I think compiler must give error. How could you compile it?
>
> Returning void values is a GNU extension. I've seen it in a few
> places in the kernel. Although in general I'd prefer it we'd remove
> it (and have the compiler warn about it).
>
Cool. There were patches around to use LLVM as well.
Will it work there?
--
Thanks,
Dmitry
On Tue, Oct 07, 2014 at 08:11:42PM +0300, Dmitry Kasatkin wrote:
> If file_set_fowner op is now type of "void", how you can actually
> return the value?
> I think compiler must give error. How could you compile it?
Returning void values is a GNU extension. I've seen it in a few
places in the kernel. Although in general I'd prefer it we'd remove
it (and have the compiler warn about it).
On Tue, 7 Oct 2014 20:34:57 +0300
Dmitry Kasatkin <[email protected]> wrote:
> On 7 October 2014 20:17, Christoph Hellwig <[email protected]> wrote:
> > On Tue, Oct 07, 2014 at 08:11:42PM +0300, Dmitry Kasatkin wrote:
> >> If file_set_fowner op is now type of "void", how you can actually
> >> return the value?
> >> I think compiler must give error. How could you compile it?
> >
> > Returning void values is a GNU extension. I've seen it in a few
> > places in the kernel. Although in general I'd prefer it we'd remove
> > it (and have the compiler warn about it).
> >
>
> Cool. There were patches around to use LLVM as well.
> Will it work there?
>
Well, no one has complained so far, and this has been in linux-next for
more than a month or so. I was just getting ready to send my pull
request to Linus, so you caught this just in time.
I'll respin this patch to not call "return" there and let it stew in
-next for a couple of days before I send the pull request.
Thanks,
--
Jeff Layton <[email protected]>
On 4 September 2014 15:38, Jeff Layton <[email protected]> wrote:
> security_file_set_fowner always returns 0, so make it f_setown and
> __f_setown void return functions and fix up the error handling in the
> callers.
>
> Cc: [email protected]
> Cc: Christoph Hellwig <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> drivers/net/tun.c | 4 +---
> drivers/tty/tty_io.c | 3 ++-
> fs/fcntl.c | 21 +++++++--------------
> fs/locks.c | 2 +-
> fs/notify/dnotify/dnotify.c | 8 +-------
> include/linux/fs.h | 4 ++--
> include/linux/security.h | 8 ++++----
> net/socket.c | 3 ++-
> security/capability.c | 4 ++--
> security/security.c | 2 +-
> security/selinux/hooks.c | 4 +---
> security/smack/smack_lsm.c | 3 +--
> 12 files changed, 25 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index acaaf6784179..186ce541c657 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2152,9 +2152,7 @@ static int tun_chr_fasync(int fd, struct file *file, int on)
> goto out;
>
> if (on) {
> - ret = __f_setown(file, task_pid(current), PIDTYPE_PID, 0);
> - if (ret)
> - goto out;
> + __f_setown(file, task_pid(current), PIDTYPE_PID, 0);
> tfile->flags |= TUN_FASYNC;
> } else
> tfile->flags &= ~TUN_FASYNC;
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 8fbad3410c75..aea3b66f7bf2 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2163,8 +2163,9 @@ static int __tty_fasync(int fd, struct file *filp, int on)
> }
> get_pid(pid);
> spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> - retval = __f_setown(filp, pid, type, 0);
> + __f_setown(filp, pid, type, 0);
> put_pid(pid);
> + retval = 0;
> }
> out:
> return retval;
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22d1c3df61ac..99d440a4a6ba 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -98,26 +98,19 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> write_unlock_irq(&filp->f_owner.lock);
> }
>
> -int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> +void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> int force)
> {
> - int err;
> -
> - err = security_file_set_fowner(filp);
> - if (err)
> - return err;
> -
> + security_file_set_fowner(filp);
> f_modown(filp, pid, type, force);
> - return 0;
> }
> EXPORT_SYMBOL(__f_setown);
>
> -int f_setown(struct file *filp, unsigned long arg, int force)
> +void f_setown(struct file *filp, unsigned long arg, int force)
> {
> enum pid_type type;
> struct pid *pid;
> int who = arg;
> - int result;
> type = PIDTYPE_PID;
> if (who < 0) {
> type = PIDTYPE_PGID;
> @@ -125,9 +118,8 @@ int f_setown(struct file *filp, unsigned long arg, int force)
> }
> rcu_read_lock();
> pid = find_vpid(who);
> - result = __f_setown(filp, pid, type, force);
> + __f_setown(filp, pid, type, force);
> rcu_read_unlock();
> - return result;
> }
> EXPORT_SYMBOL(f_setown);
>
> @@ -181,7 +173,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg)
> if (owner.pid && !pid)
> ret = -ESRCH;
> else
> - ret = __f_setown(filp, pid, type, 1);
> + __f_setown(filp, pid, type, 1);
> rcu_read_unlock();
>
> return ret;
> @@ -302,7 +294,8 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> force_successful_syscall_return();
> break;
> case F_SETOWN:
> - err = f_setown(filp, arg, 1);
> + f_setown(filp, arg, 1);
> + err = 0;
> break;
> case F_GETOWN_EX:
> err = f_getown_ex(filp, arg);
> diff --git a/fs/locks.c b/fs/locks.c
> index d7e15a256f8f..18e87f11a25f 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1776,7 +1776,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
> if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new))
> new = NULL;
>
> - error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
> + __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
> out_unlock:
> spin_unlock(&inode->i_lock);
> if (fl)
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index abc8cbcfe90e..caaaf9dfe353 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -346,13 +346,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out;
> }
>
> - error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
> - if (error) {
> - /* if we added, we must shoot */
> - if (dn_mark == new_dn_mark)
> - destroy = 1;
> - goto out;
> - }
> + __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
>
> error = attach_dn(dn, dn_mark, id, fd, filp, mask);
> /* !error means that we attached the dn to the dn_mark, so don't free it */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 435e3d9ec5cf..96528f73dda4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1139,8 +1139,8 @@ extern void fasync_free(struct fasync_struct *);
> /* can be called from interrupts */
> extern void kill_fasync(struct fasync_struct **, int, int);
>
> -extern int __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
> -extern int f_setown(struct file *filp, unsigned long arg, int force);
> +extern void __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
> +extern void f_setown(struct file *filp, unsigned long arg, int force);
> extern void f_delown(struct file *filp);
> extern pid_t f_getown(struct file *filp);
> extern int send_sigurg(struct fown_struct *fown);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 623f90e5f38d..b10e7af95d3b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1559,7 +1559,7 @@ struct security_operations {
> int (*file_lock) (struct file *file, unsigned int cmd);
> int (*file_fcntl) (struct file *file, unsigned int cmd,
> unsigned long arg);
> - int (*file_set_fowner) (struct file *file);
> + void (*file_set_fowner) (struct file *file);
> int (*file_send_sigiotask) (struct task_struct *tsk,
> struct fown_struct *fown, int sig);
> int (*file_receive) (struct file *file);
> @@ -1834,7 +1834,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> unsigned long prot);
> int security_file_lock(struct file *file, unsigned int cmd);
> int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
> -int security_file_set_fowner(struct file *file);
> +void security_file_set_fowner(struct file *file);
> int security_file_send_sigiotask(struct task_struct *tsk,
> struct fown_struct *fown, int sig);
> int security_file_receive(struct file *file);
> @@ -2312,9 +2312,9 @@ static inline int security_file_fcntl(struct file *file, unsigned int cmd,
> return 0;
> }
>
> -static inline int security_file_set_fowner(struct file *file)
> +static inline void security_file_set_fowner(struct file *file)
> {
> - return 0;
> + return;
> }
>
> static inline int security_file_send_sigiotask(struct task_struct *tsk,
> diff --git a/net/socket.c b/net/socket.c
> index 95ee7d8682e7..769c9671847e 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1069,7 +1069,8 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
> err = -EFAULT;
> if (get_user(pid, (int __user *)argp))
> break;
> - err = f_setown(sock->file, pid, 1);
> + f_setown(sock->file, pid, 1);
> + err = 0;
> break;
> case FIOGETOWN:
> case SIOCGPGRP:
> diff --git a/security/capability.c b/security/capability.c
> index a74fde6a7468..d68c57a62bcf 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -343,9 +343,9 @@ static int cap_file_fcntl(struct file *file, unsigned int cmd,
> return 0;
> }
>
> -static int cap_file_set_fowner(struct file *file)
> +static void cap_file_set_fowner(struct file *file)
> {
> - return 0;
> + return;
> }
>
> static int cap_file_send_sigiotask(struct task_struct *tsk,
> diff --git a/security/security.c b/security/security.c
> index e41b1a8d7644..fd301c177d04 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -775,7 +775,7 @@ int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> return security_ops->file_fcntl(file, cmd, arg);
> }
>
> -int security_file_set_fowner(struct file *file)
> +void security_file_set_fowner(struct file *file)
> {
> return security_ops->file_set_fowner(file);
If file_set_fowner op is now type of "void", how you can actually
return the value?
I think compiler must give error. How could you compile it?
- Dmitry
> }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b0e940497e23..ada0d0bf3463 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3346,14 +3346,12 @@ static int selinux_file_fcntl(struct file *file, unsigned int cmd,
> return err;
> }
>
> -static int selinux_file_set_fowner(struct file *file)
> +static void selinux_file_set_fowner(struct file *file)
> {
> struct file_security_struct *fsec;
>
> fsec = file->f_security;
> fsec->fown_sid = current_sid();
> -
> - return 0;
> }
>
> static int selinux_file_send_sigiotask(struct task_struct *tsk,
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e6ab307ce86e..69e5635d89e5 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1390,12 +1390,11 @@ static int smack_mmap_file(struct file *file,
> * Returns 0
> * Further research may be required on this one.
> */
> -static int smack_file_set_fowner(struct file *file)
> +static void smack_file_set_fowner(struct file *file)
> {
> struct smack_known *skp = smk_of_current();
>
> file->f_security = skp->smk_known;
> - return 0;
> }
>
> /**
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
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
On Tue, 13 Jan 2015 12:03:43 +1300
NeilBrown <[email protected]> wrote:
> 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
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 <[email protected]>
On Mon, 12 Jan 2015 18:25:00 -0500 Jeff Layton <[email protected]>
wrote:
> On Tue, 13 Jan 2015 12:03:43 +1300
> NeilBrown <[email protected]> wrote:
>
> > 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
>
> 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