Hi Christoph,
Looks reasonable to me - comparing xattr_permission() with
attr_user_capable() and attr_trusted_capable().
Also the xattr_permission() makes more sense with its MAY_WRITE
testing otherwise it looks like we'd be doing some unfair
tests in the getxattr case.
I'll run thru qa and check in soon.
--Tim
Christoph Hellwig wrote:
> No need for addition permission checks in the xattr handler,
> fs/xattr.c:xattr_permission() already does them, and in fact slightly
> more strict then what was in the attr_capable handlers.
>
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c 2008-04-29
> 21:32:56.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c 2008-04-29 21:33:30.000000000
> +0200
> @@ -747,15 +747,11 @@ xfs_vn_setxattr(
> char *attr = (char *)name;
> attrnames_t *namesp;
> int xflags = 0;
> - int error;
>
> namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
> if (!namesp)
> return -EOPNOTSUPP;
> attr += namesp->attr_namelen;
> - error = namesp->attr_capable(vp, NULL);
> - if (error)
> - return error;
>
> /* Convert Linux syscall to XFS internal ATTR flags */
> if (flags & XATTR_CREATE)
> @@ -777,15 +773,11 @@ xfs_vn_getxattr(
> char *attr = (char *)name;
> attrnames_t *namesp;
> int xflags = 0;
> - ssize_t error;
>
> namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
> if (!namesp)
> return -EOPNOTSUPP;
> attr += namesp->attr_namelen;
> - error = namesp->attr_capable(vp, NULL);
> - if (error)
> - return error;
>
> /* Convert Linux syscall to XFS internal ATTR flags */
> if (!size) {
> @@ -825,15 +817,12 @@ xfs_vn_removexattr(
> char *attr = (char *)name;
> attrnames_t *namesp;
> int xflags = 0;
> - int error;
>
> namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
> if (!namesp)
> return -EOPNOTSUPP;
> attr += namesp->attr_namelen;
> - error = namesp->attr_capable(vp, NULL);
> - if (error)
> - return error;
> +
> xflags |= namesp->attr_flag;
> return namesp->attr_remove(vp, attr, xflags);
> }
> Index: linux-2.6-xfs/fs/xfs/xfs_attr.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_attr.c 2008-04-29 21:32:25.000000000
> +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_attr.c 2008-04-29 21:32:51.000000000 +0200
> @@ -2622,43 +2622,6 @@ attr_lookup_namespace(
> return NULL;
> }
>
> -/*
> - * Some checks to prevent people abusing EAs to get over quota:
> - * - Don't allow modifying user EAs on devices/symlinks;
> - * - Don't allow modifying user EAs if sticky bit set;
> - */
> -STATIC int
> -attr_user_capable(
> - bhv_vnode_t *vp,
> - cred_t *cred)
> -{
> - struct inode *inode = vn_to_inode(vp);
> -
> - if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> - return -EPERM;
> - if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode) &&
> - !capable(CAP_SYS_ADMIN))
> - return -EPERM;
> - if (S_ISDIR(inode->i_mode) && (inode->i_mode & S_ISVTX) &&
> - (current_fsuid(cred) != inode->i_uid) && !capable(CAP_FOWNER))
> - return -EPERM;
> - return 0;
> -}
> -
> -STATIC int
> -attr_trusted_capable(
> - bhv_vnode_t *vp,
> - cred_t *cred)
> -{
> - struct inode *inode = vn_to_inode(vp);
> -
> - if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> - return -EPERM;
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> - return 0;
> -}
> -
> STATIC int
> attr_system_set(
> bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> @@ -2709,7 +2672,6 @@ struct attrnames attr_system = {
> .attr_get = attr_system_get,
> .attr_set = attr_system_set,
> .attr_remove = attr_system_remove,
> - .attr_capable = (attrcapable_t)fs_noerr,
> };
>
> struct attrnames attr_trusted = {
> @@ -2719,7 +2681,6 @@ struct attrnames attr_trusted = {
> .attr_get = attr_generic_get,
> .attr_set = attr_generic_set,
> .attr_remove = attr_generic_remove,
> - .attr_capable = attr_trusted_capable,
> };
>
> struct attrnames attr_secure = {
> @@ -2729,7 +2690,6 @@ struct attrnames attr_secure = {
> .attr_get = attr_generic_get,
> .attr_set = attr_generic_set,
> .attr_remove = attr_generic_remove,
> - .attr_capable = (attrcapable_t)fs_noerr,
> };
>
> struct attrnames attr_user = {
> @@ -2738,7 +2698,6 @@ struct attrnames attr_user = {
> .attr_get = attr_generic_get,
> .attr_set = attr_generic_set,
> .attr_remove = attr_generic_remove,
> - .attr_capable = attr_user_capable,
> };
>
> struct attrnames *attr_namespaces[] =
> Index: linux-2.6-xfs/fs/xfs/xfs_attr.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_attr.h 2008-04-29 21:33:38.000000000
> +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_attr.h 2008-04-29 21:33:52.000000000 +0200
> @@ -42,7 +42,6 @@ typedef int (*attrset_t)(bhv_vnode_t *,
> typedef int (*attrget_t)(bhv_vnode_t *, char *, void *, size_t, int);
> typedef int (*attrremove_t)(bhv_vnode_t *, char *, int);
> typedef int (*attrexists_t)(bhv_vnode_t *);
> -typedef int (*attrcapable_t)(bhv_vnode_t *, struct cred *);
>
> typedef struct attrnames {
> char * attr_name;
> @@ -52,7 +51,6 @@ typedef struct attrnames {
> attrset_t attr_set;
> attrremove_t attr_remove;
> attrexists_t attr_exists;
> - attrcapable_t attr_capable;
> } attrnames_t;
>
> #define ATTR_NAMECOUNT 4
>
|