git: abe95116ba10 - main - unionfs: rework pathname handling
Jason A. Harmening
jah at FreeBSD.org
Wed Sep 1 14:51:48 UTC 2021
The branch main has been updated by jah:
URL: https://cgit.FreeBSD.org/src/commit/?id=abe95116ba10c8ab56f3ccd72a969ee29a9eef0b
commit abe95116ba10c8ab56f3ccd72a969ee29a9eef0b
Author: Jason A. Harmening <jah at FreeBSD.org>
AuthorDate: 2021-08-29 21:36:15 +0000
Commit: Jason A. Harmening <jah at FreeBSD.org>
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);
More information about the dev-commits-src-all
mailing list