race with i_flock?

I've been getting some funny freezes while running Specweb. I don't think this is the cause, but I did run into it.

Program received signal SIGSEGV, Segmentation fault. 0xc0147540 in may_open (nd=0xdaa69f78, acc_mode=6, flag=3)

    at /home/dave/oprofile/linux-2.5.25/include/linux/fs.h:1047

which is:
static inline int get_lease(struct inode *inode, unsigned int mode) {
------->if (inode->i_flock && (inode->i_flock->fl_flags & FL_LEASE))

                return __get_lease(inode, mode);
        return 0;
}

It appears that i_flock is NULL:

(gdb) print inode
$1 = (struct inode *) 0xe8d638f0
(gdb) print *inode
$2 = {...big snip

     i_flock = 0x0,
     ...}

(gdb) print &inode->i_flock
$3 = (struct file_lock **) 0xe8d6396c

But, there was a check for that just a second earlier. Looks racy to me. I noticed that there is use of i_sem in some of the calling functions, but not in get_lease()'s call sequence. Is this a problem?
A quick grep for i_flock didn't show any obvious places where it
was set back to NULL.

#0 0xc0147540 in may_open (nd=0xdaa69f78, acc_mode=6, flag=3)

    at /home/dave/oprofile/linux-2.5.25/include/linux/fs.h:1047
#1  0xc014786c in open_namei (pathname=0xe7292000 "/mnt/sdc1/www/post.log", flag=3, mode=438,
    nd=0xdaa69f78) at namei.c:1294
#2  0xc013bcbb in filp_open (filename=0xe7292000 "/mnt/sdc1/www/post.log", flags=2, mode=438)
    at open.c:616
#3  0xc013c0df in sys_open (filename=0x8130cd0 "/mnt/sdc1/www/post.log", flags=2, mode=438)
    at open.c:806

#4 0xc0106fbb in syscall_call ()

Dave Hansen
[email protected]

I get that on about every 1000th reboot (ie: once a week ;))

I have a poke around in kgdb when it happens and everything looks just fine. So what must be happening is that some other CPU has come in and fixed up the pointer before the kgdb stub captured that CPU.

I'll grab a trace next time.

-

Doh! That's entirely possible. open() could race with posix_lock_file and remove the first element of the i_flock list between the two tests. So... let's change get_lease() to be:

static inline int get_lease(struct inode *inode, unsigned int mode)

{
	if (inode->i_flock)
		return __get_lease(inode, mode);
	return 0;
}

__get_lease in 2.5.x has sufficient checks in it already; 2.4 does not and needs something like this:

lock_kernel();
flock = inode->i_flock;
+ if (!flock || (flock->fl_flags & FL_LEASE) == 0)
+ goto out;
if (flock->fl_type & F_INPROGRESS) {
if ((mode & O_NONBLOCK)

This must be a 1-insn wide race. I'm very impressed you managed to hit it ;-)

Revolutions do not require corporate support.

You have no idea :)

How about this patch? I can't believe that I'm spreading the BKL, but it is needed in this case. And, you _are_ removing it from flocking in 2.5, right?

Dave Hansen
[email protected]

@@ -1083,8 +1083,6 @@
 	alloc_err = lease_alloc(NULL, mode & FMODE_WRITE ? F_WRLCK : F_RDLCK,
 			&new_fl);
 
-	lock_kernel();
-
 	time_out_leases(inode);
 
 	flock = inode->i_flock;

@@ -1155,7 +1153,6 @@
 	}
 
 out:
-	unlock_kernel();
 	if (!alloc_err)
 		locks_free_lock(new_fl);
 	return error;

@@ -1044,9 +1044,12 @@
 
 static inline int get_lease(struct inode *inode, unsigned int mode)
 {
+	int ret = 0;
+	lock_kernel();
 	if (inode->i_flock && (inode->i_flock->fl_flags & FL_LEASE))
-		return __get_lease(inode, mode);
-	return 0;
+		ret = __get_lease(inode, mode);
+	unlock_kernel();
+	return ret;
 }
 
 /* fs/open.c */

Hang on, I don't see that it's required. We're entitled to check the condition when not holding a lock, since assignments are atomic. Then we go back to check the condition still exists while holding the lock.

Revolutions do not require corporate support.