From 65588fd503870d308c8350c2edc41a69dcae0c7e Mon Sep 17 00:00:00 2001 From: Edward Tomasz Napierala Date: Mon, 20 Jul 2009 19:16:42 +0000 Subject: [PATCH] Fix permission handling for extended attributes in ZFS. Without this change, ZFS uses SunOS Alternate Data Streams semantics - each EA has its own permissions, which are set at EA creation time and - unlike SunOS - invisible to the user and impossible to change. From the user point of view, it's just broken: sometimes access is granted when it shouldn't be, sometimes it's denied when it shouldn't be. This patch makes it behave just like UFS, i.e. depend on current file permissions. Also, it fixes returned error codes (ENOATTR instead of ENOENT) and makes listextattr(2) return 0 instead of EPERM where there is no EA directory (i.e. the file never had any EA). Reviewed by: pjd (idea, not actual code) Approved by: re (kib) --- .../opensolaris/uts/common/fs/zfs/zfs_acl.c | 10 ++++++ .../opensolaris/uts/common/fs/zfs/zfs_dir.c | 12 +++++-- .../opensolaris/uts/common/fs/zfs/zfs_vnops.c | 32 ++++++++++++++++++- 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c index dfbebc240362..d46627346a11 100644 --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c @@ -2360,6 +2360,15 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr) is_attr = ((zp->z_phys->zp_flags & ZFS_XATTR) && (ZTOV(zp)->v_type == VDIR)); +#ifdef __FreeBSD__ + /* + * In FreeBSD, we don't care about permissions of individual ADS. + * Note that not checking them is not just an optimization - without + * this shortcut, EA operations may bogusly fail with EACCES. + */ + if (zp->z_phys->zp_flags & ZFS_XATTR) + return (0); +#else /* * If attribute then validate against base file */ @@ -2385,6 +2394,7 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr) mode |= ACE_READ_NAMED_ATTRS; } } +#endif if ((error = zfs_zaccess_common(check_zp, mode, &working_mode, &check_privs, skipaclchk, cr)) == 0) { diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_dir.c b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_dir.c index 3ae43f1684b1..10d3b8671083 100644 --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_dir.c +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_dir.c @@ -831,8 +831,14 @@ zfs_make_xattrdir(znode_t *zp, vattr_t *vap, vnode_t **xvpp, cred_t *cr) *xvpp = NULL; + /* + * In FreeBSD, access checking for creating an EA is being done + * in zfs_setextattr(), + */ +#ifndef __FreeBSD__ if (error = zfs_zaccess(zp, ACE_WRITE_NAMED_ATTRS, 0, B_FALSE, cr)) return (error); +#endif tx = dmu_tx_create(zfsvfs->z_os); dmu_tx_hold_bonus(tx, zp->z_id); @@ -906,12 +912,14 @@ top: ASSERT(zp->z_phys->zp_xattr == 0); -#ifdef TODO if (!(flags & CREATE_XATTR_DIR)) { zfs_dirent_unlock(dl); +#ifdef __FreeBSD__ + return (ENOATTR); +#else return (ENOENT); - } #endif + } if (zfsvfs->z_vfs->vfs_flag & VFS_RDONLY) { zfs_dirent_unlock(dl); diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c index 2cf4304dc811..3f0e808bbb3f 100644 --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c @@ -4502,6 +4502,11 @@ vop_getextattr { vnode_t *xvp = NULL, *vp; int error, flags; + error = extattr_check_cred(ap->a_vp, ap->a_attrnamespace, + ap->a_cred, ap->a_td, VREAD); + if (error != 0) + return (error); + error = zfs_create_attrname(ap->a_attrnamespace, ap->a_name, attrname, sizeof(attrname)); if (error != 0) @@ -4523,6 +4528,8 @@ vop_getextattr { vp = nd.ni_vp; NDFREE(&nd, NDF_ONLY_PNBUF); if (error != 0) { + if (error == ENOENT) + error = ENOATTR; ZFS_EXIT(zfsvfs); return (error); } @@ -4564,6 +4571,11 @@ vop_deleteextattr { vnode_t *xvp = NULL, *vp; int error, flags; + error = extattr_check_cred(ap->a_vp, ap->a_attrnamespace, + ap->a_cred, ap->a_td, VWRITE); + if (error != 0) + return (error); + error = zfs_create_attrname(ap->a_attrnamespace, ap->a_name, attrname, sizeof(attrname)); if (error != 0) @@ -4584,6 +4596,8 @@ vop_deleteextattr { vp = nd.ni_vp; NDFREE(&nd, NDF_ONLY_PNBUF); if (error != 0) { + if (error == ENOENT) + error = ENOATTR; ZFS_EXIT(zfsvfs); return (error); } @@ -4623,6 +4637,11 @@ vop_setextattr { vnode_t *xvp = NULL, *vp; int error, flags; + error = extattr_check_cred(ap->a_vp, ap->a_attrnamespace, + ap->a_cred, ap->a_td, VWRITE); + if (error != 0) + return (error); + error = zfs_create_attrname(ap->a_attrnamespace, ap->a_name, attrname, sizeof(attrname)); if (error != 0) @@ -4631,7 +4650,7 @@ vop_setextattr { ZFS_ENTER(zfsvfs); error = zfs_lookup(ap->a_vp, NULL, &xvp, NULL, 0, ap->a_cred, td, - LOOKUP_XATTR); + LOOKUP_XATTR | CREATE_XATTR_DIR); if (error != 0) { ZFS_EXIT(zfsvfs); return (error); @@ -4690,6 +4709,11 @@ vop_listextattr { vnode_t *xvp = NULL, *vp; int done, error, eof, pos; + error = extattr_check_cred(ap->a_vp, ap->a_attrnamespace, + ap->a_cred, ap->a_td, VREAD); + if (error) + return (error); + error = zfs_create_attrname(ap->a_attrnamespace, "", attrprefix, sizeof(attrprefix)); if (error != 0) @@ -4701,6 +4725,12 @@ vop_listextattr { error = zfs_lookup(ap->a_vp, NULL, &xvp, NULL, 0, ap->a_cred, td, LOOKUP_XATTR); if (error != 0) { + /* + * ENOATTR means that the EA directory does not yet exist, + * i.e. there are no extended attributes there. + */ + if (error == ENOATTR) + error = 0; ZFS_EXIT(zfsvfs); return (error); }