Skip to content

Commit

Permalink
zfs_zaccess_trivial() causes deadlock for xattr attributes
Browse files Browse the repository at this point in the history 
zfs_zaccess_trivial() calls the generic_permission() to read
xattr attributes. This may cause deadlock if xattr and dent
locks were already held. This commit add support to prevent
deadlock if dent_lock() is held from generic_permission()
context.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
  • Loading branch information
@ixhamza
ixhamza committed Dec 4, 2022
1 parent 7764411 commit 11d64be
Show file tree
Hide file tree
Showing 8 changed files with 40 additions and 14 deletions.
3 changes: 2 additions & 1 deletion include/os/freebsd/zfs/sys/zfs_dir.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ extern void zfs_unlinked_add(znode_t *, dmu_tx_t *);
extern void zfs_unlinked_drain(zfsvfs_t *zfsvfs);
extern int zfs_sticky_remove_access(znode_t *, znode_t *, cred_t *cr);
extern int zfs_get_xattrdir(znode_t *, znode_t **, cred_t *, int);
extern int zfs_make_xattrdir(znode_t *, vattr_t *, znode_t **, cred_t *);
extern int zfs_make_xattrdir(znode_t *, vattr_t *, znode_t **, cred_t *,
boolean_t);

#ifdef __cplusplus
}
Expand Down
3 changes: 2 additions & 1 deletion include/os/linux/zfs/sys/zfs_dir.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ extern void zfs_unlinked_drain(zfsvfs_t *zfsvfs);
extern void zfs_unlinked_drain_stop_wait(zfsvfs_t *zfsvfs);
extern int zfs_sticky_remove_access(znode_t *, znode_t *, cred_t *cr);
extern int zfs_get_xattrdir(znode_t *, znode_t **, cred_t *, int);
extern int zfs_make_xattrdir(znode_t *, vattr_t *, znode_t **, cred_t *);
extern int zfs_make_xattrdir(znode_t *, vattr_t *, znode_t **, cred_t *,
boolean_t skip_acl);

#ifdef __cplusplus
}
Expand Down
5 changes: 3 additions & 2 deletions module/os/freebsd/zfs/zfs_dir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,8 @@ zfs_dirempty(znode_t *dzp)
}

int
zfs_make_xattrdir(znode_t *zp, vattr_t *vap, znode_t **xvpp, cred_t *cr)
zfs_make_xattrdir(znode_t *zp, vattr_t *vap, znode_t **xvpp, cred_t *cr,
boolean_t skip_acl)
{
zfsvfs_t *zfsvfs = zp->z_zfsvfs;
znode_t *xzp;
Expand Down Expand Up @@ -911,7 +912,7 @@ zfs_get_xattrdir(znode_t *zp, znode_t **xzpp, cred_t *cr, int flags)
va.va_mode = S_IFDIR | S_ISVTX | 0777;
zfs_fuid_map_ids(zp, cr, &va.va_uid, &va.va_gid);

error = zfs_make_xattrdir(zp, &va, xzpp, cr);
error = zfs_make_xattrdir(zp, &va, xzpp, cr, B_FALSE);

if (error == ERESTART) {
/* NB: we already did dmu_tx_wait() if necessary */
Expand Down
8 changes: 5 additions & 3 deletions module/os/linux/zfs/zfs_dir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1098,7 +1098,8 @@ zfs_dirempty(znode_t *dzp)
}

int
zfs_make_xattrdir(znode_t *zp, vattr_t *vap, znode_t **xzpp, cred_t *cr)
zfs_make_xattrdir(znode_t *zp, vattr_t *vap, znode_t **xzpp, cred_t *cr,
boolean_t skip_acl)
{
zfsvfs_t *zfsvfs = ZTOZSB(zp);
znode_t *xzp;
Expand All @@ -1112,7 +1113,7 @@ zfs_make_xattrdir(znode_t *zp, vattr_t *vap, znode_t **xzpp, cred_t *cr)

*xzpp = NULL;

if ((error = zfs_zaccess(zp, ACE_WRITE_NAMED_ATTRS, 0, B_FALSE, cr,
if ((error = zfs_zaccess(zp, ACE_WRITE_NAMED_ATTRS, 0, skip_acl, cr,
kcred->user_ns)))
return (error);

Expand Down Expand Up @@ -1185,6 +1186,7 @@ zfs_get_xattrdir(znode_t *zp, znode_t **xzpp, cred_t *cr, int flags)
zfs_dirlock_t *dl;
vattr_t va;
int error;
boolean_t skip_acl = (flags & ATTR_NOACLCHECK) ? B_TRUE : B_FALSE;
top:
error = zfs_dirent_lock(&dl, zp, "", &xzp, ZXATTR, NULL, NULL);
if (error)
Expand Down Expand Up @@ -1221,7 +1223,7 @@ zfs_get_xattrdir(znode_t *zp, znode_t **xzpp, cred_t *cr, int flags)
zfs_fuid_map_ids(zp, cr, &va.va_uid, &va.va_gid);

va.va_dentry = NULL;
error = zfs_make_xattrdir(zp, &va, xzpp, cr);
error = zfs_make_xattrdir(zp, &va, xzpp, cr, skip_acl);
zfs_dirent_unlock(dl);

if (error == ERESTART) {
Expand Down
3 changes: 2 additions & 1 deletion module/os/linux/zfs/zfs_vnops_os.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,7 @@ zfs_create(znode_t *dzp, char *name, vattr_t *vap, int excl,
boolean_t fuid_dirtied;
boolean_t have_acl = B_FALSE;
boolean_t waited = B_FALSE;
boolean_t skipaclchk = (flag & ATTR_NOACLCHECK) ?B_TRUE :B_FALSE;

/*
* If we have an ephemeral id, ACL, or XVATTR then
Expand Down Expand Up @@ -627,7 +628,7 @@ zfs_create(znode_t *dzp, char *name, vattr_t *vap, int excl,
* Create a new file object and update the directory
* to reference it.
*/
if ((error = zfs_zaccess(dzp, ACE_ADD_FILE, 0, B_FALSE, cr,
if ((error = zfs_zaccess(dzp, ACE_ADD_FILE, 0, skipaclchk, cr,
mnt_ns))) {
if (have_acl)
zfs_acl_ids_free(&acl_ids);
Expand Down
28 changes: 23 additions & 5 deletions module/os/linux/zfs/zpl_xattr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,14 +433,25 @@ zpl_xattr_get(struct inode *ip, const char *name, void *value, size_t size)
cred_t *cr = CRED();
fstrans_cookie_t cookie;
int error;
boolean_t locked_xattr = B_FALSE;

crhold(cr);
cookie = spl_fstrans_mark();
if ((error = zpl_enter_verify_zp(zfsvfs, zp, FTAG)) != 0)
goto out;
rw_enter(&zp->z_xattr_lock, RW_READER);

/*
* generic_permission() may end up calling this function
* with z_xattr_lock lock held. We skip reacquiring the
* same lock in order to avoid the deadlock.
*/
if (!RW_WRITE_HELD(&zp->z_xattr_lock)) {
rw_enter(&zp->z_xattr_lock, RW_READER);
locked_xattr = B_TRUE;
}
error = __zpl_xattr_get(ip, name, value, size, cr);
rw_exit(&zp->z_xattr_lock);
if (locked_xattr == B_TRUE)
rw_exit(&zp->z_xattr_lock);
zpl_exit(zfsvfs, FTAG);
out:
spl_fstrans_unmark(cookie);
Expand All @@ -459,18 +470,23 @@ zpl_xattr_set_dir(struct inode *ip, const char *name, const void *value,
int lookup_flags, error;
const int xattr_mode = S_IFREG | 0644;
loff_t pos = 0;
znode_t *zp = ITOZ(ip);
int flag_dir = LOOKUP_XATTR;
int flag_xattr = 0;

/*
* Lookup the xattr directory. When we're adding an entry pass
* CREATE_XATTR_DIR to ensure the xattr directory is created.
* When removing an entry this flag is not passed to avoid
* unnecessarily creating a new xattr directory.
*/
lookup_flags = LOOKUP_XATTR;
if (ZTOZSB(zp)->z_acl_type == ZFS_ACLTYPE_POSIX)
flag_dir |= ATTR_NOACLCHECK;
lookup_flags = flag_dir;
if (value != NULL)
lookup_flags |= CREATE_XATTR_DIR;

error = -zfs_lookup(ITOZ(ip), NULL, &dxzp, lookup_flags,
error = -zfs_lookup(zp, NULL, &dxzp, lookup_flags,
cr, NULL, NULL);
if (error)
goto out;
Expand Down Expand Up @@ -498,8 +514,10 @@ zpl_xattr_set_dir(struct inode *ip, const char *name, const void *value,
vap->va_uid = crgetuid(cr);
vap->va_gid = crgetgid(cr);

if (ZTOZSB(dxzp)->z_acl_type == ZFS_ACLTYPE_POSIX)
flag_xattr |= ATTR_NOACLCHECK;
error = -zfs_create(dxzp, (char *)name, vap, 0, 0644, &xzp,
cr, 0, NULL, kcred->user_ns);
cr, flag_xattr, NULL, kcred->user_ns);
if (error)
goto out;
}
Expand Down
3 changes: 2 additions & 1 deletion module/zfs/zfs_replay.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,8 @@ zfs_replay_create(void *arg1, void *arg2, boolean_t byteswap)

break;
case TX_MKXATTR:
error = zfs_make_xattrdir(dzp, &xva.xva_vattr, &zp, kcred);
error = zfs_make_xattrdir(dzp, &xva.xva_vattr, &zp, kcred,
B_FALSE);
break;
case TX_SYMLINK:
name = (char *)(lr + 1);
Expand Down
1 change: 1 addition & 0 deletions tests/zfs-tests/tests/functional/acl/posix/posix_004_pos.ksh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ log_must setfacl -d -m u:$ZFS_ACL_STAFF1:rwx $TESTDIR
log_must setfacl -b $TESTDIR

log_must chown $ZFS_ACL_STAFF1 $TESTDIR
log_must setfacl -d -m u:$ZFS_ACL_STAFF1:rwx $TESTDIR
log_must chown 0 $TESTDIR

log_pass "chown works with POSIX ACLs"

0 comments on commit 11d64be

Please sign in to comment.