Hi,

When traversing the mount list, the current mount point is locked
with vfs_busy().  When a FOREACH_SAFE macro is used, the next pointer
is not locked and could be freed by another process.

I remember that someone showed me a panic that ended in one of these
functions, but I have lost the bug report.

Anyway, the fix is easy in most places, do not use _SAFE as it is
unsafe.  In vfs_unmountall() the current pointer is actullay freed.
I have added a comment and will think about it again after my other
dounmount() fix has been commited.

The problem has been introduced in 2013 with this commit message.
> Change 'mountlist' from CIRCLEQ to TAILQ. Be paranoid and
> use TAILQ_*_SAFE more than might be needed.

ok?

bluhm

Index: kern/vfs_subr.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/vfs_subr.c,v
retrieving revision 1.256
diff -u -p -r1.256 vfs_subr.c
--- kern/vfs_subr.c     10 Jan 2017 19:56:34 -0000      1.256
+++ kern/vfs_subr.c     12 Jan 2017 15:52:51 -0000
@@ -1251,12 +1251,12 @@ vprint(char *label, struct vnode *vp)
 void
 printlockedvnodes(void)
 {
-       struct mount *mp, *nmp;
+       struct mount *mp;
        struct vnode *vp;
 
        printf("Locked vnodes\n");
 
-       TAILQ_FOREACH_SAFE(mp, &mountlist, mnt_list, nmp) {
+       TAILQ_FOREACH(mp, &mountlist, mnt_list) {
                if (vfs_busy(mp, VB_READ|VB_NOWAIT))
                        continue;
                LIST_FOREACH(vp, &mp->mnt_vnodelist, v_mntvnodes) {
@@ -1574,8 +1574,9 @@ vfs_unmountall(void)
  retry:
        allerror = 0;
        TAILQ_FOREACH_REVERSE_SAFE(mp, &mountlist, mntlist, mnt_list, nmp) {
-               if ((vfs_busy(mp, VB_WRITE|VB_NOWAIT)) != 0)
+               if (vfs_busy(mp, VB_WRITE|VB_NOWAIT))
                        continue;
+               /* XXX Here is a race, the next pointer is not locked. */
                if ((error = dounmount(mp, MNT_FORCE, curproc)) != 0) {
                        printf("unmount of %s failed with error %d\n",
                            mp->mnt_stat.f_mntonname, error);
Index: kern/vfs_syscalls.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.267
diff -u -p -r1.267 vfs_syscalls.c
--- kern/vfs_syscalls.c 10 Jan 2017 20:13:17 -0000      1.267
+++ kern/vfs_syscalls.c 12 Jan 2017 15:44:37 -0000
@@ -425,10 +425,10 @@ struct ctldebug debug0 = { "syncprt", &s
 int
 sys_sync(struct proc *p, void *v, register_t *retval)
 {
-       struct mount *mp, *nmp;
+       struct mount *mp;
        int asyncflag;
 
-       TAILQ_FOREACH_REVERSE_SAFE(mp, &mountlist, mntlist, mnt_list, nmp) {
+       TAILQ_FOREACH_REVERSE(mp, &mountlist, mntlist, mnt_list) {
                if (vfs_busy(mp, VB_READ|VB_NOWAIT))
                        continue;
                if ((mp->mnt_flag & MNT_RDONLY) == 0) {
@@ -570,7 +570,7 @@ sys_getfsstat(struct proc *p, void *v, r
                syscallarg(size_t) bufsize;
                syscallarg(int) flags;
        } */ *uap = v;
-       struct mount *mp, *nmp;
+       struct mount *mp;
        struct statfs *sp;
        struct statfs *sfsp;
        size_t count, maxcount;
@@ -580,7 +580,7 @@ sys_getfsstat(struct proc *p, void *v, r
        sfsp = SCARG(uap, buf);
        count = 0;
 
-       TAILQ_FOREACH_SAFE(mp, &mountlist, mnt_list, nmp) {
+       TAILQ_FOREACH(mp, &mountlist, mnt_list) {
                if (vfs_busy(mp, VB_READ|VB_NOWAIT))
                        continue;
                if (sfsp && count < maxcount) {

Reply via email to