lease/delegation/oplock semantics

From: J. Bruce Fields <[email protected]>

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 <[email protected]>

---
 fs/inode.c         |    1 +
 fs/locks.c         |   28 ++++++++++++++++++++++++++++
 fs/namei.c         |    6 +++++-
 include/linux/fs.h |    3 +++
 4 files changed, 37 insertions(+), 1 deletions(-)

fs/inode.c +1 -0

index 33c963d..4f253a2 100644
@@ -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;

fs/locks.c +28 -0

index 0a4f50d..7699b1b 100644
@@ -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)

fs/namei.c +5 -1

index 3cb616d..079e68c 100644
@@ -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) {

include/linux/fs.h +3 -0

index 1b95af3..daf8443 100644
@@ -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);

Quoted 5 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

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.

So unless the break_lease() call is moved from may_open() to after nameidata_to_filp(), I don't see any other options.

Mimi

Quoted 113 lines

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 <[email protected]>

---
 fs/inode.c         |    1 +
 fs/locks.c         |   28 ++++++++++++++++++++++++++++
 fs/namei.c         |    6 +++++-
 include/linux/fs.h |    3 +++
 4 files changed, 37 insertions(+), 1 deletions(-)

fs/inode.c +1 -0

index 33c963d..4f253a2 100644
@@ -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;

fs/locks.c +28 -0

index 0a4f50d..7699b1b 100644
@@ -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)

fs/namei.c +5 -1

index 3cb616d..079e68c 100644
@@ -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) {

include/linux/fs.h +3 -0

index 1b95af3..daf8443 100644
@@ -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);

Quoted 7 lines · Mimi Zohar

Quoted 5 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

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.

Quoted 2 lines

So unless the break_lease() call is moved from may_open() to after nameidata_to_filp(), I don't see any other options.

Actually, offhand I can't see why that wouldn't be OK.

Though I think we still end up needing something like i_blockleases to handle unlink, link, rename, chown, and chmod.

Quoted 11 lines · J. Bruce Fields

Quoted 9 lines · Mimi Zohar

Quoted 5 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

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.

So unless the break_lease() call is moved from may_open() to after nameidata_to_filp(), I don't see any other options.

Quoted 4 lines

Actually, offhand I can't see why that wouldn't be OK.

Though I think we still end up needing something like i_blockleases to handle unlink, link, rename, chown, and chmod.

Well, I guess there's a bizarre alternative that wouldn't require a new inode field:

What we care about is conflicts between read leases and operations that modify the metadata of the inode or the set of names pointing to it.

As far as I can tell those operations all take the i_mutex either on the inode itself or on the parents of one of its aliases.

So, you could prevent break_lease/setlease races by calling setlease under *all* of those i_mutexes:

- take i_mutex on the inode
- take i_lock to prevent the set of aliases from changing
- take i_mutex for parent of each alias
- set the lease
- drop the parent i_mutexes, etc.

where the i_mutexes would all be taken with mutex_trylock, and we'd just fail the whole setlease if any of them failed.

???

--b.

Quoted 17 lines · J. Bruce Fields

Quoted 15 lines · J. Bruce Fields

Quoted 9 lines · Mimi Zohar

Quoted 5 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

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.

So unless the break_lease() call is moved from may_open() to after nameidata_to_filp(), I don't see any other options.

Actually, offhand I can't see why that wouldn't be OK.

Though I think we still end up needing something like i_blockleases to handle unlink, link, rename, chown, and chmod.

Quoted 2 lines

Well, I guess there's a bizarre alternative that wouldn't require a new inode field:

In lieu of adding a new inode field, another possible option, a bit kludgy, would be extending i_flock with an additional fl_flag FL_BLOCKLEASE.

#define IS_BLOCKLEASE(fl) (fl->fl_flags & FL_BLOCKLEASE)

Mimi

Quoted 21 lines

What we care about is conflicts between read leases and operations that modify the metadata of the inode or the set of names pointing to it.

As far as I can tell those operations all take the i_mutex either on the inode itself or on the parents of one of its aliases.

So, you could prevent break_lease/setlease races by calling setlease under *all* of those i_mutexes:

- take i_mutex on the inode
- take i_lock to prevent the set of aliases from changing
- take i_mutex for parent of each alias
- set the lease
- drop the parent i_mutexes, etc.

where the i_mutexes would all be taken with mutex_trylock, and we'd just fail the whole setlease if any of them failed.

???

--b.

Quoted 25 lines · Mimi Zohar

Quoted 12 lines · J. Bruce Fields

Quoted 11 lines · J. Bruce Fields

Quoted 9 lines · Mimi Zohar

Quoted 5 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

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.

So unless the break_lease() call is moved from may_open() to after nameidata_to_filp(), I don't see any other options.

Quoted 7 lines

Quoted 4 lines

Actually, offhand I can't see why that wouldn't be OK.

Though I think we still end up needing something like i_blockleases to handle unlink, link, rename, chown, and chmod.

Well, I guess there's a bizarre alternative that wouldn't require a new inode field:

In lieu of adding a new inode field, another possible option, a bit kludgy, would be extending i_flock with an additional fl_flag FL_BLOCKLEASE.

Quoted 1 line

#define IS_BLOCKLEASE(fl) (fl->fl_flags & FL_BLOCKLEASE)

Alas, that would mean adding and removing one of these file locks around every single link, unlink, rename,....

--b.

Quoted 18 lines

Mimi

Quoted 15 lines

What we care about is conflicts between read leases and operations that modify the metadata of the inode or the set of names pointing to it.

As far as I can tell those operations all take the i_mutex either on the inode itself or on the parents of one of its aliases.

So, you could prevent break_lease/setlease races by calling setlease under *all* of those i_mutexes:

- take i_mutex on the inode
- take i_lock to prevent the set of aliases from changing
- take i_mutex for parent of each alias
- set the lease
- drop the parent i_mutexes, etc.

Quoted 8 lines

Quoted 6 lines

where the i_mutexes would all be taken with mutex_trylock, and we'd just fail the whole setlease if any of them failed.

???

--b.

Quoted 26 lines · J. Bruce Fields

Quoted 25 lines · Mimi Zohar

Quoted 12 lines · J. Bruce Fields

Quoted 11 lines · J. Bruce Fields

Quoted 9 lines · Mimi Zohar

Quoted 5 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

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.

So unless the break_lease() call is moved from may_open() to after nameidata_to_filp(), I don't see any other options.

Quoted 7 lines

Quoted 4 lines

Actually, offhand I can't see why that wouldn't be OK.

Though I think we still end up needing something like i_blockleases to handle unlink, link, rename, chown, and chmod.

Well, I guess there's a bizarre alternative that wouldn't require a new inode field:

In lieu of adding a new inode field, another possible option, a bit kludgy, would be extending i_flock with an additional fl_flag FL_BLOCKLEASE.

Quoted 6 lines

Quoted 1 line

#define IS_BLOCKLEASE(fl) (fl->fl_flags & FL_BLOCKLEASE)

Alas, that would mean adding and removing one of these file locks around every single link, unlink, rename,....

--b.

You're adding a call to break_lease() for each of them. Currently __break_lease() is only called if a lease exists. Assuming there aren't any existing leases, couldn't break_lease() call something like block_lease()? The free would be after the link, unlink, ..., completed/failed.

(You wouldn't actually need to alloc/free the 'struct file_lock' each time, just set the pointer and reset to NULL.)

Quoted 40 lines · Mimi Zohar

Quoted 26 lines · J. Bruce Fields

Quoted 25 lines · Mimi Zohar

Quoted 12 lines · J. Bruce Fields

Quoted 11 lines · J. Bruce Fields

Quoted 9 lines · Mimi Zohar

Quoted 5 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

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.

So unless the break_lease() call is moved from may_open() to after nameidata_to_filp(), I don't see any other options.

Quoted 7 lines

Quoted 4 lines

Actually, offhand I can't see why that wouldn't be OK.

Though I think we still end up needing something like i_blockleases to handle unlink, link, rename, chown, and chmod.

Well, I guess there's a bizarre alternative that wouldn't require a new inode field:

In lieu of adding a new inode field, another possible option, a bit kludgy, would be extending i_flock with an additional fl_flag FL_BLOCKLEASE.

Quoted 6 lines

Quoted 1 line

#define IS_BLOCKLEASE(fl) (fl->fl_flags & FL_BLOCKLEASE)

Alas, that would mean adding and removing one of these file locks around every single link, unlink, rename,....

--b.

You're adding a call to break_lease() for each of them. Currently __break_lease() is only called if a lease exists. Assuming there aren't any existing leases, couldn't break_lease() call something like block_lease()? The free would be after the link, unlink, ..., completed/failed.

Quoted 2 lines

(You wouldn't actually need to alloc/free the 'struct file_lock' each time, just set the pointer and reset to NULL.)

Well, the pointer has to be set to something. I suppose we could put a struct file_lock on the stack.

--b.

Quoted 23 lines · J. Bruce Fields

Quoted 21 lines · Mimi Zohar

Quoted 11 lines · J. Bruce Fields

Quoted 5 lines · Mimi Zohar

In lieu of adding a new inode field, another possible option, a bit kludgy, would be extending i_flock with an additional fl_flag FL_BLOCKLEASE.

#define IS_BLOCKLEASE(fl) (fl->fl_flags & FL_BLOCKLEASE)

Alas, that would mean adding and removing one of these file locks around every single link, unlink, rename,....

--b.

You're adding a call to break_lease() for each of them. Currently __break_lease() is only called if a lease exists. Assuming there aren't any existing leases, couldn't break_lease() call something like block_lease()? The free would be after the link, unlink, ..., completed/failed.

(You wouldn't actually need to alloc/free the 'struct file_lock' each time, just set the pointer and reset to NULL.)

Quoted 4 lines

Well, the pointer has to be set to something. I suppose we could put a struct file_lock on the stack.

--b.

Instead of putting the struct file_lock on the stack, how about creating a dummy list containing a single element with FL_BLOCKLEASE set?

Quoted 6 lines · Mimi Zohar

You're adding a call to break_lease() for each of them. Currently __break_lease() is only called if a lease exists. Assuming there aren't any existing leases, couldn't break_lease() call something like block_lease()? The free would be after the link, unlink, ..., completed/failed.

Quoted 2 lines

(You wouldn't actually need to alloc/free the 'struct file_lock' each time, just set the pointer and reset to NULL.)

Well, the pointer has to be set to something. I suppose we could put a struct file_lock on the stack.