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 > _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
