On Tue, Jun 18, 2013 at 12:32:16PM -0300, Carlos Maiolino wrote: > XFS removes sgid bits of subdirectories 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. > > With this patch, we extend the meaning of XFS_ATTR_NOACL flag to avoid these > checks when acls are being inherited (thanks hch). > > Also, 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(). > > Changelog: > > V2: Extends the meaning of XFS_ATTR_NOACL instead of wrap the tests into another > function > > Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx> > --- > fs/xfs/xfs_iops.c | 27 ++++++++++++++------------- > 1 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index ca9ecaa..3547d89 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; > > @@ -495,15 +492,18 @@ xfs_setattr_nonsize( > > trace_xfs_setattr(ip); > > - if (mp->m_flags & XFS_MOUNT_RDONLY) > - return XFS_ERROR(EROFS); > + /* If acls are being inherited, we already have this checked */ > + if (!(flags & XFS_ATTR_NOACL)) { > + if (mp->m_flags & XFS_MOUNT_RDONLY) > + return XFS_ERROR(EROFS); > > - if (XFS_FORCED_SHUTDOWN(mp)) > - return XFS_ERROR(EIO); > + if (XFS_FORCED_SHUTDOWN(mp)) > + return XFS_ERROR(EIO); I'd leave the RO and shutdown checks outside the NOACL branch... > > - error = -inode_change_ok(inode, iattr); > - if (error) > - return XFS_ERROR(error); > + error = -inode_change_ok(inode, iattr); > + if (error) > + return XFS_ERROR(error); > + } > > ASSERT((mask & ATTR_SIZE) == 0); > > @@ -594,9 +594,10 @@ xfs_setattr_nonsize( > * The set-user-ID and set-group-ID bits of a file will be > * cleared upon successful return from chown() > */ > - 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); I'm not sure I understand why this is part of this patch - the ACL path does not enter this code branch (ATTR_UID/GID) so it doesn't affect ACL inheritence. So this is some other behavioural change? Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs