Memory shortage can result in inconsistent flocks state
Subsystems (2)
PATCH 1/1 Memory shortage can result in inconsistent flocks state
When the flock_lock_file() is called to change the flock from F_RDLCK to F_WRLCK or vice versa the existing flock can be removed without appropriate warning.
Look:
for_each_lock(inode, before) {
struct file_lock *fl = *before;
if (IS_POSIX(fl))
break;
if (IS_LEASE(fl))
continue;
if (filp != fl->fl_file)
continue;
if (request->fl_type == fl->fl_type)
goto out;
found = 1;
locks_delete_lock(before); <<<<<< !
break;
}if after this point the subsequent locks_alloc_lock() will fail the return code will be -ENOMEM, but the existing lock is already removed.
This is a known feature that such "re-locking" is not atomic, but in the racy case the file should stay locked (although by some other process), but in this case the file will be unlocked.
The proposal is to prepare the lock in advance keeping no chance to fail in the future code.
Found during making the flocks pid-namespaces aware.
---
fs/locks.c +8 -4
@@ -732,6 +732,14 @@ static int flock_lock_file(struct file *lock_kernel ();if (request->fl_flags &FL_ACCESS ) goto find_conflict; + +if (request->fl_type !=F_UNLCK ) { + error = -ENOMEM ; + new_fl =locks_alloc_lock (); +if (new_fl ==NULL ) + goto out; + } +for_each_lock (inode, before) {struct file_lock *fl = *before;if (IS_POSIX (fl))
@@ -753,10 +761,6 @@ static int flock_lock_file(struct file * goto out; } - error = -ENOMEM ; - new_fl =locks_alloc_lock (); -if (new_fl ==NULL ) - goto out;/* * If a higher-priority process was blocked on the old file lock, * give it the opportunity to lock the file.
Quoted 3 lines · Pavel Emelyanov
This is a known feature that such "re-locking" is not atomic, but in the racy case the file should stay locked (although by some other process), but in this case the file will be unlocked.
That's a little subtle (I assume you've never seen this actually happen?), but it makes sense to me.
Quoted 2 lines
The proposal is to prepare the lock in advance keeping no chance to fail in the future code.
And the patch certainly looks correct.
I can add it to my (trivial) lock patches, if that's helpful--it'll get folded into the branch -mm pulls from and I can pass it along to Linus for 2.6.24.
What I don't have that I wish I did is good regression tests for the flock or lease code (for posix locks I've been using connectathon, though that misses some important things too).
--b.
Quoted 7 lines · J. Bruce Fields
Quoted 3 lines · Pavel Emelyanov
This is a known feature that such "re-locking" is not atomic, but in the racy case the file should stay locked (although by some other process), but in this case the file will be unlocked.
That's a little subtle (I assume you've never seen this actually happen?), but it makes sense to me.
Well, this situation is hard to notice since usually programs try to finish up when some error is returned from the kernel, but I do believe that this could happen in one of the openvz kernels since we limit the kernel memory usage for "containers" and thus -ENOMEM is a common error.
Quoted 8 lines
Quoted 2 lines
The proposal is to prepare the lock in advance keeping no chance to fail in the future code.
And the patch certainly looks correct.
I can add it to my (trivial) lock patches, if that's helpful--it'll get folded into the branch -mm pulls from and I can pass it along to Linus for 2.6.24.
Thanks.
Quoted 6 lines
What I don't have that I wish I did is good regression tests for the flock or lease code (for posix locks I've been using connectathon, though that misses some important things too).
--b.
Quoted 6 lines · Pavel Emelyanov
Quoted 5 lines · J. Bruce Fields
Quoted 3 lines · Pavel Emelyanov
This is a known feature that such "re-locking" is not atomic, but in the racy case the file should stay locked (although by some other process), but in this case the file will be unlocked.
Quoted 9 lines
Quoted 2 lines
That's a little subtle (I assume you've never seen this actually happen?), but it makes sense to me.
Well, this situation is hard to notice since usually programs try to finish up when some error is returned from the kernel, but I do believe that this could happen in one of the openvz kernels since we limit the kernel memory usage for "containers" and thus
-ENOMEMis a common error.
The fault injection framework should be able to introduce the same error. Of course hitting the error would require careful setup of the fault parameters.
Balbir
Quoted 57 lines · Pavel Emelyanov
When the
flock_lock_file()is called to change the flock fromF_RDLCKtoF_WRLCKor vice versa the existing flock can be removed without appropriate warning.Look:
for_each_lock(inode, before) { struct file_lock *fl = *before; if (IS_POSIX(fl)) break; if (IS_LEASE(fl)) continue; if (filp != fl->fl_file) continue; if (request->fl_type == fl->fl_type) goto out; found = 1; locks_delete_lock(before); <<<<<< ! break; }if after this point the subsequent
locks_alloc_lock()will fail the return code will be-ENOMEM, but the existing lock is already removed.This is a known feature that such "re-locking" is not atomic, but in the racy case the file should stay locked (although by some other process), but in this case the file will be unlocked.
The proposal is to prepare the lock in advance keeping no chance to fail in the future code.
Found during making the flocks pid-namespaces aware.
Signed-off-by Pavel Emelyanov <[email protected]>---
fs/locks.c +8
-0@@ -732,6 +732,14 @@ static int flock_lock_file(struct file *lock_kernel ();if (request->fl_flags &FL_ACCESS ) goto find_conflict; + +if (request->fl_type !=F_UNLCK ) { + error = -ENOMEM ; + new_fl =locks_alloc_lock (); +if (new_fl ==NULL ) + goto out; + } +for_each_lock (inode, before) {struct file_lock *fl = *before;if (IS_POSIX (fl))
fs/locks.c static int flock_lock_file(struct file * · line 766
Quoted 10 lines
fs/locks.c +0
-4@@ -753,10 +761,6 @@ static int flock_lock_file(struct file * goto out; } - error = -ENOMEM ; - new_fl =locks_alloc_lock (); -if (new_fl ==NULL ) - goto out;/* * If a higher-priority process was blocked on the old file lock, * give it the opportunity to lock the file.
Doesn't that create a leak in some cases?
Quoted 14 lines
for_each_lock(inode, before) {
struct file_lock *fl = *before;
if (IS_POSIX(fl))
break;
if (IS_LEASE(fl))
continue;
if (filp != fl->fl_file)
continue;
if (request->fl_type == fl->fl_type)
goto out; <<<<<<<<<<<<<<<< LEAK?
found = 1;
locks_delete_lock(before);
break;
}Quoted 24 lines · Chuck Ebbert
Quoted 23 lines · Pavel Emelyanov
fs/locks.c +8
-0@@ -732,6 +732,14 @@ static int flock_lock_file(struct file *lock_kernel ();if (request->fl_flags &FL_ACCESS ) goto find_conflict; + +if (request->fl_type !=F_UNLCK ) { + error = -ENOMEM ; + new_fl =locks_alloc_lock (); +if (new_fl ==NULL ) + goto out; + } +for_each_lock (inode, before) {struct file_lock *fl = *before;if (IS_POSIX (fl))@@ -753,10 +761,6 @@ static int flock_lock_file(struct file * goto out; }
Quoted 20 lines
fs/locks.c static int flock_lock_file(struct file * · line 766
Quoted 7 lines
fs/locks.c +0
-4- error = -ENOMEM ; - new_fl =locks_alloc_lock (); -if (new_fl ==NULL ) - goto out;/* * If a higher-priority process was blocked on the old file lock, * give it the opportunity to lock the file. Doesn't that create a leak in some cases?
Quoted 10 lines
for_each_lock(inode, before) { struct file_lock *fl = *before; if (IS_POSIX(fl)) break; if (IS_LEASE(fl)) continue; if (filp != fl->fl_file) continue; if (request->fl_type == fl->fl_type) goto out; <<<<<<<<<<<<<<<< LEAK?
You mean, a leak of the memory allocated for new_fl? That's freed at the exit labeled with "out". It's the only exit:
out: unlock_kernel(); if (new_fl) locks_free_lock(new_fl); return error;
And new_fl is initially NULL, assigned only once by the allocation, then assigned to NULL only at the very end when we know we've succeeded.
Am I missing something else?
--b.
Quoted 4 lines
Quoted 4 lines
found = 1; locks_delete_lock(before); break; }
Quoted 22 lines · J. Bruce Fields
Quoted 12 lines
Doesn't that create a leak in some cases?
Quoted 10 lines
for_each_lock(inode, before) { struct file_lock *fl = *before; if (IS_POSIX(fl)) break; if (IS_LEASE(fl)) continue; if (filp != fl->fl_file) continue; if (request->fl_type == fl->fl_type) goto out; <<<<<<<<<<<<<<<< LEAK?You mean, a leak of the memory allocated for
new_fl? That's freed at the exit labeled with "out". It's the only exit:out: unlock_kernel(); if (new_fl) locks_free_lock(new_fl); return error;
Quoted 5 lines
And
new_flis initially NULL, assigned only once by the allocation, then assigned to NULL only at the very end when we know we've succeeded.Am I missing something else?
Never mind, I didn't look closely enough. Looks good to me.