From c6dab6dd39214d587c7013e8c6dfeb085e3eb41c Mon Sep 17 00:00:00 2001 From: Chunwei Chen Date: Thu, 19 Jan 2023 16:59:05 -0800 Subject: [PATCH] Fix unprotected zfs_znode_dmu_fini In original code, zfs_znode_dmu_fini is called in zfs_rmnode without zfs_znode_hold_enter. It seems to assume it's ok to do so when the znode is unlinked. However this assumption is not correct, as zfs_zget can be called by NFS through zpl_fh_to_dentry as pointed out by Christian in https://github.com/openzfs/zfs/pull/12767, which could result in a use-after-free bug. Reviewed-by: Brian Behlendorf Co-authored-by: Ryan Moeller Signed-off-by: Chunwei Chen Signed-off-by: Ryan Moeller Closes #12767 Closes #14364 --- include/sys/zfs_znode.h | 2 ++ module/os/freebsd/zfs/zfs_dir.c | 7 +++++++ module/os/freebsd/zfs/zfs_znode.c | 1 - module/os/linux/zfs/zfs_dir.c | 9 ++++++++- module/os/linux/zfs/zfs_znode.c | 6 +++--- 5 files changed, 20 insertions(+), 5 deletions(-) diff --git a/include/sys/zfs_znode.h b/include/sys/zfs_znode.h index 3a4a0c3cb5..de38f56dc3 100644 --- a/include/sys/zfs_znode.h +++ b/include/sys/zfs_znode.h @@ -272,6 +272,8 @@ extern int zfs_freesp(znode_t *, uint64_t, uint64_t, int, boolean_t); extern void zfs_znode_init(void); extern void zfs_znode_fini(void); extern int zfs_znode_hold_compare(const void *, const void *); +extern znode_hold_t *zfs_znode_hold_enter(zfsvfs_t *, uint64_t); +extern void zfs_znode_hold_exit(zfsvfs_t *, znode_hold_t *); extern int zfs_zget(zfsvfs_t *, uint64_t, znode_t **); extern int zfs_rezget(znode_t *); extern void zfs_zinactive(znode_t *); diff --git a/module/os/freebsd/zfs/zfs_dir.c b/module/os/freebsd/zfs/zfs_dir.c index 07232086d5..948df8e50d 100644 --- a/module/os/freebsd/zfs/zfs_dir.c +++ b/module/os/freebsd/zfs/zfs_dir.c @@ -426,6 +426,7 @@ zfs_rmnode(znode_t *zp) zfsvfs_t *zfsvfs = zp->z_zfsvfs; objset_t *os = zfsvfs->z_os; dmu_tx_t *tx; + uint64_t z_id = zp->z_id; uint64_t acl_obj; uint64_t xattr_obj; uint64_t count; @@ -445,8 +446,10 @@ zfs_rmnode(znode_t *zp) * Not enough space to delete some xattrs. * Leave it in the unlinked set. */ + ZFS_OBJ_HOLD_ENTER(zfsvfs, z_id); zfs_znode_dmu_fini(zp); zfs_znode_free(zp); + ZFS_OBJ_HOLD_EXIT(zfsvfs, z_id); return; } } else { @@ -464,8 +467,10 @@ zfs_rmnode(znode_t *zp) * Not enough space or we were interrupted by unmount. * Leave the file in the unlinked set. */ + ZFS_OBJ_HOLD_ENTER(zfsvfs, z_id); zfs_znode_dmu_fini(zp); zfs_znode_free(zp); + ZFS_OBJ_HOLD_EXIT(zfsvfs, z_id); return; } } @@ -501,8 +506,10 @@ zfs_rmnode(znode_t *zp) * which point we'll call zfs_unlinked_drain() to process it). */ dmu_tx_abort(tx); + ZFS_OBJ_HOLD_ENTER(zfsvfs, z_id); zfs_znode_dmu_fini(zp); zfs_znode_free(zp); + ZFS_OBJ_HOLD_EXIT(zfsvfs, z_id); return; } diff --git a/module/os/freebsd/zfs/zfs_znode.c b/module/os/freebsd/zfs/zfs_znode.c index e691e60a8f..76ae09f811 100644 --- a/module/os/freebsd/zfs/zfs_znode.c +++ b/module/os/freebsd/zfs/zfs_znode.c @@ -386,7 +386,6 @@ void zfs_znode_dmu_fini(znode_t *zp) { ASSERT(MUTEX_HELD(ZFS_OBJ_MUTEX(zp->z_zfsvfs, zp->z_id)) || - zp->z_unlinked || ZFS_TEARDOWN_INACTIVE_WRITE_HELD(zp->z_zfsvfs)); sa_handle_destroy(zp->z_sa_hdl); diff --git a/module/os/linux/zfs/zfs_dir.c b/module/os/linux/zfs/zfs_dir.c index 09eac37f9e..1fec4ea093 100644 --- a/module/os/linux/zfs/zfs_dir.c +++ b/module/os/linux/zfs/zfs_dir.c @@ -649,6 +649,8 @@ zfs_rmnode(znode_t *zp) objset_t *os = zfsvfs->z_os; znode_t *xzp = NULL; dmu_tx_t *tx; + znode_hold_t *zh; + uint64_t z_id = zp->z_id; uint64_t acl_obj; uint64_t xattr_obj; uint64_t links; @@ -666,8 +668,9 @@ zfs_rmnode(znode_t *zp) * Not enough space to delete some xattrs. * Leave it in the unlinked set. */ + zh = zfs_znode_hold_enter(zfsvfs, z_id); zfs_znode_dmu_fini(zp); - + zfs_znode_hold_exit(zfsvfs, zh); return; } } @@ -686,7 +689,9 @@ zfs_rmnode(znode_t *zp) * Not enough space or we were interrupted by unmount. * Leave the file in the unlinked set. */ + zh = zfs_znode_hold_enter(zfsvfs, z_id); zfs_znode_dmu_fini(zp); + zfs_znode_hold_exit(zfsvfs, zh); return; } } @@ -726,7 +731,9 @@ zfs_rmnode(znode_t *zp) * which point we'll call zfs_unlinked_drain() to process it). */ dmu_tx_abort(tx); + zh = zfs_znode_hold_enter(zfsvfs, z_id); zfs_znode_dmu_fini(zp); + zfs_znode_hold_exit(zfsvfs, zh); goto out; } diff --git a/module/os/linux/zfs/zfs_znode.c b/module/os/linux/zfs/zfs_znode.c index 395e2cf57e..1faf25d93c 100644 --- a/module/os/linux/zfs/zfs_znode.c +++ b/module/os/linux/zfs/zfs_znode.c @@ -271,7 +271,7 @@ zfs_znode_held(zfsvfs_t *zfsvfs, uint64_t obj) return (held); } -static znode_hold_t * +znode_hold_t * zfs_znode_hold_enter(zfsvfs_t *zfsvfs, uint64_t obj) { znode_hold_t *zh, *zh_new, search; @@ -304,7 +304,7 @@ zfs_znode_hold_enter(zfsvfs_t *zfsvfs, uint64_t obj) return (zh); } -static void +void zfs_znode_hold_exit(zfsvfs_t *zfsvfs, znode_hold_t *zh) { int i = ZFS_OBJ_HASH(zfsvfs, zh->zh_obj); @@ -357,7 +357,7 @@ zfs_znode_sa_init(zfsvfs_t *zfsvfs, znode_t *zp, void zfs_znode_dmu_fini(znode_t *zp) { - ASSERT(zfs_znode_held(ZTOZSB(zp), zp->z_id) || zp->z_unlinked || + ASSERT(zfs_znode_held(ZTOZSB(zp), zp->z_id) || RW_WRITE_HELD(&ZTOZSB(zp)->z_teardown_inactive_lock)); sa_handle_destroy(zp->z_sa_hdl);