NetBSD Problem Report #37706

From martin@duskware.de  Sun Jan  6 13:08:27 2008
Return-Path: <martin@duskware.de>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id ABBCE63BDFB
	for <gnats-bugs@gnats.netbsd.org>; Sun,  6 Jan 2008 13:08:27 +0000 (UTC)
Message-Id: <20080106130659.514E363BDFA@narn.NetBSD.org>
Date: Sun,  6 Jan 2008 13:06:59 +0000 (UTC)
From: ad@netbsd.org
Reply-To: ad@netbsd.org
To: netbsd-bugs-owner@NetBSD.org
Subject: Forced unmount of file systems is unsafe
X-Send-Pr-Version: www-1.0

>Number:         37706
>Category:       kern
>Synopsis:       Forced unmount of file systems is unsafe
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Jan 06 13:10:00 +0000 2008
>Closed-Date:    Mon Jun 02 01:09:12 +0000 2014
>Last-Modified:  Mon Jun 02 01:09:12 +0000 2014
>Originator:     Andrew Doran
>Release:        4.99.48
>Organization:
The NetBSD Project
>Environment:
n/a
>Description:
- Reference counting is not done for struct mount and it can disappear
  out from underneath code that is using it.

- Modification of vnode_t::v_op while v_usecount != 0 is unsafe because
  it's not known whether deadfs or the original file system code will
  be called. Additionally the original fs code could be unloaded from
  the system.
>How-To-Repeat:
Code inspection.
>Fix:
For struct mount:

- Add reference counting to struct mount. Make struct mount
  persist until all references are removed.

- Replace lockmgr() 'handoff' trick used to access struct mount with
  reference counting and a rwlock.

- Have vnodes take references to struct mount.

- Have mounts reference the underlying file system type to prevent
  fs code from being unloaded.

- Have vfs_busy() return ENOENT for file systems in purgatory (this
  is done already in a slightly different way).

For vnode_t::v_op:

- Push locking back into file systems and store locks in the inode.

- Make VOP_RECLAIM() preserve the in-core inode structure if
  v_usecount > 1, but mark the structure as 'gone'.

- Make file system locks/gates return ENOENT if the inode is 'gone'.

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->ad
Responsible-Changed-By: ad@narn.netbsd.org
Responsible-Changed-When: Tue, 29 Jan 2008 17:33:34 +0000
Responsible-Changed-Why:
take


From: Andrew Doran <ad@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/37706 CVS commit: src/sys
Date: Wed, 30 Jan 2008 11:47:05 +0000 (UTC)

 Module Name:	src
 Committed By:	ad
 Date:		Wed Jan 30 11:47:05 UTC 2008

 Modified Files:
 	src/sys/coda: coda_psdev.c
 	src/sys/compat/common: vfs_syscalls_20.c
 	src/sys/compat/netbsd32: netbsd32_compat_20.c
 	src/sys/compat/ultrix: ultrix_fs.c
 	src/sys/fs/cd9660: cd9660_vfsops.c
 	src/sys/fs/filecorefs: filecore_vfsops.c
 	src/sys/fs/msdosfs: msdosfs_vfsops.c
 	src/sys/fs/ntfs: ntfs_vfsops.c
 	src/sys/fs/puffs: puffs_msgif.c
 	src/sys/fs/union: union_vnops.c
 	src/sys/kern: vfs_lookup.c vfs_subr.c vfs_subr2.c vfs_syscalls.c
 	src/sys/miscfs/procfs: procfs_linux.c
 	src/sys/miscfs/syncfs: sync_vnops.c
 	src/sys/nfs: nfs_export.c nfs_vfsops.c
 	src/sys/sys: mount.h
 	src/sys/ufs/ext2fs: ext2fs_vfsops.c
 	src/sys/ufs/ffs: ffs_snapshot.c ffs_vfsops.c
 	src/sys/ufs/lfs: lfs_bio.c lfs_syscalls.c lfs_vfsops.c
 	src/sys/ufs/mfs: mfs_vfsops.c
 	src/sys/ufs/ufs: ufs_vfsops.c

 Log Message:
 PR kern/37706 (forced unmount of file systems is unsafe):

 - Do reference counting for 'struct mount'. Each vnode associated with a
   mount takes a reference, and in turn the mount takes a reference to the
   vfsops.
 - Now that mounts are reference counted, replace the overcomplicated mount
   locking inherited from 4.4BSD with a recursable rwlock.


 To generate a diff of this commit:
 cvs rdiff -r1.37 -r1.38 src/sys/coda/coda_psdev.c
 cvs rdiff -r1.24 -r1.25 src/sys/compat/common/vfs_syscalls_20.c
 cvs rdiff -r1.19 -r1.20 src/sys/compat/netbsd32/netbsd32_compat_20.c
 cvs rdiff -r1.45 -r1.46 src/sys/compat/ultrix/ultrix_fs.c
 cvs rdiff -r1.55 -r1.56 src/sys/fs/cd9660/cd9660_vfsops.c
 cvs rdiff -r1.47 -r1.48 src/sys/fs/filecorefs/filecore_vfsops.c
 cvs rdiff -r1.60 -r1.61 src/sys/fs/msdosfs/msdosfs_vfsops.c
 cvs rdiff -r1.64 -r1.65 src/sys/fs/ntfs/ntfs_vfsops.c
 cvs rdiff -r1.64 -r1.65 src/sys/fs/puffs/puffs_msgif.c
 cvs rdiff -r1.27 -r1.28 src/sys/fs/union/union_vnops.c
 cvs rdiff -r1.103 -r1.104 src/sys/kern/vfs_lookup.c
 cvs rdiff -r1.326 -r1.327 src/sys/kern/vfs_subr.c
 cvs rdiff -r1.15 -r1.16 src/sys/kern/vfs_subr2.c
 cvs rdiff -r1.344 -r1.345 src/sys/kern/vfs_syscalls.c
 cvs rdiff -r1.47 -r1.48 src/sys/miscfs/procfs/procfs_linux.c
 cvs rdiff -r1.21 -r1.22 src/sys/miscfs/syncfs/sync_vnops.c
 cvs rdiff -r1.31 -r1.32 src/sys/nfs/nfs_export.c
 cvs rdiff -r1.193 -r1.194 src/sys/nfs/nfs_vfsops.c
 cvs rdiff -r1.172 -r1.173 src/sys/sys/mount.h
 cvs rdiff -r1.129 -r1.130 src/sys/ufs/ext2fs/ext2fs_vfsops.c
 cvs rdiff -r1.62 -r1.63 src/sys/ufs/ffs/ffs_snapshot.c
 cvs rdiff -r1.221 -r1.222 src/sys/ufs/ffs/ffs_vfsops.c
 cvs rdiff -r1.107 -r1.108 src/sys/ufs/lfs/lfs_bio.c
 cvs rdiff -r1.127 -r1.128 src/sys/ufs/lfs/lfs_syscalls.c
 cvs rdiff -r1.254 -r1.255 src/sys/ufs/lfs/lfs_vfsops.c
 cvs rdiff -r1.88 -r1.89 src/sys/ufs/mfs/mfs_vfsops.c
 cvs rdiff -r1.36 -r1.37 src/sys/ufs/ufs/ufs_vfsops.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->analyzed
State-Changed-By: ad@NetBSD.org
State-Changed-When: Wed, 05 Mar 2008 18:07:50 +0000
State-Changed-Why:
- problem with mounts is fixed
- remaining issue is vnode_t::v_op


Responsible-Changed-From-To: ad->kern-bug-people
Responsible-Changed-By: ad@NetBSD.org
Responsible-Changed-When: Fri, 30 May 2008 12:24:50 +0000
Responsible-Changed-Why:
Requires major rototill of the vnode locking scheme; punt.


From: "Juergen Hannken-Illjes" <hannken@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/37706 CVS commit: src
Date: Thu, 27 Feb 2014 16:51:39 +0000

 Module Name:	src
 Committed By:	hannken
 Date:		Thu Feb 27 16:51:39 UTC 2014

 Modified Files:
 	src/share/man/man9: vnodeops.9 vnsubr.9
 	src/sys/coda: coda_vnops.c
 	src/sys/fs/adosfs: adutil.c
 	src/sys/fs/cd9660: cd9660_node.c
 	src/sys/fs/efs: efs_ihash.c
 	src/sys/fs/filecorefs: filecore_node.c
 	src/sys/fs/hfs: hfs_nhash.c
 	src/sys/fs/ptyfs: ptyfs_subr.c
 	src/sys/fs/tmpfs: tmpfs_vnops.c
 	src/sys/fs/union: union_vnops.c
 	src/sys/kern: vfs_vnode.c vfs_vnops.c
 	src/sys/miscfs/deadfs: dead_vnops.c
 	src/sys/miscfs/fdesc: fdesc_vnops.c
 	src/sys/miscfs/genfs: genfs.h genfs_vnops.c layer_extern.h
 	    layer_vnops.c
 	src/sys/miscfs/kernfs: kernfs_subr.c
 	src/sys/miscfs/nullfs: null_vnops.c
 	src/sys/miscfs/overlay: overlay_vnops.c
 	src/sys/miscfs/umapfs: umap_vnops.c
 	src/sys/nfs: nfs_node.c
 	src/sys/sys: param.h
 	src/sys/ufs/chfs: chfs_ihash.c
 	src/sys/ufs/lfs: ulfs_ihash.c
 	src/sys/ufs/ufs: ufs_ihash.c

 Log Message:
 The current implementation of vn_lock() is racy.  Modification of
 the vnode operations vector for active vnodes is unsafe because it
 is not known whether deadfs or the original file system will be
 called.

 - Pass down LK_RETRY to the lock operation (hint for deadfs only).

 - Change deadfs lock operation to return ENOENT if LK_RETRY is unset.

 - Change all other lock operations to check for dead vnode once
   the vnode is locked and unlock and return ENOENT in this case.

 With these changes in place vnode lock operations will never succeed
 after vclean() has marked the vnode as VI_XLOCK and before vclean()
 has changed the operations vector.

 Adresses PR kern/37706 (Forced unmount of file systems is unsafe)

 Discussed on tech-kern.

 Welcome to 6.99.33


 To generate a diff of this commit:
 cvs rdiff -u -r1.92 -r1.93 src/share/man/man9/vnodeops.9
 cvs rdiff -u -r1.41 -r1.42 src/share/man/man9/vnsubr.9
 cvs rdiff -u -r1.94 -r1.95 src/sys/coda/coda_vnops.c
 cvs rdiff -u -r1.15 -r1.16 src/sys/fs/adosfs/adutil.c
 cvs rdiff -u -r1.29 -r1.30 src/sys/fs/cd9660/cd9660_node.c
 cvs rdiff -u -r1.9 -r1.10 src/sys/fs/efs/efs_ihash.c
 cvs rdiff -u -r1.25 -r1.26 src/sys/fs/filecorefs/filecore_node.c
 cvs rdiff -u -r1.12 -r1.13 src/sys/fs/hfs/hfs_nhash.c
 cvs rdiff -u -r1.25 -r1.26 src/sys/fs/ptyfs/ptyfs_subr.c
 cvs rdiff -u -r1.117 -r1.118 src/sys/fs/tmpfs/tmpfs_vnops.c
 cvs rdiff -u -r1.56 -r1.57 src/sys/fs/union/union_vnops.c
 cvs rdiff -u -r1.31 -r1.32 src/sys/kern/vfs_vnode.c
 cvs rdiff -u -r1.188 -r1.189 src/sys/kern/vfs_vnops.c
 cvs rdiff -u -r1.55 -r1.56 src/sys/miscfs/deadfs/dead_vnops.c
 cvs rdiff -u -r1.117 -r1.118 src/sys/miscfs/fdesc/fdesc_vnops.c
 cvs rdiff -u -r1.31 -r1.32 src/sys/miscfs/genfs/genfs.h
 cvs rdiff -u -r1.189 -r1.190 src/sys/miscfs/genfs/genfs_vnops.c
 cvs rdiff -u -r1.34 -r1.35 src/sys/miscfs/genfs/layer_extern.h
 cvs rdiff -u -r1.54 -r1.55 src/sys/miscfs/genfs/layer_vnops.c
 cvs rdiff -u -r1.25 -r1.26 src/sys/miscfs/kernfs/kernfs_subr.c
 cvs rdiff -u -r1.38 -r1.39 src/sys/miscfs/nullfs/null_vnops.c
 cvs rdiff -u -r1.19 -r1.20 src/sys/miscfs/overlay/overlay_vnops.c
 cvs rdiff -u -r1.55 -r1.56 src/sys/miscfs/umapfs/umap_vnops.c
 cvs rdiff -u -r1.116 -r1.117 src/sys/nfs/nfs_node.c
 cvs rdiff -u -r1.441 -r1.442 src/sys/sys/param.h
 cvs rdiff -u -r1.2 -r1.3 src/sys/ufs/chfs/chfs_ihash.c
 cvs rdiff -u -r1.3 -r1.4 src/sys/ufs/lfs/ulfs_ihash.c
 cvs rdiff -u -r1.31 -r1.32 src/sys/ufs/ufs/ufs_ihash.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: analyzed->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 02 Jun 2014 01:09:12 +0000
State-Changed-Why:
fixed, we believe.


>Unformatted:

Home	PR Database Search