Wake up mandatory locks waiter on chmod (v2)

PATCH In review Pavel Emelyanov 5 participants 15 messages Sep 17, 2007 – Sep 25, 2007

Subsystems (3)

Documentation VFS File locking (flock() and fcntl()/lockf())

lore

Share & tools

lore

Pavel Emelyanov

author

2007-09-17 08:16 UTC #0 lore raw

PATCH 1/1 Wake up mandatory locks waiter on chmod (v2)

When the process is blocked on mandatory lock and someone changes the inode's permissions, so that the lock is no longer mandatory, nobody wakes up the blocked process, but probably should.

Switched to use mandatory_lock() static inline function.

Signed-off-by Pavel Emelyanov <[email protected]>

Cc J. Bruce Fields <[email protected]>

---

 fs/attr.c          |    9 +++++++--
 fs/locks.c         |   17 ++++++++++++++++-
 include/linux/fs.h |    1 +
 3 files changed, 24 insertions(+), 3 deletions(-)

fs/attr.c +7 -2

index ae58bd3..da643b0 100644
@@ -104,7 +104,7 @@ int notify_change(struct dentry * dentry
 {
 	struct inode *inode = dentry->d_inode;
 	mode_t mode;
-	int error;
+	int error, mandatory;
 	struct timespec now;
 	unsigned int ia_valid = attr->ia_valid;

@@ -151,6 +151,8 @@ int notify_change(struct dentry * dentry
 	if (ia_valid & ATTR_SIZE)
 		down_write(&dentry->d_inode->i_alloc_sem);
 
+	mandatory = (inode->i_flock && mandatory_lock(inode));
+
 	if (inode->i_op && inode->i_op->setattr) {
 		error = security_inode_setattr(dentry, attr);
 		if (!error)

@@ -171,8 +173,11 @@ int notify_change(struct dentry * dentry
 	if (ia_valid & ATTR_SIZE)
 		up_write(&dentry->d_inode->i_alloc_sem);
 
-	if (!error)
+	if (!error) {
 		fsnotify_change(dentry, ia_valid);
+		if (mandatory)
+			locks_wakeup_mandatory(inode);
+	}
 
 	return error;
 }

fs/locks.c +16 -1

index a71c589..6481fd9 100644
@@ -1110,7 +1110,8 @@ int locks_mandatory_area(int read_write,
 			break;
 		if (!(fl.fl_flags & FL_SLEEP))
 			break;
-		error = wait_event_interruptible(fl.fl_wait, !fl.fl_next);
+		error = wait_event_interruptible(fl.fl_wait,
+				!fl.fl_next || !__mandatory_lock(inode));
 		if (!error) {
 			/*
 			 * If we've been sleeping someone might have

@@ -1129,6 +1130,20 @@ int locks_mandatory_area(int read_write,
 
 EXPORT_SYMBOL(locks_mandatory_area);
 
+void locks_wakeup_mandatory(struct inode *inode)
+{
+	struct file_lock *fl, **before;
+
+	lock_kernel();
+	for_each_lock(inode, before) {
+		fl = *before;
+
+		if (IS_POSIX(fl))
+			locks_wake_up_blocks(fl);
+	}
+	unlock_kernel();
+}
+
 /* We already had a lease on this file; just change its type */
 int lease_modify(struct file_lock **before, int arg)
 {

include/linux/fs.h +1 -0

index 9c519e6..215eea3 100644
@@ -1483,6 +1483,7 @@ extern struct kset fs_subsys;
 
 extern int locks_mandatory_locked(struct inode *);
 extern int locks_mandatory_area(int, struct inode *, struct file *, loff_t, size_t);
+extern void locks_wakeup_mandatory(struct inode *);
 
 /*
  * Candidates for mandatory locking have the setgid bit set

Trond Myklebust

2007-09-17 13:55 UTC #1 lore raw

Quoted 3 lines · Pavel Emelyanov

When the process is blocked on mandatory lock and someone changes the inode's permissions, so that the lock is no longer mandatory, nobody wakes up the blocked process, but probably should.

Please explain in more detail why we need this patch.

I don't see why changing a file from taking mandatory locks to advisory locks is really a useful operation that we need to support. For one thing, we don't support changing a file from using advisory locking to mandatory locking on-the-fly. Secondly, changing the locking type certainly isn't a documented operation and quite frankly, it doesn't even appear to make sense: if the file needs mandatory locking, then that means that you have a need to protect against some untrusted application that isn't following the locking rules. Then suddenly, you declare that you will trust that application after all???

Cheers,
Trond

Pavel Emelyanov

author

2007-09-17 14:19 UTC #2 lore raw

Quoted 8 lines · Trond Myklebust

Quoted 3 lines · Pavel Emelyanov
When the process is blocked on mandatory lock and someone changes the inode's permissions, so that the lock is no longer mandatory, nobody wakes up the blocked process, but probably should.
Please explain in more detail why we need this patch.
From "this fixes an OOPs/deadlock/leak" POV we do not. This is

just an attempt to make the locking code be more consistent and clean.

On the other hand, as far as I see from the original comment the code author expected the permissions to change and tried to handle this case. In this particular code there's a BUG - task will not be woken up. So this is a fix for this place. If we don't want to see tasks blocked on no-longer-mandatory locks, then we should remove the original check and comment.

Quoted 4 lines

I don't see why changing a file from taking mandatory locks to advisory locks is really a useful operation that we need to support. For one thing, we don't support changing a file from using advisory locking to mandatory locking on-the-fly. Secondly, changing the locking type

Actually we have some code trying to do it - no mandatory locking is allowed for already opened file.

Quoted 5 lines

certainly isn't a documented operation and quite frankly, it doesn't even appear to make sense: if the file needs mandatory locking, then that means that you have a need to protect against some untrusted application that isn't following the locking rules. Then suddenly, you declare that you will trust that application after all???

Not trust, but keep things look as they are described in docs. I.e. two conditions "inode is marked as mandlock" and "task cannot proceed with opening mand.lock-ed file" must go in pair.

Quoted 2 lines

Cheers,
Trond

Thanks,
Pavel

Trond Myklebust

2007-09-17 16:01 UTC #3 lore raw

Quoted 11 lines · Pavel Emelyanov

Quoted 6 lines · Trond Myklebust
Quoted 3 lines · Pavel Emelyanov
When the process is blocked on mandatory lock and someone changes the inode's permissions, so that the lock is no longer mandatory, nobody wakes up the blocked process, but probably should.
Please explain in more detail why we need this patch.
From "this fixes an OOPs/deadlock/leak" POV we do not. This is just an attempt to make the locking code be more consistent and clean.

Why do you think we get a deadlock or leak? AFAICS if the user turns off mandatory locks on the file, then the existing locks default back into advisory locks which use the same notification mechanism as the mandatory locks.

IOW: the process that is waiting in locks_mandatory_area() will be released as soon as the advisory lock is dropped. If that theory is broken in practice, then that is the bug that we need to fix. We neither want to add a load of locking crap to notify_change(), nor should we need to.

Cheers
Trond

Pavel Emelyanov

author

2007-09-18 06:37 UTC #4 lore raw

Quoted 6 lines · Trond Myklebust

Quoted 5 lines · Pavel Emelyanov
Quoted 4 lines · Trond Myklebust
Quoted 3 lines · Pavel Emelyanov
When the process is blocked on mandatory lock and someone changes the inode's permissions, so that the lock is no longer mandatory, nobody wakes up the blocked process, but probably should.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Please explain in more detail why we need this patch.
From "this fixes an OOPs/deadlock/leak" POV we do not. This is just an attempt to make the locking code be more consistent and clean.
Why do you think we get a deadlock or leak? AFAICS if the user turns off

I didn't' tell that.

Quoted 3 lines

mandatory locks on the file, then the existing locks default back into advisory locks which use the same notification mechanism as the mandatory locks.

True.

Quoted 5 lines

IOW: the process that is waiting in locks_mandatory_area() will be released as soon as the advisory lock is dropped. If that theory is broken in practice, then that is the bug that we need to fix. We neither want to add a load of locking crap to notify_change(), nor should we need to.

We have this for inotify already. Adding wakeup for mandatory lock is not that bad.

Anyway - I noticed, that the system state can become not consistent and proposed the way to fix it. If this inconsistency is not a big deal, and nobody cares, than I'm fine with forgetting this patch, since I have no other arguments to protect it, but "this is just not very nice without this patch".

Quoted 4 lines

Cheers
Trond

Thanks,
Pavel

J. Bruce Fields

2007-09-18 15:20 UTC #5 lore raw

Quoted 7 lines · Pavel Emelyanov

Quoted 5 lines · Trond Myklebust
IOW: the process that is waiting in locks_mandatory_area() will be released as soon as the advisory lock is dropped. If that theory is broken in practice, then that is the bug that we need to fix. We neither want to add a load of locking crap to notify_change(), nor should we need to.

Quoted 8 lines

We have this for inotify already. Adding wakeup for mandatory lock is not that bad.
Anyway - I noticed, that the system state can become not consistent and proposed the way to fix it. If this inconsistency is not a big deal, and nobody cares, than I'm fine with forgetting this patch, since I have no other arguments to protect it, but "this is just not very nice without this patch".

Maybe this should be documented, e.g. in fcntl(2). I'm not sure exactly what we'd say--we probably don't want to commit to the current behavior. Maybe something like "behavior is undefined when setting or clearing mandatory locking on a file while it is locked".

--b.

Trond Myklebust

2007-09-18 16:15 UTC #6 lore raw

Quoted 4 lines · J. Bruce Fields

Maybe this should be documented, e.g. in fcntl(2). I'm not sure exactly what we'd say--we probably don't want to commit to the current behavior. Maybe something like "behavior is undefined when setting or clearing mandatory locking on a file while it is locked".

The behaviour is pretty much undefined if you set/clear mandatory locking on the file while some application has it open. It is hard to see how you can avoid that unless you exclude simultaneous chmod, read(), write(), and fcntl(SETLK) operations.

Note also that strictly speaking, we're not even compliant with the System V behaviour on read() and write(). See:

http://www.unix.org.ua/orelly/networking_2ndEd/nfs/ch11_01.htm
and
http://docs.sun.com/app/docs/doc/801-6736/6i13fom0a?l=en&a=view&q=mandatory+lock

According to these docs, we should be wrapping each and every read() and write() syscall with a mandatory lock. The fact that we're not, and yet still not seeing any complaints just goes to show how few people are actually using and relying on this...

Trond

J. Bruce Fields

2007-09-18 16:52 UTC #7 lore raw

Quoted 11 lines · Trond Myklebust

Note also that strictly speaking, we're not even compliant with the System V behaviour on read() and write(). See:
http://www.unix.org.ua/orelly/networking_2ndEd/nfs/ch11_01.htm
and
http://docs.sun.com/app/docs/doc/801-6736/6i13fom0a?l=en&a=view&q=mandatory+lock
According to these docs, we should be wrapping each and every read() and write() syscall with a mandatory lock. The fact that we're not, and yet still not seeing any complaints just goes to show how few people are actually using and relying on this...

So currently there's nothing to prevent this:

- write passes locks_mandatory_area() checks
- get mandatory lock
- read old data
- write updates file data
- read new data

You can see the data change even while you hold a mandatory lock that should exclude writes.

Similarly you might think that an application could prevent anyone from seeing the intermediate state of a file while it performs a series of writes under an exclusive mandatory lock, but actually there's nothing to stop a read in progress from racing with acquisition of the lock.

Unless I'm missing something, that makes our mandatory lock implementation pretty pointless. I wish we could either fix it or just ditch it, but I suppose either option would be unpopular.

--b.

Trond Myklebust

2007-09-18 16:55 UTC #8 lore raw

Quoted 24 lines · J. Bruce Fields

Quoted 11 lines · Trond Myklebust
Note also that strictly speaking, we're not even compliant with the System V behaviour on read() and write(). See:
http://www.unix.org.ua/orelly/networking_2ndEd/nfs/ch11_01.htm
and
http://docs.sun.com/app/docs/doc/801-6736/6i13fom0a?l=en&a=view&q=mandatory+lock
According to these docs, we should be wrapping each and every read() and write() syscall with a mandatory lock. The fact that we're not, and yet still not seeing any complaints just goes to show how few people are actually using and relying on this...
So currently there's nothing to prevent this:
- write passes locks_mandatory_area() checks
- get mandatory lock
- read old data
- write updates file data
- read new data
You can see the data change even while you hold a mandatory lock that should exclude writes.

Quoted 8 lines

Similarly you might think that an application could prevent anyone from seeing the intermediate state of a file while it performs a series of writes under an exclusive mandatory lock, but actually there's nothing to stop a read in progress from racing with acquisition of the lock.
Unless I'm missing something, that makes our mandatory lock implementation pretty pointless. I wish we could either fix it or just ditch it, but I suppose either option would be unpopular.

It gets even better when you throw mmap() into the mix :-)

Trond

J. Bruce Fields

2007-09-18 17:40 UTC #9 lore raw

Quoted 17 lines · Trond Myklebust

Quoted 16 lines · J. Bruce Fields
So currently there's nothing to prevent this:
- write passes locks_mandatory_area() checks
- get mandatory lock
- read old data
- write updates file data
- read new data
You can see the data change even while you hold a mandatory lock that should exclude writes.
Similarly you might think that an application could prevent anyone from seeing the intermediate state of a file while it performs a series of writes under an exclusive mandatory lock, but actually there's nothing to stop a read in progress from racing with acquisition of the lock.

Quoted 5 lines

Quoted 3 lines
Unless I'm missing something, that makes our mandatory lock implementation pretty pointless. I wish we could either fix it or just ditch it, but I suppose either option would be unpopular.
It gets even better when you throw mmap() into the mix :-)

Hm. Documentation/mandatory.txt claims that it mandatory locks and mmap() with MAP_SHARED exclude each other, but I can't see where that's enfoced. That file doesn't make any mention of the above race.

So for now I think someone should update that file and fcntl(2) to mention these problems and to recommend rather strongly against using mandatory locking.

--b.

Hugh Dickins

2007-09-18 18:39 UTC #10 lore raw

Quoted 7 lines · J. Bruce Fields

Quoted 2 lines · Trond Myklebust
It gets even better when you throw mmap() into the mix :-)
Hm. Documentation/mandatory.txt claims that it mandatory locks and mmap() with MAP_SHARED exclude each other, but I can't see where that's enfoced. That file doesn't make any mention of the above race.

I believe the locks_verify_locked() call from mm/mmap.c prevents mmap'ing shared-write a file with mandatory locks in force; and the mapping_writably_mapped() calls from fs/locks.c prevent mandatory locking on a file while it's mmap'ed shared-write.

Though I think there's no lock to prevent those checks racing, so it's not quite watertight.

Hugh

J. Bruce Fields

2007-09-25 16:56 UTC #11 lore raw

PATCH 1/2 Documentation: move mandatory locking documentation to filesystems/

In reply to J. Bruce Fields

Shouldn't this mandatory-locking documentation be in the Documentation/filesystems directory?

Give it a more descriptive name while we're at it, and update 00-INDEX with a more inclusive description of Documentation/filesystems (which has already talked about more than just individual filesystems).

Signed-off-by J. Bruce Fields <[email protected]>

---

Quoted 7 lines · bfields

Hm. Documentation/mandatory.txt claims that it mandatory locks and mmap() with MAP_SHARED exclude each other, but I can't see where that's enfoced. That file doesn't make any mention of the above race.
So for now I think someone should update that file and fcntl(2) to mention these problems and to recommend rather strongly against using mandatory locking.

Anyone have an objection if I submit these two patches to Linus for 2.6.24?

--b.

 Documentation/00-INDEX                          |    4 +-
 Documentation/filesystems/00-INDEX              |    2 +
 Documentation/filesystems/mandatory-locking.txt |  152 +++++++++++++++++++++++
 Documentation/locks.txt                         |   10 +-
 Documentation/mandatory.txt                     |  152 -----------------------
 5 files changed, 160 insertions(+), 160 deletions(-)
 create mode 100644 Documentation/filesystems/mandatory-locking.txt
 delete mode 100644 Documentation/mandatory.txt

Documentation/00-INDEX +1 -3

index 43e89b1..910473c 100644
@@ -145,7 +145,7 @@ fb/
 feature-removal-schedule.txt
 	- list of files and features that are going to be removed.
 filesystems/
-	- directory with info on the various filesystems that Linux supports.
+	- info on the vfs and the various filesystems that Linux supports.
 firmware_class/
 	- request_firmware() hotplug interface info.
 floppy.txt

@@ -240,8 +240,6 @@ m68k/
 	- directory with info about Linux on Motorola 68k architecture.
 magic-number.txt
 	- list of magic numbers used to mark/protect kernel data structures.
-mandatory.txt
-	- info on the Linux implementation of Sys V mandatory file locking.
 mca.txt
 	- info on supporting Micro Channel Architecture (e.g. PS/2) systems.
 md.txt

Documentation/filesystems/00-INDEX +2 -0

index 59db1bc..73c8e94 100644
@@ -52,6 +52,8 @@ isofs.txt
 	- info and mount options for the ISO 9660 (CDROM) filesystem.
 jfs.txt
 	- info and mount options for the JFS filesystem.
+mandatory
+	- info on the Linux implementation of Sys V mandatory file locking.
 ncpfs.txt
 	- info on Novell Netware(tm) filesystem using NCP protocol.
 ntfs.txt

Documentation/filesystems/mandatory-locking.txt new file +152 -0

152 lines not on this page · raw message

Documentation/locks.txt +5 -5

index e3b402e..fab857a 100644
@@ -53,11 +53,11 @@ fcntl(), with all the problems that implies.
 1.3 Mandatory Locking As A Mount Option
 ---------------------------------------
 
-Mandatory locking, as described in 'Documentation/mandatory.txt' was prior
-to this release a general configuration option that was valid for all
-mounted filesystems. This had a number of inherent dangers, not the least
-of which was the ability to freeze an NFS server by asking it to read a
-file for which a mandatory lock existed.
+Mandatory locking, as described in 'Documentation/filesystems/mandatory.txt'
+was prior to this release a general configuration option that was valid for
+all mounted filesystems.  This had a number of inherent dangers, not the
+least of which was the ability to freeze an NFS server by asking it to read
+a file for which a mandatory lock existed.
 
 From this release of the kernel, mandatory locking can be turned on and off
 on a per-filesystem basis, using the mount options 'mand' and 'nomand'.

Documentation/mandatory.txt deleted +0 ~~-152~~

152 lines not on this page · raw message

Signature · 1 line

1.5.3.1.139.g9346b

J. Bruce Fields

2007-09-25 16:56 UTC #12 lore raw

PATCH 2/2 locks: add warning about mandatory locking races

The mandatory file locking implementation has long-standing races that probably render it useless. I know of no plans to fix them. Till we do, we should at least warn people.

Signed-off-by J. Bruce Fields <[email protected]>

---
 Documentation/filesystems/mandatory-locking.txt |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

Documentation/filesystems/mandatory-locking.txt +20 -1

index bc449d4..8ac5cfb 100644
@@ -3,7 +3,26 @@
 		Andy Walker <[email protected]>
 
 			   15 April 1996
-
+		     (Updated September 2007)
+
+0. Why should I avoid mandatory locking?
+----------------------------------------
+
+The Linux implementation is prey to a number of difficult-to-fix race
+conditions which in practice make it not dependable:
+
+	- The write system call checks for a mandatory lock only once
+	  at its start.  It is therefore possible for a lock request to
+	  be granted after this check but before the data is modified.
+	  A process may then see file data change even while a mandatory
+	  lock was held.
+	- Similarly, an exclusive lock may be granted on a file after
+	  the kernel has decided to proceed with a read, but before the
+	  read has actually completed, and the reading process may see
+	  the file data in a state which should not have been visible
+	  to it.
+	- Similar races make the claimed mutual exclusion between lock
+	  and mmap similarly unreliable.
 
 1. What is  mandatory locking?
 ------------------------------

Signature · 1 line

1.5.3.1.139.g9346b

Randy Dunlap

Acked-by on v1 1/2

2007-09-25 17:14 UTC #13 lore raw

Re: [PATCH 1/2] Documentation: move mandatory locking documentation to filesystems/

In reply to J. Bruce Fields

Quoted 2 lines · On Tue, 25 Sep 2007 12:55:51 -0400 J. Bruce Fields wrote:

Shouldn't this mandatory-locking documentation be in the Documentation/filesystems directory?

Agreed.

Quoted 3 lines

Give it a more descriptive name while we're at it, and update 00-INDEX with a more inclusive description of Documentation/filesystems (which has already talked about more than just individual filesystems).

OK.
One correction below.

Then

Acked-by Randy Dunlap <[email protected]>

Quoted 54 lines

Signed-off-by J. Bruce Fields <[email protected]>

---

Quoted 7 lines · bfields

Hm. Documentation/mandatory.txt claims that it mandatory locks and mmap() with MAP_SHARED exclude each other, but I can't see where that's enfoced. That file doesn't make any mention of the above race.
So for now I think someone should update that file and fcntl(2) to mention these problems and to recommend rather strongly against using mandatory locking.

Anyone have an objection if I submit these two patches to Linus for 2.6.24?

--b.

 Documentation/00-INDEX                          |    4 +-
 Documentation/filesystems/00-INDEX              |    2 +
 Documentation/filesystems/mandatory-locking.txt |  152 +++++++++++++++++++++++
 Documentation/locks.txt                         |   10 +-
 Documentation/mandatory.txt                     |  152 -----------------------
 5 files changed, 160 insertions(+), 160 deletions(-)
 create mode 100644 Documentation/filesystems/mandatory-locking.txt
 delete mode 100644 Documentation/mandatory.txt

Documentation/00-INDEX +1 -3

index 43e89b1..910473c 100644
@@ -145,7 +145,7 @@ fb/
 feature-removal-schedule.txt
 	- list of files and features that are going to be removed.
 filesystems/
-	- directory with info on the various filesystems that Linux supports.
+	- info on the vfs and the various filesystems that Linux supports.
 firmware_class/
 	- request_firmware() hotplug interface info.
 floppy.txt

@@ -240,8 +240,6 @@ m68k/
 	- directory with info about Linux on Motorola 68k architecture.
 magic-number.txt
 	- list of magic numbers used to mark/protect kernel data structures.
-mandatory.txt
-	- info on the Linux implementation of Sys V mandatory file locking.
 mca.txt
 	- info on supporting Micro Channel Architecture (e.g. PS/2) systems.
 md.txt

Documentation/filesystems/00-INDEX +0 -0

index 59db1bc..73c8e94 100644

Documentation/filesystems/00-INDEX isofs.txt · line 55

Quoted 4 lines

Documentation/filesystems/00-INDEX +1 -0

@@ -52,6 +52,8 @@ isofs.txt
 	- info and mount options for the ISO 9660 (CDROM) filesystem.
 jfs.txt
 	- info and mount options for the JFS filesystem.
+mandatory

mandatory-locking.txt

Documentation/filesystems/00-INDEX isofs.txt · line 59

Quoted 4 lines

Documentation/filesystems/00-INDEX +1 -0

+	- info on the Linux implementation of Sys V mandatory file locking.
 ncpfs.txt
 	- info on Novell Netware(tm) filesystem using NCP protocol.
 ntfs.txt

---
~Randy
Phaedrus says that Quality is about caring.

J. Bruce Fields

2007-09-25 17:25 UTC #14 lore raw

Re: [PATCH 1/2] Documentation: move mandatory locking documentation to filesystems/

Quoted 1 line · Randy Dunlap

One correction below.

...

Quoted 9 lines

Documentation/filesystems/00-INDEX isofs.txt · line 55
Quoted 7 lines
Documentation/filesystems/00-INDEX +1 -0
@@ -52,6 +52,8 @@ isofs.txt
 	- info and mount options for the ISO 9660 (CDROM) filesystem.
 jfs.txt
 	- info and mount options for the JFS filesystem.
+mandatory
mandatory-locking.txt

Thanks, fixed.

--b.