[PATCH] Subject: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls [V3]

[PATCH] Subject: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls [V3]

[Carlos Maiolino]

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

V3: Remove S_ISDIR check in xfs_setattr_nonsize() from the patch

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_iops.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ca9ecaa..2e5aca8 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);
 
-	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);
 
-- 
1.8.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Subject: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls [V3]

[Carlos Maiolino]

Hi, any comments on this one?

On Fri, Jun 21, 2013 at 02:45:53PM -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
> 
> V3: Remove S_ISDIR check in xfs_setattr_nonsize() from the patch
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_iops.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ca9ecaa..2e5aca8 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);
>  
> -	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);
>  
> -- 
> 1.8.1.4
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Subject: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls [V3]

[Ben Myers]

Hey Carlos,

On Wed, Jul 03, 2013 at 12:24:12PM -0300, Carlos Maiolino wrote:
> Hi, any comments on this one?

This goes with generic/313, right?

http://oss.sgi.com/archives/xfs/2013-05/msg00957.html

Thanks,
	Ben

> On Fri, Jun 21, 2013 at 02:45:53PM -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
> > 
> > V3: Remove S_ISDIR check in xfs_setattr_nonsize() from the patch
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  fs/xfs/xfs_iops.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index ca9ecaa..2e5aca8 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);
> >  
> > -	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);
> >  
> > -- 
> > 1.8.1.4
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> -- 
> Carlos
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Subject: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls [V3]

[Ben Myers]

On Fri, Jun 21, 2013 at 02:45:53PM -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
> 
> V3: Remove S_ISDIR check in xfs_setattr_nonsize() from the patch
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_iops.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ca9ecaa..2e5aca8 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;
> -

inode_change_ok has had the check for whether to clear S_ISGID since the
initial import of Linus's tree, and it is called when ATTR_MODE is set there,
just as in xfs_setattr_nonsize, and xfs_setattr_size.  That aspect of this
looks ok to me.

>  	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);
>  
> -	error = -inode_change_ok(inode, iattr);
> -	if (error)
> -		return XFS_ERROR(error);
> +		error = -inode_change_ok(inode, iattr);
> +		if (error)
> +			return XFS_ERROR(error);
> +	}

I'm not so sure about this change yet.  Looks like the two relevant callers are:

.set - xattr_handler
  xfs_xattr_acl_set
    xfs_set_mode
      xfs_setattr_nonsize(..., XFS_ATTR_NOACL);

and

xfs_vn_mknod
  xfs_inherit_acl
    xfs_set_mode
      xfs_setattr_nonsize(..., XFS_ATTR_NOACL);

I suggest moving the forced shutdown and readonly checks outside of the
XFS_ATTR_NOACL conditional.  I'm not seeing those checks in xfs_attr_acl_set or
xfs_vn_mknod and it won't hurt to be careful.

It also seems like inode_change_ok might have some other checks that are
necessary to determine whether it is ok to update the mode and ctime here.  A
call to inode_owner_or_capable as is done in inode_change_ok would cover this
possibility.

Other than those two suggestions this looks pretty good to me.

Regards,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Subject: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls [V3]

[Dave Chinner]

On Mon, Jul 08, 2013 at 04:11:21PM -0500, Ben Myers wrote:
> On Fri, Jun 21, 2013 at 02:45:53PM -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
> > 
> > V3: Remove S_ISDIR check in xfs_setattr_nonsize() from the patch
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> >  
> > -	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);
> >  
> > -	error = -inode_change_ok(inode, iattr);
> > -	if (error)
> > -		return XFS_ERROR(error);
> > +		error = -inode_change_ok(inode, iattr);
> > +		if (error)
> > +			return XFS_ERROR(error);
> > +	}
> 
> I'm not so sure about this change yet.  Looks like the two relevant callers are:
> 
> .set - xattr_handler
>   xfs_xattr_acl_set
>     xfs_set_mode
>       xfs_setattr_nonsize(..., XFS_ATTR_NOACL);
> 
> and
> 
> xfs_vn_mknod
>   xfs_inherit_acl
>     xfs_set_mode
>       xfs_setattr_nonsize(..., XFS_ATTR_NOACL);
> 
> I suggest moving the forced shutdown and readonly checks outside of the
> XFS_ATTR_NOACL conditional.  I'm not seeing those checks in xfs_attr_acl_set or
> xfs_vn_mknod and it won't hurt to be careful.

In both cases, the read-only checks are done at much higher layers
and so we don't ever get to xfs_setattr_nonsize() through these
paths with a read-only filesystem. Shutdown doesn't really matter -
the transaction commit will fail if the filesystem is shut down...

> It also seems like inode_change_ok might have some other checks that are
> necessary to determine whether it is ok to update the mode and ctime here.  A
> call to inode_owner_or_capable as is done in inode_change_ok would cover this
> possibility.

The inode permission checks are already done by xfs_xattr_acl_set():

	if ((current_fsuid() != inode->i_uid) && !capable(CAP_FOWNER))
		return -EPERM;

and in the case of xfs_inherit_acl() the user has just created the
file so they - by definition - have permission to inherit the ACL
and modify the mode of the inode they just created.

So there is no need for changes to inode_change_ok() here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Subject: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls [V3]

[Ben Myers]

On Tue, Jul 09, 2013 at 09:42:35AM +1000, Dave Chinner wrote:
> On Mon, Jul 08, 2013 at 04:11:21PM -0500, Ben Myers wrote:
> > On Fri, Jun 21, 2013 at 02:45:53PM -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
> > > 
> > > V3: Remove S_ISDIR check in xfs_setattr_nonsize() from the patch
> > > 
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > >  
> > > -	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);
> > >  
> > > -	error = -inode_change_ok(inode, iattr);
> > > -	if (error)
> > > -		return XFS_ERROR(error);
> > > +		error = -inode_change_ok(inode, iattr);
> > > +		if (error)
> > > +			return XFS_ERROR(error);
> > > +	}
> > 
> > I'm not so sure about this change yet.  Looks like the two relevant callers are:
> > 
> > .set - xattr_handler
> >   xfs_xattr_acl_set
> >     xfs_set_mode
> >       xfs_setattr_nonsize(..., XFS_ATTR_NOACL);
> > 
> > and
> > 
> > xfs_vn_mknod
> >   xfs_inherit_acl
> >     xfs_set_mode
> >       xfs_setattr_nonsize(..., XFS_ATTR_NOACL);
> > 
> > I suggest moving the forced shutdown and readonly checks outside of the
> > XFS_ATTR_NOACL conditional.  I'm not seeing those checks in xfs_attr_acl_set or
> > xfs_vn_mknod and it won't hurt to be careful.
> 
> In both cases, the read-only checks are done at much higher layers
> and so we don't ever get to xfs_setattr_nonsize() through these
> paths with a read-only filesystem. Shutdown doesn't really matter -
> the transaction commit will fail if the filesystem is shut down...
> 
> > It also seems like inode_change_ok might have some other checks that are
> > necessary to determine whether it is ok to update the mode and ctime here.  A
> > call to inode_owner_or_capable as is done in inode_change_ok would cover this
> > possibility.
> 
> The inode permission checks are already done by xfs_xattr_acl_set():
> 
> 	if ((current_fsuid() != inode->i_uid) && !capable(CAP_FOWNER))
> 		return -EPERM;
> 
> and in the case of xfs_inherit_acl() the user has just created the
> file so they - by definition - have permission to inherit the ACL
> and modify the mode of the inode they just created.
> 
> So there is no need for changes to inode_change_ok() here.

Carlos, if you agree with Dave's assessment consider this 

Reviewed-by: Ben Myers <bpm@sgi.com>

let me know what you think and I'll pull it in.

Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Subject: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls [V3]

[Carlos Maiolino]

On Tue, Jul 09, 2013 at 12:06:08PM -0500, Ben Myers wrote:
> On Tue, Jul 09, 2013 at 09:42:35AM +1000, Dave Chinner wrote:
> > On Mon, Jul 08, 2013 at 04:11:21PM -0500, Ben Myers wrote:
> > > On Fri, Jun 21, 2013 at 02:45:53PM -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
> > > > 
> > > > V3: Remove S_ISDIR check in xfs_setattr_nonsize() from the patch
> > > > 
> > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > >  
> > > > -	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);
> > > >  
> > > > -	error = -inode_change_ok(inode, iattr);
> > > > -	if (error)
> > > > -		return XFS_ERROR(error);
> > > > +		error = -inode_change_ok(inode, iattr);
> > > > +		if (error)
> > > > +			return XFS_ERROR(error);
> > > > +	}
> > > 
> > > I'm not so sure about this change yet.  Looks like the two relevant callers are:
> > > 
> > > .set - xattr_handler
> > >   xfs_xattr_acl_set
> > >     xfs_set_mode
> > >       xfs_setattr_nonsize(..., XFS_ATTR_NOACL);
> > > 
> > > and
> > > 
> > > xfs_vn_mknod
> > >   xfs_inherit_acl
> > >     xfs_set_mode
> > >       xfs_setattr_nonsize(..., XFS_ATTR_NOACL);
> > > 
> > > I suggest moving the forced shutdown and readonly checks outside of the
> > > XFS_ATTR_NOACL conditional.  I'm not seeing those checks in xfs_attr_acl_set or
> > > xfs_vn_mknod and it won't hurt to be careful.
> > 
> > In both cases, the read-only checks are done at much higher layers
> > and so we don't ever get to xfs_setattr_nonsize() through these
> > paths with a read-only filesystem. Shutdown doesn't really matter -
> > the transaction commit will fail if the filesystem is shut down...
> > 
> > > It also seems like inode_change_ok might have some other checks that are
> > > necessary to determine whether it is ok to update the mode and ctime here.  A
> > > call to inode_owner_or_capable as is done in inode_change_ok would cover this
> > > possibility.
> > 
> > The inode permission checks are already done by xfs_xattr_acl_set():
> > 
> > 	if ((current_fsuid() != inode->i_uid) && !capable(CAP_FOWNER))
> > 		return -EPERM;
> > 
> > and in the case of xfs_inherit_acl() the user has just created the
> > file so they - by definition - have permission to inherit the ACL
> > and modify the mode of the inode they just created.
> > 
> > So there is no need for changes to inode_change_ok() here.
> 
> Carlos, if you agree with Dave's assessment consider this 
> 
> Reviewed-by: Ben Myers <bpm@sgi.com>
> 
> let me know what you think and I'll pull it in.
> 
> Thanks,
> 	Ben
> 
Hi Ben,

Yes, I agree with Dave's statements, I removed the unneeded checks, because they
were done at higher layers.

please, feel free to pull it in, my apologies to have not answered it on IRC, it
was holiday here and I just saw your message late this morning.

Cheers,

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] Subject: [PATCH] xfs: fix sgid inheritance for subdirectories inheriting default acls [V3]

[Ben Myers]

On Wed, Jul 10, 2013 at 10:22:58AM -0300, Carlos Maiolino wrote:
> On Tue, Jul 09, 2013 at 12:06:08PM -0500, Ben Myers wrote:
> > On Tue, Jul 09, 2013 at 09:42:35AM +1000, Dave Chinner wrote:
> > > On Mon, Jul 08, 2013 at 04:11:21PM -0500, Ben Myers wrote:
> > > > On Fri, Jun 21, 2013 at 02:45:53PM -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
> > > > > 
> > > > > V3: Remove S_ISDIR check in xfs_setattr_nonsize() from the patch
> > > > > 
> > > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > > >  
> > > > > -	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);
> > > > >  
> > > > > -	error = -inode_change_ok(inode, iattr);
> > > > > -	if (error)
> > > > > -		return XFS_ERROR(error);
> > > > > +		error = -inode_change_ok(inode, iattr);
> > > > > +		if (error)
> > > > > +			return XFS_ERROR(error);
> > > > > +	}
> > > > 
> > > > I'm not so sure about this change yet.  Looks like the two relevant callers are:
> > > > 
> > > > .set - xattr_handler
> > > >   xfs_xattr_acl_set
> > > >     xfs_set_mode
> > > >       xfs_setattr_nonsize(..., XFS_ATTR_NOACL);
> > > > 
> > > > and
> > > > 
> > > > xfs_vn_mknod
> > > >   xfs_inherit_acl
> > > >     xfs_set_mode
> > > >       xfs_setattr_nonsize(..., XFS_ATTR_NOACL);
> > > > 
> > > > I suggest moving the forced shutdown and readonly checks outside of the
> > > > XFS_ATTR_NOACL conditional.  I'm not seeing those checks in xfs_attr_acl_set or
> > > > xfs_vn_mknod and it won't hurt to be careful.
> > > 
> > > In both cases, the read-only checks are done at much higher layers
> > > and so we don't ever get to xfs_setattr_nonsize() through these
> > > paths with a read-only filesystem. Shutdown doesn't really matter -
> > > the transaction commit will fail if the filesystem is shut down...
> > > 
> > > > It also seems like inode_change_ok might have some other checks that are
> > > > necessary to determine whether it is ok to update the mode and ctime here.  A
> > > > call to inode_owner_or_capable as is done in inode_change_ok would cover this
> > > > possibility.
> > > 
> > > The inode permission checks are already done by xfs_xattr_acl_set():
> > > 
> > > 	if ((current_fsuid() != inode->i_uid) && !capable(CAP_FOWNER))
> > > 		return -EPERM;
> > > 
> > > and in the case of xfs_inherit_acl() the user has just created the
> > > file so they - by definition - have permission to inherit the ACL
> > > and modify the mode of the inode they just created.
> > > 
> > > So there is no need for changes to inode_change_ok() here.
> > 
> > Carlos, if you agree with Dave's assessment consider this 
> > 
> > Reviewed-by: Ben Myers <bpm@sgi.com>
> > 
> > let me know what you think and I'll pull it in.
> > 
> > Thanks,
> > 	Ben
> > 
> Hi Ben,
> 
> Yes, I agree with Dave's statements, I removed the unneeded checks, because they
> were done at higher layers.

Excellent.

> please, feel free to pull it in, my apologies to have not answered it on IRC, it
> was holiday here and I just saw your message late this morning.

No worries.  ;)

I've pulled this in.

Thanks,
Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs