Subject: kern/1145: umount & df may panic NetBSD
To: None <gnats@NetBSD.ORG>
From: Arne Henrik Juul <arnej@imf.unit.no>
List: netbsd-bugs
Date: 01/20/1997 13:41:06
According to cgd my original patch was:
: proposed fix insufficieint and erroneous.  thinking about correct
: locking solution.

Yes, my proposed fix was wrong.  I have an updated version, with
a bit more analysis:

The problem is that either the mp pointer or the nmp pointer could
get outdated if we had to wait and had a context switch someplace in
this code, typically in VFS_STATFS (which might need to perform an
NFS request).  If the *nmp filesystem got unmounted while waiting, the
original code would crash.  The following patch assumes that mp
cannot be invalidated during the VFS_STATFS (this seems to be the case
from my code inspection), and that there are no other places that a
context switch may occur.  (I'm a bit unsure about copyout(), but the
rest should be OK.)

At the very least, the code after this patch should have no new
bugs and solve the problem I originally reported.  Problems may
remain if VFS_STATFS does not in fact lock mp, or if copyout() could
block (and mp gets unmounted during that block).

Inspection of the corresponding code in FreeBSD shows that they lock
the filesystem in question using vfs_busy(mp).  If this is possible
in NetBSD the FreeBSD version of the routine should probably be
imported instead of this fix.  If using a vfs_busy() locking scheme
is a long-term goal, this fix should be applied in the meantime.

  -arnej

--- sys/kern/vfs_syscalls.c.orig	Sun Dec 22 05:06:45 1996
+++ sys/kern/vfs_syscalls.c	Mon Jan 20 04:59:36 1997
@@ -570,30 +570,32 @@
 	long count, maxcount, error;
 
 	maxcount = SCARG(uap, bufsize) / sizeof(struct statfs);
 	sfsp = (caddr_t)SCARG(uap, buf);
 	for (count = 0,
 	     mp = mountlist.cqh_first; mp != (void *)&mountlist; mp = nmp) {
-		nmp = mp->mnt_list.cqe_next;
 		if (sfsp && count < maxcount &&
 		    ((mp->mnt_flag & MNT_MLOCK) == 0)) {
 			sp = &mp->mnt_stat;
 			/*
 			 * If MNT_NOWAIT is specified, do not refresh the
 			 * fsstat cache. MNT_WAIT overrides MNT_NOWAIT.
 			 */
 			if (((SCARG(uap, flags) & MNT_NOWAIT) == 0 ||
 			    (SCARG(uap, flags) & MNT_WAIT)) &&
-			    (error = VFS_STATFS(mp, sp, p)))
+			    (error = VFS_STATFS(mp, sp, p))) {
+				nmp = mp->mnt_list.cqe_next;
 				continue;
+			}
 			sp->f_flags = mp->mnt_flag & MNT_VISFLAGMASK;
 			error = copyout(sp, sfsp, sizeof(*sp));
 			if (error)
 				return (error);
 			sfsp += sizeof(*sp);
 		}
+		nmp = mp->mnt_list.cqe_next;
 		count++;
 	}
 	if (sfsp && count > maxcount)
 		*retval = maxcount;
 	else
 		*retval = count;