broken flock()

broken flock()

[David Ford]

>From http://sendmail.org/

NOTE: Linux appears to have broken flock() again.  Unless
	the bug is fixed before sendmail 8.13 is shipped,
	8.13 will change the default locking method to
	fcntl() for Linux kernel 2.4 and later.  You may
	want to do this in 8.12 by compiling with
	-DHASFLOCK=0.  Be sure to update other sendmail
	related programs to match locking techniques.

Is it really broken or is sendmail smoking crack like when they said that itimers in Linux didn't work?

David

Re: broken flock()

[Stephen C. Tweedie]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

Hi,

On Fri, Jun 28, 2002 at 01:02:59PM -0400, David Ford wrote:
 
> NOTE: Linux appears to have broken flock() again.  Unless
> 	the bug is fixed before sendmail 8.13 is shipped,
> 	8.13 will change the default locking method to
> 	fcntl() for Linux kernel 2.4 and later.  You may
> 	want to do this in 8.12 by compiling with
> 	-DHASFLOCK=0.  Be sure to update other sendmail
> 	related programs to match locking techniques.
 
> Is it really broken or is sendmail smoking crack like when they said
> that itimers in Linux didn't work?

It really is broken, and sendmail triggers it (at least their
commercial binaries do).  I've already been talking to willy about the
problem.

The trouble is the accounting: if one process opens a fd and then
fork()s, it is possible for the lock to be taken in the parent and
released in the child (or vice versa) --- unless there's an explicit
flock(LOCK_UN), then the lock will be released implicitly when the
last reference to the fd is closed.

When this happens, we get the lock count incremented in one task and
decremented in another.  That can wrap the lock count backwards to -1
(or rather ~0UL), which causes the locks rlimit check to think we've
exceeded the lock quota and new lock requests will fail.  It's easy to
reproduce this: try the attached prog.  It produces an erroneous
ENOLCK due to the bug.

Cheers,
 Stephen

[-- Attachment #2: locklim.c --]
[-- Type: text/plain, Size: 980 bytes --]

#include <sys/types.h>
#include <sys/wait.h>
#include <sys/file.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>



int lock_file(int fd, int l_options) {

	int ret;
	
	ret = flock(fd, l_options);

	if (ret) 
		perror("flock error");
	return ret;
}

int main() {

	int fd;
	char *filename = "/tmp/lockf1";
	pid_t pid;
	int syncpipe[2];
	char c;

	pipe(syncpipe);
	
	fd = open(filename, O_CREAT | O_RDWR, 0666);
	if(fd < 0) {
		perror("parent could not open file");
		exit(1);
	}

	pid = fork();

	if(pid < 0) {
		perror("fork failed");
		exit(1);
	}
	
	if (pid) {
		lock_file(fd, LOCK_EX);
		if (close(fd))
			perror("parent: error closing file");
		write(syncpipe[1], &c, 1);
	} else {
		/* Wait until the parent has taken the lock */
		read(syncpipe[0], &c, 1);
		
		lock_file(fd, LOCK_UN);
                lock_file(fd, LOCK_EX);
                if(close(fd))
			perror("child: error closing file");
		exit(0);
	}

	wait(NULL);
	return 0;
	
}

broken flock()

[Matthew Wilcox]

OK, I seem to have accumulated at least 4 different bug reports of the
same problem over the last year (!).  I've cc'd everyone who's sent mail
about it that I could find.  Please exercise common sense when replying
to this...

The problem is definitely in the file lock accounting.  Since it's
effectively useless anyway, taking it out seems to be the right thing
to do for 2.4.  Here's a patch to do that:

diff -urNX dontdiff linux-2418/fs/locks.c linux-2418-acct/fs/locks.c
--- linux-2418/fs/locks.c	Thu Oct 11 08:52:18 2001
+++ linux-2418-acct/fs/locks.c	Mon Jul  1 16:23:36 2002
@@ -134,15 +134,9 @@
 static kmem_cache_t *filelock_cache;
 
 /* Allocate an empty lock structure. */
-static struct file_lock *locks_alloc_lock(int account)
+static struct file_lock *locks_alloc_lock(void)
 {
-	struct file_lock *fl;
-	if (account && current->locks >= current->rlim[RLIMIT_LOCKS].rlim_cur)
-		return NULL;
-	fl = kmem_cache_alloc(filelock_cache, SLAB_KERNEL);
-	if (fl)
-		current->locks++;
-	return fl;
+	return kmem_cache_alloc(filelock_cache, SLAB_KERNEL);
 }
 
 /* Free a lock which is not in use. */
@@ -152,7 +146,6 @@
 		BUG();
 		return;
 	}
-	current->locks--;
 	if (waitqueue_active(&fl->fl_wait))
 		panic("Attempting to free lock with active wait queue");
 
@@ -219,7 +212,7 @@
 /* Fill in a file_lock structure with an appropriate FLOCK lock. */
 static struct file_lock *flock_make_lock(struct file *filp, unsigned int type)
 {
-	struct file_lock *fl = locks_alloc_lock(1);
+	struct file_lock *fl = locks_alloc_lock();
 	if (fl == NULL)
 		return NULL;
 
@@ -348,7 +341,7 @@
 /* Allocate a file_lock initialised to this type of lease */
 static int lease_alloc(struct file *filp, int type, struct file_lock **flp)
 {
-	struct file_lock *fl = locks_alloc_lock(1);
+	struct file_lock *fl = locks_alloc_lock();
 	if (fl == NULL)
 		return -ENOMEM;
 
@@ -712,7 +705,7 @@
 			 size_t count)
 {
 	struct file_lock *fl;
-	struct file_lock *new_fl = locks_alloc_lock(0);
+	struct file_lock *new_fl = locks_alloc_lock();
 	int error;
 
 	if (new_fl == NULL)
@@ -872,8 +865,8 @@
 	 * We may need two file_lock structures for this operation,
 	 * so we get them in advance to avoid races.
 	 */
-	new_fl = locks_alloc_lock(0);
-	new_fl2 = locks_alloc_lock(0);
+	new_fl = locks_alloc_lock();
+	new_fl2 = locks_alloc_lock();
 	error = -ENOLCK; /* "no luck" */
 	if (!(new_fl && new_fl2))
 		goto out_nolock;
@@ -1426,7 +1419,7 @@
 int fcntl_setlk(unsigned int fd, unsigned int cmd, struct flock *l)
 {
 	struct file *filp;
-	struct file_lock *file_lock = locks_alloc_lock(0);
+	struct file_lock *file_lock = locks_alloc_lock();
 	struct flock flock;
 	struct inode *inode;
 	int error;
@@ -1582,7 +1575,7 @@
 int fcntl_setlk64(unsigned int fd, unsigned int cmd, struct flock64 *l)
 {
 	struct file *filp;
-	struct file_lock *file_lock = locks_alloc_lock(0);
+	struct file_lock *file_lock = locks_alloc_lock();
 	struct flock64 flock;
 	struct inode *inode;
 	int error;

-- 
Revolutions do not require corporate support.