From a08add3067e5ebecbae6a7da5bd967c5dbbf2ef2 Mon Sep 17 00:00:00 2001 From: Chunwei Chen Date: Wed, 30 Dec 2015 15:47:11 -0800 Subject: [PATCH] Prevent duplicated xattr between SA and dir When replacing an xattr would cause overflowing in SA, we would fallback to xattr dir. However, current implementation don't clear the one in SA, so we would end up with duplicated SA. For example, running the following script on an xattr=sa filesystem would cause duplicated "user.1". -- dup_xattr.sh begin -- randbase64() { dd if=/dev/urandom bs=1 count=$1 2>/dev/null | openssl enc -a -A } file=$1 touch $file setfattr -h -n user.1 -v `randbase64 5000` $file setfattr -h -n user.2 -v `randbase64 20000` $file setfattr -h -n user.3 -v `randbase64 20000` $file setfattr -h -n user.1 -v `randbase64 20000` $file getfattr -m. -d $file -- dup_xattr.sh end -- Also, when a filesystem is switch from xattr=sa to xattr=on, it will never modify those in SA. This would cause strange behavior like, you cannot delete an xattr, or setxattr would cause duplicate and the result would not match when you getxattr. For example, the following shell sequence. -- shell begin -- $ sudo zfs set xattr=sa pp/fs0 $ touch zzz $ setfattr -n user.test -v asdf zzz $ sudo zfs set xattr=on pp/fs0 $ setfattr -x user.test zzz setfattr: zzz: No such attribute $ getfattr -d zzz user.test="asdf" $ setfattr -n user.test -v zxcv zzz $ getfattr -d zzz user.test="asdf" user.test="asdf" -- shell end -- We fix this behavior, by first finding where the xattr resides before setxattr. Then, after we successfully updated the xattr in one location, we will clear the other location. Note that, because update and clear are not in single tx, we could still end up with duplicated xattr. But by doing setxattr again, it can be fixed. Signed-off-by: Chunwei Chen Closes #3472 Closes #4153 --- module/zfs/zpl_xattr.c | 73 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 5 deletions(-) diff --git a/module/zfs/zpl_xattr.c b/module/zfs/zpl_xattr.c index 253ea96617..e39d94eaaa 100644 --- a/module/zfs/zpl_xattr.c +++ b/module/zfs/zpl_xattr.c @@ -337,6 +337,45 @@ out: return (error); } +#define XATTR_NOENT 0x0 +#define XATTR_IN_SA 0x1 +#define XATTR_IN_DIR 0x2 +/* check where the xattr resides */ +static int +__zpl_xattr_where(struct inode *ip, const char *name, int *where, cred_t *cr) +{ + znode_t *zp = ITOZ(ip); + zfs_sb_t *zsb = ZTOZSB(zp); + int error; + + ASSERT(where); + ASSERT(RW_LOCK_HELD(&zp->z_xattr_lock)); + + *where = XATTR_NOENT; + if (zsb->z_use_sa && zp->z_is_sa) { + error = zpl_xattr_get_sa(ip, name, NULL, 0); + if (error >= 0) + *where |= XATTR_IN_SA; + else if (error != -ENOENT) + return (error); + } + + error = zpl_xattr_get_dir(ip, name, NULL, 0, cr); + if (error >= 0) + *where |= XATTR_IN_DIR; + else if (error != -ENOENT) + return (error); + + if (*where == (XATTR_IN_SA|XATTR_IN_DIR)) + cmn_err(CE_WARN, "ZFS: inode %p has xattr \"%s\"" + " in both SA and dir", ip, name); + if (*where == XATTR_NOENT) + error = -ENODATA; + else + error = 0; + return (error); +} + static int zpl_xattr_get(struct inode *ip, const char *name, void *value, size_t size) { @@ -449,7 +488,15 @@ zpl_xattr_set_sa(struct inode *ip, const char *name, const void *value, znode_t *zp = ITOZ(ip); nvlist_t *nvl; size_t sa_size; - int error; + int error = 0; + + mutex_enter(&zp->z_lock); + if (zp->z_xattr_cached == NULL) + error = -zfs_sa_get_xattr(zp); + mutex_exit(&zp->z_lock); + + if (error) + return (error); ASSERT(zp->z_xattr_cached); nvl = zp->z_xattr_cached; @@ -501,6 +548,7 @@ zpl_xattr_set(struct inode *ip, const char *name, const void *value, zfs_sb_t *zsb = ZTOZSB(zp); cred_t *cr = CRED(); fstrans_cookie_t cookie; + int where; int error; crhold(cr); @@ -514,12 +562,14 @@ zpl_xattr_set(struct inode *ip, const char *name, const void *value, * * XATTR_CREATE: fail if xattr already exists * XATTR_REPLACE: fail if xattr does not exist + * + * We also want to know if it resides in sa or dir, so we can make + * sure we don't end up with duplicate in both places. */ - error = __zpl_xattr_get(ip, name, NULL, 0, cr); + error = __zpl_xattr_where(ip, name, &where, cr); if (error < 0) { if (error != -ENODATA) goto out; - if (flags & XATTR_REPLACE) goto out; @@ -534,13 +584,26 @@ zpl_xattr_set(struct inode *ip, const char *name, const void *value, } /* Preferentially store the xattr as a SA for better performance */ - if (zsb->z_use_sa && zsb->z_xattr_sa && zp->z_is_sa) { + if (zsb->z_use_sa && zp->z_is_sa && + (zsb->z_xattr_sa || (value == NULL && where & XATTR_IN_SA))) { error = zpl_xattr_set_sa(ip, name, value, size, flags, cr); - if (error == 0) + if (error == 0) { + /* + * Successfully put into SA, we need to clear the one + * in dir. + */ + if (where & XATTR_IN_DIR) + zpl_xattr_set_dir(ip, name, NULL, 0, 0, cr); goto out; + } } error = zpl_xattr_set_dir(ip, name, value, size, flags, cr); + /* + * Successfully put into dir, we need to clear the one in SA. + */ + if (error == 0 && (where & XATTR_IN_SA)) + zpl_xattr_set_sa(ip, name, NULL, 0, 0, cr); out: rw_exit(&ITOZ(ip)->z_xattr_lock); rrm_exit(&(zsb)->z_teardown_lock, FTAG);