nfs client oops, all 2.4 kernels

[Trond Myklebust]

>>>>> " " == Erik DeBill <[e-mail]> writes:

     > I've been running into a repeatable oops in the NFS client
     > code, apparently related to file locking.

This is the same Oops as was sent in by Olivier Molteni. It's due to
an incorrect assumption in the locking code. To repeat:

2 processes are calling close(), which under POSIX, tries to free all
locks held by a program. The problem occurs when you `dup()' a file.

In that case, both processes can call close() at the same time, thus
triggering a call to filp_close().

The race boils down to:

   -  locks_unlock_delete() assumes that the BKL (kernel_lock()) is
      sufficient to protect against *thisfl_p from disappearing
      beneath it due to some second process.
BUT
   -  The call to lock() in locks_unlock_delete() sleeps when the
      underlying filesystem is NFS, hence 2 processes can race despite
      the BKL assumption.

IMHO the correct thing to do is to detach the lock from the global
linked list *before* one makes the call to f_op->lock() down to the
NFS code.

This would probably entail splitting locks_delete_lock() into 2
procedures: one that does the unlinking from the global list:

        *thisfl_p = fl->fl_next;
        fl->fl_next = NULL;

        list_del(&fl->fl_link);
        INIT_LIST_HEAD(&fl->fl_link);

the other that does the rest.

Could you check if the appended patch works?

Cheers,
   Trond

--- linux-2.4.9/fs/locks.c.orig	Thu Jul  5 00:39:28 2001
+++ linux-2.4.9/fs/locks.c	Tue Sep 11 10:41:53 2001
@@ -473,21 +473,26 @@
 		fl->fl_insert(fl);
 }
 
-/* Delete a lock and then free it.
- * Remove our lock from the lock lists, wake up processes that are blocked
- * waiting for this lock, notify the FS that the lock has been cleared and
- * finally free the lock.
+/*
+ * Remove lock from the lock lists
  */
-static void locks_delete_lock(struct file_lock **thisfl_p, unsigned int wait)
+static inline void _unhash_lock(struct file_lock **thisfl_p)
 {
 	struct file_lock *fl = *thisfl_p;
 
 	*thisfl_p = fl->fl_next;
 	fl->fl_next = NULL;
 
-	list_del(&fl->fl_link);
-	INIT_LIST_HEAD(&fl->fl_link);
+	list_del_init(&fl->fl_link);
+}
 
+/*
+ * Wake up processes that are blocked waiting for this lock,
+ * notify the FS that the lock has been cleared and
+ * finally free the lock.
+ */
+static inline void _delete_lock(struct file_lock *fl, unsigned int wait)
+{
 	fasync_helper(0, fl->fl_file, 0, &fl->fl_fasync);
 	if (fl->fl_fasync != NULL){
 		printk(KERN_ERR "locks_delete_lock: fasync == %p\n", fl->fl_fasync);
@@ -502,6 +507,15 @@
 }
 
 /*
+ * Delete a lock and then free it.
+ */
+static void locks_delete_lock(struct file_lock **thisfl_p, unsigned int wait)
+{
+	_unhash_lock(thisfl_p);
+	_delete_lock(*thisfl_p, wait);
+}
+
+/*
  * Call back client filesystem in order to get it to unregister a lock,
  * then delete lock. Essentially useful only in locks_remove_*().
  * Note: this must be called with the semaphore already held!
@@ -511,12 +525,13 @@
 	struct file_lock *fl = *thisfl_p;
 	int (*lock)(struct file *, int, struct file_lock *);
 
+	_unhash_lock(thisfl_p);
 	if (fl->fl_file->f_op &&
 	    (lock = fl->fl_file->f_op->lock) != NULL) {
 		fl->fl_type = F_UNLCK;
 		lock(fl->fl_file, F_SETLK, fl);
 	}
-	locks_delete_lock(thisfl_p, 0);
+	_delete_lock(fl, 0);
 }
 
 /* Determine if lock sys_fl blocks lock caller_fl. Common functionality
@@ -1661,6 +1676,7 @@
 	while ((fl = *before) != NULL) {
 		if ((fl->fl_flags & FL_POSIX) && fl->fl_owner == owner) {
 			locks_unlock_delete(before);
+			before = &inode->i_flock;
 			continue;
 		}
 		before = &fl->fl_next;