kern/48135: Bad locking for umount

[mlelstv%serpens.de@localhost]

>Number:         48135
>Category:       kern
>Synopsis:       Bad locking for umount
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Aug 18 11:00:00 +0000 2013
>Originator:     mlelstv%serpens.de@localhost
>Release:        NetBSD 6.99.23
>Organization:
        
>Environment:
        
        
System: NetBSD pussyfoot 6.99.23 NetBSD 6.99.23 (PUSSYFOOT) #2: Sun Aug 18 
12:13:03 CEST 2013 
mlelstv@pussyfoot:/home/netbsd-current/obj.amd64/home/netbsd-current/src/sys/arch/amd64/compile/PUSSYFOOT
 amd64
Architecture: x86_64
Machine: amd64
>Description:
Accessing a filesystem that is being umounted can cause a panic
for a LOCKDEBUG kernel and undefined behaviour otherwise.

There is a race condition between vfs_busy() and vfs_destroy().

When vfs_destroy() decrements the reference count of a mount to
zero, it assumes that no code accesses the mount point.

vfs_busy() starts with the assumption that there is already
a reference to the mount point. However, that reference may go
away at any time.

A LOCKDEBUG kernel panics when vfs_busy() has acquired a mutex
and vfs_destroy() tries to destroy the mutex. Without that check
vfs_destroy() continues to free the data structure that is still
used by vfs_busy().

>How-To-Repeat:
On a LOCKDEBUG system, start automounter with auto_attrcache=1, and
run something like:

while true; do
        ls -ld /amd/watchpoint/.
done

>Fix:

The following diff makes vfs_busy() increment the reference count
to protect the data structure and calls vfs_destroy() in its
failure path to unreference (and possibly free) the data structure.

The last hunk avoids another possible use-after-free case in
vfs_unbusy().

Index: sys/kern/vfs_mount.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_mount.c,v
retrieving revision 1.19
diff -u -r1.19 vfs_mount.c
--- sys/kern/vfs_mount.c        28 Apr 2013 21:34:31 -0000      1.19
+++ sys/kern/vfs_mount.c        18 Aug 2013 10:48:42 -0000
@@ -288,6 +288,7 @@
 
        KASSERT(mp->mnt_refcnt > 0);
 
+       atomic_inc_uint(&mp->mnt_refcnt);
        mutex_enter(&mp->mnt_unmounting);
        if (__predict_false((mp->mnt_iflag & IMNT_GONE) != 0)) {
                mutex_exit(&mp->mnt_unmounting);
@@ -295,6 +296,7 @@
                        KASSERT(mutex_owned(&mountlist_lock));
                        *nextp = CIRCLEQ_NEXT(mp, mnt_list);
                }
+               vfs_destroy(mp);
                return ENOENT;
        }
        ++mp->mnt_busynest;
@@ -303,7 +305,6 @@
        if (nextp != NULL) {
                mutex_exit(&mountlist_lock);
        }
-       atomic_inc_uint(&mp->mnt_refcnt);
        return 0;
 }
 
@@ -328,13 +329,13 @@
        KASSERT(mp->mnt_busynest != 0);
        mp->mnt_busynest--;
        mutex_exit(&mp->mnt_unmounting);
-       if (!keepref) {
-               vfs_destroy(mp);
-       }
        if (nextp != NULL) {
                KASSERT(mutex_owned(&mountlist_lock));
                *nextp = CIRCLEQ_NEXT(mp, mnt_list);
        }
+       if (!keepref) {
+               vfs_destroy(mp);
+       }
 }
 
 /*

>Unformatted: