[src/trunk]: src/sys/fs/puffs Add a mutex for operations that touch size (set...

[manu <manu%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/0a56c0019764
branches:  trunk
changeset: 768918:0a56c0019764
user:      manu <manu%NetBSD.org@localhost>
date:      Mon Aug 29 04:12:45 2011 +0000

description:
Add a mutex for operations that touch size (setattr, getattr, write, fsync).

This is required to avoid data corruption bugs, where a getattr slices
itself within a setattr operation, and sets the size to the stall value
it got from the filesystem. That value is smaller than the one set by
setattr, and the call to uvm_vnp_setsize() trigged a spurious truncate.
The result is a chunk of zeroed data in the file.

Such a situation can easily happen when the ioflush thread issue a
VOP_FSYNC/puffs_vnop_sync/flushvncache/dosetattrn while andother process
do a sys_stat/VOP_GETATTR/puffs_vnop_getattr.

This mutex on size operation can be removed the day we decide VOP_GETATTR
has to operated on a locked vnode, since the other operations that touch
size already require that.

diffstat:

 sys/fs/puffs/puffs_node.c  |   6 +++-
 sys/fs/puffs/puffs_sys.h   |   4 ++-
 sys/fs/puffs/puffs_vnops.c |  47 +++++++++++++++++++++++++++++++++++++++------
 3 files changed, 47 insertions(+), 10 deletions(-)

diffs (204 lines):

diff -r 8b9b982402d7 -r 0a56c0019764 sys/fs/puffs/puffs_node.c
--- a/sys/fs/puffs/puffs_node.c Mon Aug 29 00:39:16 2011 +0000
+++ b/sys/fs/puffs/puffs_node.c Mon Aug 29 04:12:45 2011 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: puffs_node.c,v 1.19 2011/06/30 20:09:41 wiz Exp $      */
+/*     $NetBSD: puffs_node.c,v 1.20 2011/08/29 04:12:45 manu Exp $     */
 
 /*
  * Copyright (c) 2005, 2006, 2007  Antti Kantee.  All Rights Reserved.
@@ -30,7 +30,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: puffs_node.c,v 1.19 2011/06/30 20:09:41 wiz Exp $");
+__KERNEL_RCSID(0, "$NetBSD: puffs_node.c,v 1.20 2011/08/29 04:12:45 manu Exp $");
 
 #include <sys/param.h>
 #include <sys/hash.h>
@@ -155,6 +155,7 @@
                }
                KASSERT(pnc != NULL);
        }
+       mutex_init(&pnode->pn_sizemtx, MUTEX_DEFAULT, IPL_NONE);
        mutex_exit(&pmp->pmp_lock);
 
        vp->v_data = pnode;
@@ -463,6 +464,7 @@
        if (--pn->pn_refcount == 0) {
                mutex_exit(&pn->pn_mtx);
                mutex_destroy(&pn->pn_mtx);
+               mutex_destroy(&pn->pn_sizemtx);
                seldestroy(&pn->pn_sel);
                pool_put(&puffs_pnpool, pn);
        } else {
diff -r 8b9b982402d7 -r 0a56c0019764 sys/fs/puffs/puffs_sys.h
--- a/sys/fs/puffs/puffs_sys.h  Mon Aug 29 00:39:16 2011 +0000
+++ b/sys/fs/puffs/puffs_sys.h  Mon Aug 29 04:12:45 2011 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: puffs_sys.h,v 1.77 2011/01/11 14:04:54 kefren Exp $    */
+/*     $NetBSD: puffs_sys.h,v 1.78 2011/08/29 04:12:45 manu Exp $      */
 
 /*
  * Copyright (c) 2005, 2006  Antti Kantee.  All Rights Reserved.
@@ -209,6 +209,8 @@
 
        struct lockf *  pn_lockf;
 
+       kmutex_t        pn_sizemtx;     /* size modification mutex      */
+
        LIST_ENTRY(puffs_node) pn_hashent;
 };
 
diff -r 8b9b982402d7 -r 0a56c0019764 sys/fs/puffs/puffs_vnops.c
--- a/sys/fs/puffs/puffs_vnops.c        Mon Aug 29 00:39:16 2011 +0000
+++ b/sys/fs/puffs/puffs_vnops.c        Mon Aug 29 04:12:45 2011 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: puffs_vnops.c,v 1.154 2011/07/04 08:07:30 manu Exp $   */
+/*     $NetBSD: puffs_vnops.c,v 1.155 2011/08/29 04:12:45 manu Exp $   */
 
 /*
  * Copyright (c) 2005, 2006, 2007  Antti Kantee.  All Rights Reserved.
@@ -30,7 +30,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: puffs_vnops.c,v 1.154 2011/07/04 08:07:30 manu Exp $");
+__KERNEL_RCSID(0, "$NetBSD: puffs_vnops.c,v 1.155 2011/08/29 04:12:45 manu Exp $");
 
 #include <sys/param.h>
 #include <sys/buf.h>
@@ -839,6 +839,18 @@
        struct puffs_node *pn = VPTOPP(vp);
        int error = 0;
 
+       /*
+        * A lock is required so that we do not race with 
+        * setattr, write and fsync when changing vp->v_size.
+        * This is critical, since setting a stall smaler value
+        * triggers a file truncate in uvm_vnp_setsize(), which
+        * most of the time means data corruption (a chunk of
+        * data is replaced by zeroes). This can be removed if
+        * we decide one day that VOP_GETATTR must operate on 
+        * a locked vnode.
+        */
+       mutex_enter(&pn->pn_sizemtx);
+
        REFPN(pn);
        vap = ap->a_vap;
 
@@ -889,6 +901,9 @@
  out:
        puffs_releasenode(pn);
        PUFFS_MSG_RELEASE(getattr);
+       
+       mutex_exit(&pn->pn_sizemtx);
+
        return error;
 }
 
@@ -902,6 +917,8 @@
        struct puffs_node *pn = vp->v_data;
        int error = 0;
 
+       KASSERT(!(flags & SETATTR_CHSIZE) || mutex_owned(&pn->pn_sizemtx));
+
        if ((vp->v_mount->mnt_flag & MNT_RDONLY) &&
            (vap->va_uid != (uid_t)VNOVAL || vap->va_gid != (gid_t)VNOVAL
            || vap->va_atime.tv_sec != VNOVAL || vap->va_mtime.tv_sec != VNOVAL
@@ -972,8 +989,14 @@
                struct vattr *a_vap;
                kauth_cred_t a_cred;
        } */ *ap = v;
-
-       return dosetattr(ap->a_vp, ap->a_vap, ap->a_cred, SETATTR_CHSIZE);
+       struct puffs_node *pn = ap->a_vp->v_data;
+       int error;
+
+       mutex_enter(&pn->pn_sizemtx);
+       error = dosetattr(ap->a_vp, ap->a_vap, ap->a_cred, SETATTR_CHSIZE);
+       mutex_exit(&pn->pn_sizemtx);
+
+       return error;
 }
 
 static __inline int
@@ -1023,6 +1046,7 @@
        int error;
 
        pnode = vp->v_data;
+       mutex_enter(&pnode->pn_sizemtx);
 
        if (doinact(pmp, pnode->pn_stat & PNODE_DOINACT)) {
                flushvncache(vp, 0, 0, false);
@@ -1045,6 +1069,7 @@
                *ap->a_recycle = true;
        }
 
+       mutex_exit(&pnode->pn_sizemtx);
        VOP_UNLOCK(vp);
 
        return 0;
@@ -1328,14 +1353,15 @@
        } */ *ap = v;
        PUFFS_MSG_VARS(vn, fsync);
        struct vnode *vp = ap->a_vp;
+       struct puffs_node *pn = VPTOPP(vp);
        struct puffs_mount *pmp = MPTOPUFFSMP(vp->v_mount);
-       struct puffs_node *pn = VPTOPP(vp);
        int error, dofaf;
 
+       mutex_enter(&pn->pn_sizemtx);
        error = flushvncache(vp, ap->a_offlo, ap->a_offhi,
            (ap->a_flags & FSYNC_WAIT) == FSYNC_WAIT);
        if (error)
-               return error;
+               goto out;
 
        /*
         * HELLO!  We exit already here if the user server does not
@@ -1343,8 +1369,9 @@
         * has references neither in the kernel or the fs server.
         * Otherwise we continue to issue fsync() forward.
         */
+       error = 0;
        if (!EXISTSOP(pmp, FSYNC) || (pn->pn_stat & PNODE_DYING))
-               return 0;
+               goto out;
 
        dofaf = (ap->a_flags & FSYNC_WAIT) == 0 || ap->a_flags == FSYNC_LAZY;
        /*
@@ -1377,6 +1404,8 @@
 
        error = checkerr(pmp, error, __func__);
 
+out:
+       mutex_exit(&pn->pn_sizemtx);
        return error;
 }
 
@@ -1902,6 +1931,7 @@
        } */ *ap = v;
        PUFFS_MSG_VARS(vn, write);
        struct vnode *vp = ap->a_vp;
+       struct puffs_node *pn = VPTOPP(vp);
        struct puffs_mount *pmp = MPTOPUFFSMP(vp->v_mount);
        struct uio *uio = ap->a_uio;
        size_t tomove, argsize;
@@ -1913,6 +1943,8 @@
        error = uflags = 0;
        write_msg = NULL;
 
+       mutex_enter(&pn->pn_sizemtx);
+
        if (vp->v_type == VREG && PUFFS_USE_PAGECACHE(pmp)) {
                ubcflags = UBC_WRITE | UBC_PARTIALOK | UBC_UNMAP_FLAG(vp);
 
@@ -2034,6 +2066,7 @@
                puffs_msgmem_release(park_write);
        }
 
+       mutex_exit(&pn->pn_sizemtx);
        return error;
 }