On 06/14/2013 03:41 PM, Carlos Maiolino wrote:
> XFS removes sgid bits of subdirectories created under a directory containing a
> default acl.
>
> When a default acl is set, it implies xfs to call xfs_setattr_nonsize() in its
> code path. Such function is shared among mkdir and chmod system calls, and
> does some checks unneeded by mkdir (calling inode_change_ok()). Such checks
> remove sgid bit from the inode after it has been granted. So, this patch wraps
> up these checks to be used only in chmod path.
>
> Also, chmod should not remove sgid bit from an inode if this is a directory,
> so,
> it adds a check if S_ISDIR is set in the inode mode, into
> xfs_setattr_nonsize()
>
> Done that, xfs_setattr_mode() doesn't need to re-check for group id and
> capabilities permissions, this only implies in another try to remove sgid bit
> from the directories. Such check is already done either on inode_change_ok()
> or
> xfs_setattr_nonsize().
>
> This addresses SGI bug 280:
> http://oss.sgi.com/bugzilla/show_bug.cgi?id=280
>
> Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> ---
Hi Carlos,
A couple comments, mostly aesthetic...
> fs/xfs/xfs_iops.c | 51 ++++++++++++++++++++++++++++++++++-----------------
> 1 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ca9ecaa..5c9c505 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -467,9 +467,6 @@ xfs_setattr_mode(
> ASSERT(tp);
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> - if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> - mode &= ~S_ISGID;
> -
> ip->i_d.di_mode &= S_IFMT;
> ip->i_d.di_mode |= mode & ~S_IFMT;
>
> @@ -477,23 +474,20 @@ xfs_setattr_mode(
> inode->i_mode |= mode & ~S_IFMT;
> }
>
> +/* Check inode permissions and filesystem errors
> + *
> + * Wrapper to some pre-checks needed while changing
> + * inode attributes
> + */
> int
> -xfs_setattr_nonsize(
> +xfs_setattr_setup(
> struct xfs_inode *ip,
> struct iattr *iattr,
> int flags)
> {
> xfs_mount_t *mp = ip->i_mount;
> struct inode *inode = VFS_I(ip);
> - int mask = iattr->ia_valid;
> - xfs_trans_t *tp;
> int error;
> - uid_t uid = 0, iuid = 0;
> - gid_t gid = 0, igid = 0;
> - struct xfs_dquot *udqp = NULL, *gdqp = NULL;
> - struct xfs_dquot *olddquot1 = NULL, *olddquot2 = NULL;
> -
> - trace_xfs_setattr(ip);
>
> if (mp->m_flags & XFS_MOUNT_RDONLY)
> return XFS_ERROR(EROFS);
> @@ -505,6 +499,27 @@ xfs_setattr_nonsize(
> if (error)
> return XFS_ERROR(error);
>
Is it safe to package up these other checks as well out of that inherit
code path (i.e., the read-only and shutdown check)?
> + return xfs_setattr_nonsize(ip, iattr, flags);
It seems a little confusing (IMO) for a _setup() function to go and do
work by calling xfs_setattr_nonsize(). What about creating the helper
check function, but calling it and xfs_attr_nonsize() from
xfs_vn_setattr() (assuming _setup() doesn't return an error)?
Brian
> +}
> +
> +int
> +xfs_setattr_nonsize(
> + struct xfs_inode *ip,
> + struct iattr *iattr,
> + int flags)
> +{
> + xfs_mount_t *mp = ip->i_mount;
> + struct inode *inode = VFS_I(ip);
> + int mask = iattr->ia_valid;
> + xfs_trans_t *tp;
> + int error;
> + uid_t uid = 0, iuid = 0;
> + gid_t gid = 0, igid = 0;
> + struct xfs_dquot *udqp = NULL, *gdqp = NULL;
> + struct xfs_dquot *olddquot1 = NULL, *olddquot2 = NULL;
> +
> + trace_xfs_setattr(ip);
> +
> ASSERT((mask & ATTR_SIZE) == 0);
>
> /*
> @@ -592,11 +607,13 @@ xfs_setattr_nonsize(
> * CAP_FSETID overrides the following restrictions:
> *
> * The set-user-ID and set-group-ID bits of a file will be
> - * cleared upon successful return from chown()
> + * cleared upon successful return from chown().
> + * Of a directory, these bits should be kept.
> */
> - if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) &&
> - !capable(CAP_FSETID))
> - ip->i_d.di_mode &= ~(S_ISUID|S_ISGID);
> + if (!S_ISDIR(inode->i_mode))
> + if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) &&
> + !capable(CAP_FSETID))
> + ip->i_d.di_mode &= ~(S_ISUID|S_ISGID);
>
> /*
> * Change the ownerships and register quota modifications
> @@ -915,7 +932,7 @@ xfs_vn_setattr(
> {
> if (iattr->ia_valid & ATTR_SIZE)
> return -xfs_setattr_size(XFS_I(dentry->d_inode), iattr, 0);
> - return -xfs_setattr_nonsize(XFS_I(dentry->d_inode), iattr, 0);
> + return -xfs_setattr_setup(XFS_I(dentry->d_inode), iattr, 0);
> }
>
> STATIC int
>
|