From: VMware, Inc <> Date: Thu, 17 Dec 2009 22:58:54 +0000 (-0800) Subject: FreeBSD VMBLOCK: avoid crash when doing lookup in a subdirectory X-Git-Tag: 2009.12.16-217847~10 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b88a4ac0b09242cc69f07e8eb260ad76c2d84a82;p=thirdparty%2Fopen-vm-tools.git FreeBSD VMBLOCK: avoid crash when doing lookup in a subdirectory When trying to reconstruct path to a vnode during lookup of a file within subdirectory the driver gets confused and crashes the kernel. This can be easily triggered by doing H->G DnD of a small directory. Instead of trying to reconstruct it every time let's just store the latest lookup that lead to discovery of the vnode (similar to how Linux driver does this). Signed-off-by: Marcelo Vanzin --- diff --git a/open-vm-tools/modules/freebsd/vmblock/subr.c b/open-vm-tools/modules/freebsd/vmblock/subr.c index 23c6fc50a..17865a048 100644 --- a/open-vm-tools/modules/freebsd/vmblock/subr.c +++ b/open-vm-tools/modules/freebsd/vmblock/subr.c @@ -330,7 +330,8 @@ VMBlockInsMntQueDtr(struct vnode *vp, // IN: node to cleanup * Zero on success, an appropriate system error otherwise. * * Side effects: - * None. + * In case of success takes over ownership of pathname thus caller + * has to dispose of it only if error was signalled. * * Original function comment: * @@ -348,8 +349,8 @@ int VMBlockNodeGet(struct mount *mp, // IN: VMBlock fs info struct vnode *lowervp, // IN: lower layer vnode struct vnode **vpp, // OUT: upper layer/alias vnode - struct vnode *dvp) // IN: Pointer to vmblock layer parent, - // if any + char *pathname) // IN: Pointer to the path we took to + // reach this vnode { struct VMBlockNode *xp; struct vnode *vp; @@ -383,6 +384,7 @@ VMBlockNodeGet(struct mount *mp, // IN: VMBlock fs info return error; } + xp->name = pathname; xp->backVnode = vp; xp->lowerVnode = lowervp; vp->v_type = lowervp->v_type; @@ -415,190 +417,14 @@ VMBlockNodeGet(struct mount *mp, // IN: VMBlock fs info vp->v_vnlock = &vp->v_lock; xp->lowerVnode = NULL; vrele(vp); - return 0; } else { - xp->parentVnode = dvp; + *vpp = vp; } - *vpp = vp; return 0; } -/* - *----------------------------------------------------------------------------- - * - * VMBlockSetNodeName -- - * - * If not already set, assign a VMBlockNode its lower layer vnode's - * path name. (E.g., if /tmp/VMwareDND is remounted to /var/run/vmblock, - * the root VMBlockNode will have a path name of "/tmp/VMwareDND".) - * - * In the interest of saving a little memory, for all vnodes except the - * mountpoint, we only copy the first pathname component (i.e., up to the - * first slash). - * - * Results: - * None. - * - * Side effects: - * This function may not be called with mutexes held. - * - *----------------------------------------------------------------------------- - */ - -void -VMBlockSetNodeName(struct vnode *vp, // IN: assignee vnode - const char *name) // IN: name assigned -{ - VMBlockNode *blockNode; - char *slash; - - KASSERT(vp != NULL, ("vp is NULL")); - KASSERT(name != NULL, ("name is NULL")); - - blockNode = VPTOVMB(vp); - - /* - * We assume that blocks are only placed on directories, not files, - * and by doing so define the relationship between VMBlockNodes and pathnames - * is 1:1. (One cannot hardlink directories.) As such, renaming a node is - * pointless, so we just return if this node is already named. - */ - if (blockNode->componentName != NULL) { - return; - } - - /* - * If given an absolute path or a lookup's final pathname component, - * copy until the end of the string. Otherwise, copy only until - * the first slash is found. - */ - slash = index(name, '/'); - - if ((name[0] == '/') || (slash == NULL)) { - blockNode->componentSize = strlen(name) + 1; - } else { - blockNode->componentSize = (slash - name) + 1; - } - - /* - * Finally allocate & copy buffer. - */ - blockNode->componentName = malloc(blockNode->componentSize, - M_VMBLOCKFSNODE, M_WAITOK); - strlcpy(blockNode->componentName, name, blockNode->componentSize); -} - - -/* - *----------------------------------------------------------------------------- - * - * VMBlockBuildBlockName -- - * - * Given a leaf vnode, compile and return a string representing its - * entire pathname. - * - * After grabbing a buffer from a pathname zone/slab, we copy the - * leaf's component name to the end of the buffer. Then we look up - * its parent and prepend the parent's name in the buffer, and so on - * until we reach the filesystem mountpoint. - * - * Results: - * Pointer to C-string pathname on success, NULL otherwise. - * - * Side effects: - * The returned string must be destroyed with VMBlockDestroyBlockName. - * Also, the caller must not be holding any mutexes. (uma_zalloc is - * allowed to sleep.) - * - *----------------------------------------------------------------------------- - */ - -char * -VMBlockBuildBlockName(struct vnode *vp) // IN: leaf vnode -{ - VMBlockNode *xp; - unsigned int written = 0; - char *buf, *tstart; - - buf = uma_zalloc(VMBlockPathnameZone, M_WAITOK); - tstart = &buf[MAXPATHLEN]; - - /* - * Stopping after processing the root vnode (no parent): - * - Decrement copy destination pointer by the size of the current - * componentName's buffer. - * - Memcpy componentName to destination pointer. (Memcpy will include - * terminating NUL.) - * - Replace each component's terminator with a pathname delimiter ('/'). - * (For simplicity, even the leaf component is terminated with a '/', - * but the final built string is manually terminated after this loop.) - * - * This function and its dependent VMBlockVnode::parentVnode member assume - * the following: - * - We expect only to be called in VMBlockVopLookup with vp locked. - * As such, vp is locked, and all its parent directories up to the - * VMBlock filesystem root are also locked. This means we don't have to - * worry about VMBlock vnodes disappearing out from under us. - */ - do { - xp = VPTOVMB(vp); - vp = xp->parentVnode; - - /* Note: componentSize includes terminator. */ - tstart -= xp->componentSize; - written += xp->componentSize; - if (written > MAXPATHLEN) { - Warning("%s: name too long (%u bytes)\n", __func__, written); - uma_zfree(VMBlockPathnameZone, buf); - return NULL; - } - - /* Copy & reterminate. */ - memcpy(tstart, xp->componentName, xp->componentSize); - tstart[xp->componentSize - 1] = '/'; - } while (vp != NULL); - - /* Reterminate string. */ - buf[MAXPATHLEN - 1] = '\0'; - - /* - * Since we copied components from leaf to root, the start of the pathname - * string could be anywhere in the buffer. To keep things simple for - * callers, just move it to the beginning of the buffer. - * - * Don't worry! Bcopy() works with overlapping buffers, and "written" - * includes the NUL terminator. - */ - bcopy(tstart, buf, written); - return buf; -} - - -/* - *----------------------------------------------------------------------------- - * - * VMBlockDestroyBlockName -- - * - * Free a name compiled by VMBlockBuildBlockName. - * - * Results: - * None. - * - * Side effects: - * None. - * - *----------------------------------------------------------------------------- - */ - -void -VMBlockDestroyBlockName(char *blockName) // IN: name to free -{ - uma_zfree(VMBlockPathnameZone, blockName); -} - - #ifdef DIAGNOSTIC /* if (DIAGNOSTIC) { */ /* diff --git a/open-vm-tools/modules/freebsd/vmblock/vfsops.c b/open-vm-tools/modules/freebsd/vmblock/vfsops.c index a430b6193..bd53a8c1e 100644 --- a/open-vm-tools/modules/freebsd/vmblock/vfsops.c +++ b/open-vm-tools/modules/freebsd/vmblock/vfsops.c @@ -124,6 +124,7 @@ VMBlockVFSMount(struct mount *mp, // IN: mount(2) parameters struct nameidata nd, *ndp = &nd; struct vnode *lowerrootvp, *vp; char *target; + char *pathname; int len, error = 0; VMBLOCKDEBUG("VMBlockVFSMount(mp = %p)\n", (void *)mp); @@ -143,7 +144,7 @@ VMBlockVFSMount(struct mount *mp, // IN: mount(2) parameters (mp->mnt_vnodecovered->v_op == &VMBlockVnodeOps)) { return EOPNOTSUPP; } - + /* * XXX Should only be unlocked if mnt_flag & MNT_UPDATE. */ @@ -159,6 +160,16 @@ VMBlockVFSMount(struct mount *mp, // IN: mount(2) parameters return EINVAL; } + pathname = uma_zalloc(VMBlockPathnameZone, M_WAITOK); + if (pathname == NULL) { + return ENOMEM; + } + + if (strlcpy(pathname, target, MAXPATHLEN) >= MAXPATHLEN) { + uma_zfree(VMBlockPathnameZone, pathname); + return ENAMETOOLONG; + } + /* * Find lower node and lock if not already locked. */ @@ -167,6 +178,7 @@ VMBlockVFSMount(struct mount *mp, // IN: mount(2) parameters error = namei(ndp); if (error) { NDFREE(ndp, 0); + uma_zfree(VMBlockPathnameZone, pathname); return error; } NDFREE(ndp, NDF_ONLY_PNBUF); @@ -178,6 +190,7 @@ VMBlockVFSMount(struct mount *mp, // IN: mount(2) parameters if (lowerrootvp == VPTOVMB(mp->mnt_vnodecovered)->lowerVnode) { VMBLOCKDEBUG("VMBlockVFSMount: multi vmblock mount?\n"); vput(lowerrootvp); + uma_zfree(VMBlockPathnameZone, pathname); return EDEADLK; } @@ -188,7 +201,7 @@ VMBlockVFSMount(struct mount *mp, // IN: mount(2) parameters * by grabbing a VMBlockNode for our layer's root. */ xmp->mountVFS = lowerrootvp->v_mount; - error = VMBlockNodeGet(mp, lowerrootvp, &vp, NULL); + error = VMBlockNodeGet(mp, lowerrootvp, &vp, pathname); /* * Make sure the node alias worked @@ -197,16 +210,10 @@ VMBlockVFSMount(struct mount *mp, // IN: mount(2) parameters COMPAT_VOP_UNLOCK(vp, 0, compat_td); vrele(lowerrootvp); free(xmp, M_VMBLOCKFSMNT); /* XXX */ + uma_zfree(VMBlockPathnameZone, pathname); return error; } - /* - * Assign the staging area's mount path to its corresponding VMBlockNode. - * (This is to allow blocking a directory and everything under that - * directory.) - */ - VMBlockSetNodeName(vp, target); - /* * Record a reference to the new filesystem's root vnode & mark it as such. */ diff --git a/open-vm-tools/modules/freebsd/vmblock/vmblock_k.h b/open-vm-tools/modules/freebsd/vmblock/vmblock_k.h index a800c11a1..fad8c6858 100644 --- a/open-vm-tools/modules/freebsd/vmblock/vmblock_k.h +++ b/open-vm-tools/modules/freebsd/vmblock/vmblock_k.h @@ -126,13 +126,8 @@ typedef struct VMBlockNode { LIST_ENTRY(VMBlockNode) hashEntry; /* Hash chain element (contains ptr to next node, etc.) */ struct vnode *lowerVnode; /* VREFed once */ - struct vnode *backVnode; /* Back pointer */ - struct vnode *parentVnode; /* Parent directory -- intended only to - rebuild pathnames in VopLookup. - Assumptions documented in - BuildBlockName. */ - char *componentName; /* Component name from lookup */ - size_t componentSize; /* Size of componentName (len + 1) */ + struct vnode *backVnode; /* Back pointer */ + char *name; /* Looked up path to vnode */ } VMBlockNode; @@ -151,13 +146,10 @@ extern uma_zone_t VMBlockPathnameZone; int VMBlockInit(struct vfsconf *vfsp); int VMBlockUninit(struct vfsconf *vfsp); int VMBlockNodeGet(struct mount *mp, struct vnode *target, struct vnode **vpp, - struct vnode *dvp); -void VMBlockSetNodeName(struct vnode *vp, const char *name); + char *pathname); void VMBlockHashRem(struct VMBlockNode *xp); void VMBlockSetupFileOps(void); int VMBlockVopBypass(struct vop_generic_args *ap); -char *VMBlockBuildBlockName(struct vnode *vp); -void VMBlockDestroyBlockName(char *name); #ifdef DIAGNOSTIC struct vnode *VMBlockCheckVp(struct vnode *vp, char *fil, int lno); diff --git a/open-vm-tools/modules/freebsd/vmblock/vnops.c b/open-vm-tools/modules/freebsd/vmblock/vnops.c index c9241e15c..0e61e6124 100644 --- a/open-vm-tools/modules/freebsd/vmblock/vnops.c +++ b/open-vm-tools/modules/freebsd/vmblock/vnops.c @@ -545,6 +545,7 @@ struct vop_generic_args { } vppp = VOPARG_OFFSETTO(struct vnode***, descp->vdesc_vpp_offset,ap); if (*vppp) { + /* FIXME: set proper name for the vnode */ error = VMBlockNodeGet(old_vps[0]->v_mount, **vppp, *vppp, NULL); } } @@ -596,7 +597,8 @@ struct vop_lookup_args { BlockHandle blockCookie; int flags = cnp->cn_flags; int error = 0; - char *savename = NULL, *pathname = NULL; + char *pathname; + size_t pathname_len; /* * Fail attempts to modify a read-only filesystem w/o bothering with a @@ -624,8 +626,26 @@ struct vop_lookup_args { * * If we find we were forcibly unmounted, fail with EIO. */ - if ((pathname = VMBlockBuildBlockName(dvp)) == NULL) { - return ENAMETOOLONG; + + pathname = uma_zalloc(VMBlockPathnameZone, M_WAITOK); + if (pathname == NULL) { + return ENOMEM; + } + + /* + * FIXME: we need to ensure that vnode always has name set up. + * Currently VMBlockVopBypass() may produce vnodes without a name. + */ + pathname_len = strlcpy(pathname, + VPTOVMB(dvp)->name ? VPTOVMB(dvp)->name : ".", + MAXPATHLEN); + /* + * Make sure we have room in the buffer to add our component. + * + 1 is for separator (slash). + */ + if (pathname_len + 1 + cnp->cn_namelen >= MAXPATHLEN) { + error = ENAMETOOLONG; + goto out; } if ((blockCookie = BlockLookup(pathname, OS_UNKNOWN_BLOCKER)) != NULL) { @@ -650,6 +670,11 @@ struct vop_lookup_args { } } + /* We already verified that buffer is big enough. */ + pathname[pathname_len] = '/'; + bcopy(cnp->cn_nameptr, &pathname[pathname_len + 1], cnp->cn_namelen); + pathname[pathname_len + 1 + cnp->cn_namelen] = 0; + /* * Although it is possible to call VMBlockVopBypass(), we'll do a direct * call to reduce overhead @@ -657,12 +682,6 @@ struct vop_lookup_args { ldvp = VMBVPTOLOWERVP(dvp); vp = lvp = NULL; - /* - * The subsequent VOP_LOOKUP() may advance cnp->cn_nameptr, so save a copy - * in order to refer to the current component later. - */ - savename = cnp->cn_nameptr; - error = VOP_LOOKUP(ldvp, &lvp, cnp); if (error == EJUSTRETURN && (flags & ISLASTCN) && (dvp->v_mount->mnt_flag & MNT_RDONLY) && @@ -680,19 +699,20 @@ struct vop_lookup_args { VREF(dvp); vrele(lvp); } else { - error = VMBlockNodeGet(dvp->v_mount, lvp, &vp, dvp); + error = VMBlockNodeGet(dvp->v_mount, lvp, &vp, pathname); if (error) { /* XXX Cleanup needed... */ panic("VMBlockNodeGet failed"); } *ap->a_vpp = vp; - VMBlockSetNodeName(vp, savename); + /* The vnode now owns pathname so don't try to free it below. */ + pathname = NULL; } } out: if (pathname) { - VMBlockDestroyBlockName(pathname); + uma_zfree(VMBlockPathnameZone, pathname); } return error; } @@ -920,7 +940,7 @@ struct vop_ioctl_args { /* * Don't block the mount point! */ - if (!strcmp(VPTOVMB(vp)->componentName, pathbuf)) { + if (!strcmp(VPTOVMB(vp)->name, pathbuf)) { ret = EINVAL; } else { ret = (ap->a_command == VMBLOCK_ADD_FILEBLOCK) ? @@ -1417,8 +1437,8 @@ struct vop_reclaim_args { /* * Clean up VMBlockNode attachment. */ - if (xp->componentName) { - free(xp->componentName, M_VMBLOCKFSNODE); + if (xp->name) { + uma_zfree(VMBlockPathnameZone, xp->name); } free(xp, M_VMBLOCKFSNODE);