Re: [PATCH][2.5.73] stack corruption in devfs_lookup

[Andrew Morton]

Andrey Borzenkov <[e-mail]> wrote:
>
> When devfs_lookup needs to call devfsd it arranges for other lookups for the 
>  same name to wait. It is using local variable as wait queue head. After 
>  devfsd returns devfs_lookup wakes up all waiters and returns. Unfortunately 
>  there is no garantee all waiters will actually get chance to run and clean up 
>  before devfs_lookup returns. so some of them attempt to access already freed 
>  storage on stack.

OK, but I think there is a simpler fix.  We can rely on the side-effects of
prepare_to_wait() and finish_wait().

The wakeup() will remove all wait_queue_t's from the wait_queue_head, and
so when the waiters wake up and call finish_wait(), they will never touch
the now-out-of-scope waitqueue head.

It is a little faster than the currentcode, too.

Could you please test this?

diff -puN fs/devfs/base.c~devfs-oops-fix-2 fs/devfs/base.c
--- 25/fs/devfs/base.c~devfs-oops-fix-2	2003-07-06 11:55:38.000000000 -0700
+++ 25-akpm/fs/devfs/base.c	2003-07-06 11:59:23.000000000 -0700
@@ -2218,7 +2218,6 @@ static int devfs_d_revalidate_wait (stru
     struct fs_info *fs_info = dir->i_sb->s_fs_info;
     devfs_handle_t parent = get_devfs_entry_from_vfs_inode (dir);
     struct devfs_lookup_struct *lookup_info = dentry->d_fsdata;
-    DECLARE_WAITQUEUE (wait, current);
 
     if ( is_devfsd_or_child (fs_info) )
     {
@@ -2252,11 +2251,12 @@ static int devfs_d_revalidate_wait (stru
     read_lock (&parent->u.dir.lock);
     if (dentry->d_fsdata)
     {
-	set_current_state (TASK_UNINTERRUPTIBLE);
-	add_wait_queue (&lookup_info->wait_queue, &wait);
-	read_unlock (&parent->u.dir.lock);
-	schedule ();
-	remove_wait_queue (&lookup_info->wait_queue, &wait);
+	DEFINE_WAIT(wait);
+
+	prepare_to_wait(&lookup_info->wait_queue, &wait, TASK_UNINTERRUPTIBLE);
+	read_unlock(&parent->u.dir.lock);
+	schedule();
+	finish_wait(&lookup_info->wait_queue, &wait);
     }
     else read_unlock (&parent->u.dir.lock);
     return 1;
@@ -2336,6 +2336,12 @@ out:
     dentry->d_op = &devfs_dops;
     dentry->d_fsdata = NULL;
     write_lock (&parent->u.dir.lock);
+    /*
+     * This wakeup will remove all waiters' wait_queue_t's from the waitqueue
+     * head, because the waiters use prepare_to_wait()/finished_wait().
+     * Hence it is OK that the waitqueue_head goes out of scope immediately
+     * after the wakeup is delivered
+     */
     wake_up (&lookup_info.wait_queue);
     write_unlock (&parent->u.dir.lock);
     devfs_put (de);

_