fixing leases/delegations

fixing leases/delegations

[J. Bruce Fields]

NFSv4 (along with, from what I'm told, Samba) has a problem with read
leases, which is that they are currently only broken on write opens,
whereas they should really be broken on any operation that changes an
inode's metadata or any of the links pointing to it.

Also, to break leases in a non-racy way it's not sufficient to have a
single break_lease() call from the vfs code; we also need to prevent
anyone from acquiring a new lease while the operation is still in
progress.

So here's one attempt to deal with those problems, by adding to the
inode a counter which is incremented whenever we start such an operation
and decremented when we finish it.

I only handle unlink; we'd also need to do link, rename, chown, and
chmod, at least.

Comments? Better ideas?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/2] locks: introduce i_blockleases to close lease races

[J. Bruce Fields]

Since break_lease is called before i_writecount is incremented, there's
a window between the two where a setlease call would have no way to know
that an open is about to happen.

We fix this by adding a new inode field, i_blockleases, that is
incremented while a lease-breaking operation is in progress.

We will later reuse i_blockleases to enforce lease-breaking for rename,
unlink, etc.

Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/inode.c         |    1 +
 fs/locks.c         |   28 ++++++++++++++++++++++++++++
 fs/namei.c         |    6 +++++-
 include/linux/fs.h |    3 +++
 4 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 33c963d..4f253a2 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -198,6 +198,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	inode->i_uid = 0;
 	inode->i_gid = 0;
 	atomic_set(&inode->i_writecount, 0);
+	atomic_set(&inode->i_blockleases, 0);
 	inode->i_size = 0;
 	inode->i_blocks = 0;
 	inode->i_bytes = 0;
diff --git a/fs/locks.c b/fs/locks.c
index 0a4f50d..7699b1b 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1164,6 +1164,32 @@ static void time_out_leases(struct inode *inode)
 	}
 }
 
+/* Disallow all leases (read or write): */
+void disallow_leases(struct inode *inode, int flags)
+{
+	if (!inode)
+		return;
+	if (!S_ISREG(inode->i_mode))
+		return;
+	if ((flags & O_ACCMODE) == O_RDONLY)
+		return;
+	atomic_inc(&inode->i_blockleases);
+}
+EXPORT_SYMBOL_GPL(disallow_leases);
+
+void reallow_leases(struct inode *inode, int flags)
+{
+	if (!inode)
+		return;
+	if (!S_ISREG(inode->i_mode))
+		return;
+	if ((flags & O_ACCMODE) == O_RDONLY)
+		return;
+	BUG_ON(atomic_read(&inode->i_blockleases) <= 0);
+	atomic_dec(&inode->i_blockleases);
+}
+EXPORT_SYMBOL_GPL(reallow_leases);
+
 /**
  *	__break_lease	-	revoke all outstanding leases on file
  *	@inode: the inode of the file to return
@@ -1369,6 +1395,8 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 
 	if (arg != F_UNLCK) {
 		error = -EAGAIN;
+		if (atomic_read(&inode->i_blockleases))
+			goto out;
 		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
 			goto out;
 		if ((arg == F_WRLCK)
diff --git a/fs/namei.c b/fs/namei.c
index 3cb616d..079e68c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2277,10 +2277,14 @@ ok:
 		want_write = 1;
 	}
 common:
+	disallow_leases(nd->path.dentry->d_inode, open_flag);
 	error = may_open(&nd->path, acc_mode, open_flag);
-	if (error)
+	if (error) {
+		reallow_leases(nd->path.dentry->d_inode, open_flag);
 		goto exit;
+	}
 	filp = nameidata_to_filp(nd);
+	reallow_leases(nd->path.dentry->d_inode, open_flag);
 	if (!IS_ERR(filp)) {
 		error = ima_file_check(filp, op->acc_mode);
 		if (error) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1b95af3..daf8443 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -796,6 +796,7 @@ struct inode {
 #ifdef CONFIG_IMA
 	atomic_t		i_readcount; /* struct files open RO */
 #endif
+	atomic_t		i_blockleases; /* setlease fails when >0 */
 	atomic_t		i_writecount;
 #ifdef CONFIG_SECURITY
 	void			*i_security;
@@ -1161,6 +1162,8 @@ 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 lease_modify(struct file_lock **, int);
+extern void disallow_leases(struct inode *, int flags);
+extern void reallow_leases(struct inode *, int flags);
 extern int lock_may_read(struct inode *, loff_t start, unsigned long count);
 extern int lock_may_write(struct inode *, loff_t start, unsigned long count);
 extern void lock_flocks(void);
-- 
1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 2/2] locks: break lease on unlink

[J. Bruce Fields]

NFSv4 uses leases (delegations) to allow clients to do opens locally.
An open takes a component name, so to do a local open a client needs to
know that at least that last component name points at the same time as
long as they hold the delegation.  For that reason, the NFSv4 spec
requires that delegations be broken on unlink.

Waiting for a lease to be broken may mean waiting for an nfs client or a
user process to give it up, so doing that while holding the i_mutex
would risk deadlocks.  So instead we do a non-blocking lease break, then
drop the i_mutex and do a blocking lease break if necessary.

Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/namei.c    |   32 ++++++++++++++++++++++++++++----
 fs/nfsd/vfs.c |    6 +++++-
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 079e68c..4661469 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2799,6 +2799,7 @@ static long do_unlinkat(int dfd, const char __user *pathname)
 
 	nd.flags &= ~LOOKUP_PARENT;
 
+retry:
 	mutex_lock_nested(&nd.path.dentry->d_inode->i_mutex, I_MUTEX_PARENT);
 	dentry = lookup_hash(&nd);
 	error = PTR_ERR(dentry);
@@ -2806,15 +2807,27 @@ static long do_unlinkat(int dfd, const char __user *pathname)
 		/* Why not before? Because we want correct error value */
 		if (nd.last.name[nd.last.len])
 			goto slashes;
-		inode = dentry->d_inode;
-		if (inode)
-			ihold(inode);
+		if (inode && inode != dentry->d_inode) {
+			reallow_leases(inode, O_WRONLY);
+			iput(inode);
+			inode = NULL;
+		}
+		if (!inode) {
+			inode = dentry->d_inode;
+			if (inode)
+				ihold(inode);
+			disallow_leases(inode, O_WRONLY);
+		}
 		error = mnt_want_write(nd.path.mnt);
 		if (error)
 			goto exit2;
 		error = security_path_unlink(&nd.path, dentry);
 		if (error)
 			goto exit3;
+		if (inode)
+			error = break_lease(inode, O_WRONLY|O_NONBLOCK);
+		if (error)
+			goto exit3;
 		error = vfs_unlink(nd.path.dentry->d_inode, dentry);
 exit3:
 		mnt_drop_write(nd.path.mnt);
@@ -2822,8 +2835,19 @@ exit3:
 		dput(dentry);
 	}
 	mutex_unlock(&nd.path.dentry->d_inode->i_mutex);
-	if (inode)
+	if (inode) {
+		/* XXX: safe to use EWOULDBLOCK==EAGAIN to do this?: */
+		if (error == -EWOULDBLOCK) {
+			/*
+			 * Wait this time, then retry with leases left
+			 * disabled:
+			 */
+			break_lease(inode, O_WRONLY);
+			goto retry;
+		}
+		reallow_leases(inode, O_WRONLY);
 		iput(inode);	/* truncate the inode here */
+	}
 exit1:
 	path_put(&nd.path);
 	putname(name);
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index f4e056a..ef01b98 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1826,13 +1826,17 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 	if (host_err)
 		goto out_put;
 
+	disallow_leases(rdentry->d_inode, O_WRONLY);
 	host_err = nfsd_break_lease(rdentry->d_inode);
-	if (host_err)
+	if (host_err) {
+		reallow_leases(rdentry->d_inode, O_WRONLY);
 		goto out_drop_write;
+	}
 	if (type != S_IFDIR)
 		host_err = vfs_unlink(dirp, rdentry);
 	else
 		host_err = vfs_rmdir(dirp, rdentry);
+	reallow_leases(rdentry->d_inode, O_WRONLY);
 	if (!host_err)
 		host_err = commit_metadata(fhp);
 out_drop_write:
-- 
1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html