From owner-dev-commits-src-main@freebsd.org Wed Sep 1 14:51:48 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 6490366C4F3; Wed, 1 Sep 2021 14:51:48 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4H06TJ286Gz4YTR; Wed, 1 Sep 2021 14:51:48 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 27A3D26DC4; Wed, 1 Sep 2021 14:51:48 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 181EpmlR064981; Wed, 1 Sep 2021 14:51:48 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 181EpmBv064980; Wed, 1 Sep 2021 14:51:48 GMT (envelope-from git) Date: Wed, 1 Sep 2021 14:51:48 GMT Message-Id: <202109011451.181EpmBv064980@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: "Jason A. Harmening" Subject: git: abe95116ba10 - main - unionfs: rework pathname handling MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: jah X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: abe95116ba10c8ab56f3ccd72a969ee29a9eef0b Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Sep 2021 14:51:48 -0000 The branch main has been updated by jah: URL: https://cgit.FreeBSD.org/src/commit/?id=abe95116ba10c8ab56f3ccd72a969ee29a9eef0b commit abe95116ba10c8ab56f3ccd72a969ee29a9eef0b Author: Jason A. Harmening AuthorDate: 2021-08-29 21:36:15 +0000 Commit: Jason A. Harmening CommitDate: 2021-09-01 14:55:09 +0000 unionfs: rework pathname handling Running stress2 unionfs tests reliably produces a namei_zone corruption panic due to unionfs_relookup() attempting to NUL-terminate a newly- allocate pathname buffer without first validating the buffer length. Instead, avoid allocating new pathname buffers in unionfs entirely, using already-provided buffers while ensuring the the correct flags are set in struct componentname to prevent freeing or manipulation of those buffers at lower layers. While here, also compute and store the path length once in the unionfs node instead of constantly invoking strlen() on it. Reviewed by: kib, markj Differential Revision: https://reviews.freebsd.org/D31728 --- sys/fs/unionfs/union.h | 4 +- sys/fs/unionfs/union_subr.c | 97 ++++++++++++-------------------------------- sys/fs/unionfs/union_vnops.c | 17 ++++---- 3 files changed, 39 insertions(+), 79 deletions(-) diff --git a/sys/fs/unionfs/union.h b/sys/fs/unionfs/union.h index 96180480dbec..a3484f818c66 100644 --- a/sys/fs/unionfs/union.h +++ b/sys/fs/unionfs/union.h @@ -98,6 +98,7 @@ struct unionfs_node { u_long un_hashmask; /* bit mask */ char *un_path; /* path */ + int un_pathlen; /* strlen of path */ int un_flag; /* unionfs node flag */ }; @@ -124,7 +125,8 @@ int unionfs_copyfile(struct unionfs_node *unp, int docopy, struct ucred *cred, s void unionfs_create_uppervattr_core(struct unionfs_mount *ump, struct vattr *lva, struct vattr *uva, struct thread *td); int unionfs_create_uppervattr(struct unionfs_mount *ump, struct vnode *lvp, struct vattr *uva, struct ucred *cred, struct thread *td); int unionfs_mkshadowdir(struct unionfs_mount *ump, struct vnode *duvp, struct unionfs_node *unp, struct componentname *cnp, struct thread *td); -int unionfs_mkwhiteout(struct vnode *dvp, struct componentname *cnp, struct thread *td, char *path); +int unionfs_mkwhiteout(struct vnode *dvp, struct componentname *cnp, + struct thread *td, char *path, int pathlen); int unionfs_relookup(struct vnode *dvp, struct vnode **vpp, struct componentname *cnp, struct componentname *cn, struct thread *td, char *path, int pathlen, u_long nameiop); int unionfs_relookup_for_create(struct vnode *dvp, struct componentname *cnp, struct thread *td); int unionfs_relookup_for_delete(struct vnode *dvp, struct componentname *cnp, struct thread *td); diff --git a/sys/fs/unionfs/union_subr.c b/sys/fs/unionfs/union_subr.c index 04d00fd77e39..d7d6b16a6ac8 100644 --- a/sys/fs/unionfs/union_subr.c +++ b/sys/fs/unionfs/union_subr.c @@ -332,6 +332,7 @@ unionfs_nodeget(struct mount *mp, struct vnode *uppervp, malloc(cnp->cn_namelen +1, M_UNIONFSPATH, M_WAITOK|M_ZERO); bcopy(cnp->cn_nameptr, unp->un_path, cnp->cn_namelen); unp->un_path[cnp->cn_namelen] = '\0'; + unp->un_pathlen = cnp->cn_namelen; } vp->v_type = vt; vp->v_data = unp; @@ -420,6 +421,7 @@ unionfs_noderem(struct vnode *vp, struct thread *td) if (unp->un_path != NULL) { free(unp->un_path, M_UNIONFSPATH); unp->un_path = NULL; + unp->un_pathlen = 0; } if (unp->un_hashtbl != NULL) { @@ -576,16 +578,12 @@ unionfs_relookup(struct vnode *dvp, struct vnode **vpp, int error; cn->cn_namelen = pathlen; - cn->cn_pnbuf = uma_zalloc(namei_zone, M_WAITOK); - bcopy(path, cn->cn_pnbuf, pathlen); - cn->cn_pnbuf[pathlen] = '\0'; - + cn->cn_pnbuf = path; cn->cn_nameiop = nameiop; cn->cn_flags = (LOCKPARENT | LOCKLEAF | HASBUF | SAVENAME | ISLASTCN); cn->cn_lkflags = LK_EXCLUSIVE; cn->cn_thread = td; cn->cn_cred = cnp->cn_cred; - cn->cn_nameptr = cn->cn_pnbuf; if (nameiop == DELETE) @@ -599,12 +597,16 @@ unionfs_relookup(struct vnode *dvp, struct vnode **vpp, VOP_UNLOCK(dvp); if ((error = relookup(dvp, vpp, cn))) { - uma_zfree(namei_zone, cn->cn_pnbuf); - cn->cn_flags &= ~HASBUF; vn_lock(dvp, LK_EXCLUSIVE | LK_RETRY); } else vrele(dvp); + KASSERT((cn->cn_flags & HASBUF) != 0, + ("%s: HASBUF cleared", __func__)); + KASSERT((cn->cn_flags & SAVENAME) != 0, + ("%s: SAVENAME cleared", __func__)); + KASSERT(cn->cn_pnbuf == path, ("%s: cn_pnbuf changed", __func__)); + return (error); } @@ -630,7 +632,7 @@ unionfs_relookup_for_create(struct vnode *dvp, struct componentname *cnp, vp = NULLVP; error = unionfs_relookup(udvp, &vp, cnp, &cn, td, cnp->cn_nameptr, - strlen(cnp->cn_nameptr), CREATE); + cnp->cn_namelen, CREATE); if (error) return (error); @@ -643,16 +645,6 @@ unionfs_relookup_for_create(struct vnode *dvp, struct componentname *cnp, error = EEXIST; } - if (cn.cn_flags & HASBUF) { - uma_zfree(namei_zone, cn.cn_pnbuf); - cn.cn_flags &= ~HASBUF; - } - - if (!error) { - cn.cn_flags |= (cnp->cn_flags & HASBUF); - cnp->cn_flags = cn.cn_flags; - } - return (error); } @@ -674,7 +666,7 @@ unionfs_relookup_for_delete(struct vnode *dvp, struct componentname *cnp, vp = NULLVP; error = unionfs_relookup(udvp, &vp, cnp, &cn, td, cnp->cn_nameptr, - strlen(cnp->cn_nameptr), DELETE); + cnp->cn_namelen, DELETE); if (error) return (error); @@ -687,16 +679,6 @@ unionfs_relookup_for_delete(struct vnode *dvp, struct componentname *cnp, vput(vp); } - if (cn.cn_flags & HASBUF) { - uma_zfree(namei_zone, cn.cn_pnbuf); - cn.cn_flags &= ~HASBUF; - } - - if (!error) { - cn.cn_flags |= (cnp->cn_flags & HASBUF); - cnp->cn_flags = cn.cn_flags; - } - return (error); } @@ -718,7 +700,7 @@ unionfs_relookup_for_rename(struct vnode *dvp, struct componentname *cnp, vp = NULLVP; error = unionfs_relookup(udvp, &vp, cnp, &cn, td, cnp->cn_nameptr, - strlen(cnp->cn_nameptr), RENAME); + cnp->cn_namelen, RENAME); if (error) return (error); @@ -729,18 +711,7 @@ unionfs_relookup_for_rename(struct vnode *dvp, struct componentname *cnp, vput(vp); } - if (cn.cn_flags & HASBUF) { - uma_zfree(namei_zone, cn.cn_pnbuf); - cn.cn_flags &= ~HASBUF; - } - - if (!error) { - cn.cn_flags |= (cnp->cn_flags & HASBUF); - cnp->cn_flags = cn.cn_flags; - } - return (error); - } /* @@ -848,11 +819,11 @@ unionfs_mkshadowdir(struct unionfs_mount *ump, struct vnode *udvp, vput(uvp); error = EEXIST; - goto unionfs_mkshadowdir_free_out; + goto unionfs_mkshadowdir_abort; } if ((error = vn_start_write(udvp, &mp, V_WAIT | PCATCH))) - goto unionfs_mkshadowdir_free_out; + goto unionfs_mkshadowdir_abort; unionfs_create_uppervattr_core(ump, &lva, &va, td); error = VOP_MKDIR(udvp, &uvp, &nd.ni_cnd, &va); @@ -869,12 +840,6 @@ unionfs_mkshadowdir(struct unionfs_mount *ump, struct vnode *udvp, } vn_finished_write(mp); -unionfs_mkshadowdir_free_out: - if (nd.ni_cnd.cn_flags & HASBUF) { - uma_zfree(namei_zone, nd.ni_cnd.cn_pnbuf); - nd.ni_cnd.cn_flags &= ~HASBUF; - } - unionfs_mkshadowdir_abort: cnp->cn_cred = credbk; chgproccnt(cred->cr_ruidinfo, -1, 0); @@ -890,26 +855,20 @@ unionfs_mkshadowdir_abort: */ int unionfs_mkwhiteout(struct vnode *dvp, struct componentname *cnp, - struct thread *td, char *path) + struct thread *td, char *path, int pathlen) { - int error; struct vnode *wvp; struct nameidata nd; struct mount *mp; - - if (path == NULL) - path = cnp->cn_nameptr; + int error; wvp = NULLVP; NDPREINIT(&nd); if ((error = unionfs_relookup(dvp, &wvp, cnp, &nd.ni_cnd, td, path, - strlen(path), CREATE))) + pathlen, CREATE))) { return (error); + } if (wvp != NULLVP) { - if (nd.ni_cnd.cn_flags & HASBUF) { - uma_zfree(namei_zone, nd.ni_cnd.cn_pnbuf); - nd.ni_cnd.cn_flags &= ~HASBUF; - } if (dvp == wvp) vrele(wvp); else @@ -925,11 +884,6 @@ unionfs_mkwhiteout(struct vnode *dvp, struct componentname *cnp, vn_finished_write(mp); unionfs_mkwhiteout_free_out: - if (nd.ni_cnd.cn_flags & HASBUF) { - uma_zfree(namei_zone, nd.ni_cnd.cn_pnbuf); - nd.ni_cnd.cn_flags &= ~HASBUF; - } - return (error); } @@ -969,9 +923,8 @@ unionfs_vn_create_on_upper(struct vnode **vpp, struct vnode *udvp, if (unp->un_path == NULL) panic("unionfs: un_path is null"); - nd.ni_cnd.cn_namelen = strlen(unp->un_path); - nd.ni_cnd.cn_pnbuf = uma_zalloc(namei_zone, M_WAITOK); - bcopy(unp->un_path, nd.ni_cnd.cn_pnbuf, nd.ni_cnd.cn_namelen + 1); + nd.ni_cnd.cn_namelen = unp->un_pathlen; + nd.ni_cnd.cn_pnbuf = unp->un_path; nd.ni_cnd.cn_nameiop = CREATE; nd.ni_cnd.cn_flags = LOCKPARENT | LOCKLEAF | HASBUF | SAVENAME | ISLASTCN; @@ -1015,10 +968,12 @@ unionfs_vn_create_on_upper_free_out1: VOP_UNLOCK(udvp); unionfs_vn_create_on_upper_free_out2: - if (nd.ni_cnd.cn_flags & HASBUF) { - uma_zfree(namei_zone, nd.ni_cnd.cn_pnbuf); - nd.ni_cnd.cn_flags &= ~HASBUF; - } + KASSERT((nd.ni_cnd.cn_flags & HASBUF) != 0, + ("%s: HASBUF cleared", __func__)); + KASSERT((nd.ni_cnd.cn_flags & SAVENAME) != 0, + ("%s: SAVENAME cleared", __func__)); + KASSERT(nd.ni_cnd.cn_pnbuf == unp->un_path, + ("%s: cn_pnbuf changed", __func__)); return (error); } diff --git a/sys/fs/unionfs/union_vnops.c b/sys/fs/unionfs/union_vnops.c index 09fbccb3d7ff..d09b8cd50854 100644 --- a/sys/fs/unionfs/union_vnops.c +++ b/sys/fs/unionfs/union_vnops.c @@ -961,7 +961,6 @@ unionfs_fsync(struct vop_fsync_args *ap) static int unionfs_remove(struct vop_remove_args *ap) { - int error; char *path; struct unionfs_node *dunp; struct unionfs_node *unp; @@ -973,6 +972,8 @@ unionfs_remove(struct vop_remove_args *ap) struct componentname *cnp; struct componentname cn; struct thread *td; + int error; + int pathlen; UNIONFS_INTERNAL_DEBUG("unionfs_remove: enter\n"); @@ -992,7 +993,7 @@ unionfs_remove(struct vop_remove_args *ap) /* search vnode */ VOP_UNLOCK(ap->a_vp); error = unionfs_relookup(udvp, &vp, cnp, &cn, td, - cnp->cn_nameptr, strlen(cnp->cn_nameptr), DELETE); + cnp->cn_nameptr, cnp->cn_namelen, DELETE); if (error != 0 && error != ENOENT) { vn_lock(ap->a_vp, LK_EXCLUSIVE | LK_RETRY); return (error); @@ -1002,7 +1003,6 @@ unionfs_remove(struct vop_remove_args *ap) /* target vnode in upper */ uvp = vp; vrele(vp); - path = NULL; } else { /* target vnode in lower */ if (vp != NULLVP) { @@ -1013,14 +1013,16 @@ unionfs_remove(struct vop_remove_args *ap) } vn_lock(ap->a_vp, LK_EXCLUSIVE | LK_RETRY); lvp = ap->a_vp; - path = ap->a_cnp->cn_nameptr; } + path = cnp->cn_nameptr; + pathlen = cnp->cn_namelen; } else { ump = MOUNTTOUNIONFSMOUNT(ap->a_vp->v_mount); unp = VTOUNIONFS(ap->a_vp); uvp = unp->un_uppervp; lvp = unp->un_lowervp; path = unp->un_path; + pathlen = unp->un_pathlen; } if (udvp == NULLVP) @@ -1036,7 +1038,7 @@ unionfs_remove(struct vop_remove_args *ap) cnp->cn_flags |= DOWHITEOUT; error = VOP_REMOVE(udvp, uvp, cnp); } else if (lvp != NULLVP) - error = unionfs_mkwhiteout(udvp, cnp, td, path); + error = unionfs_mkwhiteout(udvp, cnp, td, path, pathlen); UNIONFS_INTERNAL_DEBUG("unionfs_remove: leave (%d)\n", error); @@ -1262,7 +1264,7 @@ unionfs_rename(struct vop_rename_args *ap) if (error != 0) goto unionfs_rename_abort; - /* Locke of tvp is canceled in order to avoid recursive lock. */ + /* Lock of tvp is canceled in order to avoid recursive lock. */ if (tvp != NULLVP && tvp != tdvp) VOP_UNLOCK(tvp); error = unionfs_relookup_for_rename(tdvp, tcnp, td); @@ -1421,7 +1423,8 @@ unionfs_rmdir(struct vop_rmdir_args *ap) error = VOP_RMDIR(udvp, uvp, cnp); } else if (lvp != NULLVP) - error = unionfs_mkwhiteout(udvp, cnp, td, unp->un_path); + error = unionfs_mkwhiteout(udvp, cnp, td, + unp->un_path, unp->un_pathlen); if (error == 0) { cache_purge(ap->a_dvp);