Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Andreas Gruenbacher]

Hello,

The usual way of manipulating a file's POSIX ACL is through the
system.posix_acl_{access,default} xattrs. Setting
system.posix_acl_access also sets the permission bits in the file
mode. The acls are cached in inode->i_acl and inode->i_default_acl.

On XFS, POSIX ACLs are also exposed as trusted.SGI_ACL_{FILE,DEFAULT}
xattrs in a different value format. However, setting these xattrs does
not update inode->i_{,default_}acl, and setting trusted.SGI_ACL_FILE
does not update the file mode; things can get out of sync:

  $ touch f
  $ setfacl -m u:agruenba:rw f
  $ ls -l f
  -rw-rw-r--+ 1 root root 0 Oct 23 15:04 f
  $ getfattr -m- -d f
  # file: f
  security.selinux="unconfined_u:object_r:user_tmp_t:s0"
  system.posix_acl_access=0sAgAAAAEABgD/////AgAGAOgDAAAEAAQA/////xAABgD/////IAAEAP////8=
  trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==

  $ chmod 0 f
  $ setfattr -n trusted.SGI_ACL_FILE -v
0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
f
  $ ls -l f
  ----------+ 1 root root 0 Oct 23 15:04 /var/tmp/f
  $ getfacl f
  # file: f
  # owner: root
  # group: root
  user::---
  user:agruenba:rw-        #effective:---
  group::r--            #effective:---
  mask::---
  other::---
  $ getfattr -m- -d f
  # file: f
  security.selinux="unconfined_u:object_r:user_tmp_t:s0"
  system.posix_acl_access=0sAgAAAAEAAAD/////AgAGAOgDAAAEAAQA/////xAAAAD/////IAAAAP////8=
  trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==

Here, the file mode and the reported value of system.posix_acl_access
are both wrong; trusted.SGI_ACL_FILE corresponds to what's stored on
disk.

Access to trusted.* attributes is limited to users capable of
CAP_SYS_ADMIN so ordinary users cannot cause this kind of damage, but
this still deserves fixing.

Thanks,
Andreas

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Brian Foster]

On Fri, Oct 23, 2015 at 03:52:54PM +0200, Andreas Gruenbacher wrote:
> Hello,
> 
> The usual way of manipulating a file's POSIX ACL is through the
> system.posix_acl_{access,default} xattrs. Setting
> system.posix_acl_access also sets the permission bits in the file
> mode. The acls are cached in inode->i_acl and inode->i_default_acl.
> 
> On XFS, POSIX ACLs are also exposed as trusted.SGI_ACL_{FILE,DEFAULT}
> xattrs in a different value format. However, setting these xattrs does
> not update inode->i_{,default_}acl, and setting trusted.SGI_ACL_FILE
> does not update the file mode; things can get out of sync:
> 

It looks like the posix_acl_* values are virtual xattrs on XFS. They get
translated to the SGI_ACL_* names before the acl code calls down into
the xattr code. The result is cached into the inode via set_cached_acl()
before the call returns.

The xattr code doesn't know anything about this so I suspect this is the
reason for the inconsistency. A direct xattr update just updates the
data on-disk and has no knowledge of the ACLs cached to the inode, the
latter of which is probably what is returned after the setxattr.

>   $ touch f
>   $ setfacl -m u:agruenba:rw f
>   $ ls -l f
>   -rw-rw-r--+ 1 root root 0 Oct 23 15:04 f
>   $ getfattr -m- -d f
>   # file: f
>   security.selinux="unconfined_u:object_r:user_tmp_t:s0"
>   system.posix_acl_access=0sAgAAAAEABgD/////AgAGAOgDAAAEAAQA/////xAABgD/////IAAEAP////8=
>   trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
> 
>   $ chmod 0 f
>   $ setfattr -n trusted.SGI_ACL_FILE -v
> 0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
> f
>   $ ls -l f
>   ----------+ 1 root root 0 Oct 23 15:04 /var/tmp/f
>   $ getfacl f
>   # file: f
>   # owner: root
>   # group: root
>   user::---
>   user:agruenba:rw-        #effective:---
>   group::r--            #effective:---
>   mask::---
>   other::---
>   $ getfattr -m- -d f
>   # file: f
>   security.selinux="unconfined_u:object_r:user_tmp_t:s0"
>   system.posix_acl_access=0sAgAAAAEAAAD/////AgAGAOgDAAAEAAQA/////xAAAAD/////IAAAAP////8=
>   trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
> 
> Here, the file mode and the reported value of system.posix_acl_access
> are both wrong; trusted.SGI_ACL_FILE corresponds to what's stored on
> disk.
> 
> Access to trusted.* attributes is limited to users capable of
> CAP_SYS_ADMIN so ordinary users cannot cause this kind of damage, but
> this still deserves fixing.
> 

Not sure there's a real use case for this, but it looks like we could
invalidate the cached ACLs if those xattrs are modified directly via the
xattr interface. Care to test the (compile tested only) hunk below? An
alternative could be to just disallow setting these xattrs directly.

Brian

> Thanks,
> Andreas
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index c0368151..651c2a3 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -57,7 +57,8 @@ static int
 xfs_xattr_set(struct dentry *dentry, const char *name, const void *value,
 		size_t size, int flags, int xflags)
 {
-	struct xfs_inode *ip = XFS_I(d_inode(dentry));
+	struct xfs_inode	*ip = XFS_I(d_inode(dentry));
+	int			error;
 
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
@@ -70,8 +71,20 @@ xfs_xattr_set(struct dentry *dentry, const char *name, const void *value,
 
 	if (!value)
 		return xfs_attr_remove(ip, (unsigned char *)name, xflags);
-	return xfs_attr_set(ip, (unsigned char *)name,
+	error = xfs_attr_set(ip, (unsigned char *)name,
 				(void *)value, size, xflags);
+	/*
+	 * Invalidate any cached ACLs if the user has bypassed the ACL
+	 * interface.
+	 */
+	if (!error && (xflags & ATTR_ROOT)) {
+		if (!strncmp(name, SGI_ACL_FILE, strlen(name)))
+			forget_cached_acl(VFS_I(ip), ACL_TYPE_ACCESS);
+		else if (!strncmp(name, SGI_ACL_DEFAULT, strlen(name)))
+			forget_cached_acl(VFS_I(ip), ACL_TYPE_DEFAULT);
+	}
+
+	return error;
 }
 
 static const struct xattr_handler xfs_xattr_user_handler = {

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Andreas Gruenbacher]

On Sat, Oct 24, 2015 at 2:57 PM, Brian Foster <bfoster@redhat.com> wrote:
> On Fri, Oct 23, 2015 at 03:52:54PM +0200, Andreas Gruenbacher wrote:
>> Hello,
>>
>> The usual way of manipulating a file's POSIX ACL is through the
>> system.posix_acl_{access,default} xattrs. Setting
>> system.posix_acl_access also sets the permission bits in the file
>> mode. The acls are cached in inode->i_acl and inode->i_default_acl.
>>
>> On XFS, POSIX ACLs are also exposed as trusted.SGI_ACL_{FILE,DEFAULT}
>> xattrs in a different value format. However, setting these xattrs does
>> not update inode->i_{,default_}acl, and setting trusted.SGI_ACL_FILE
>> does not update the file mode; things can get out of sync:
>>
>
> It looks like the posix_acl_* values are virtual xattrs on XFS. They get
> translated to the SGI_ACL_* names before the acl code calls down into
> the xattr code. The result is cached into the inode via set_cached_acl()
> before the call returns.

The posix_acl_* attributes are handled by the vfs
posix_acl_*_xattr_handler handlers which talk to the filesystem using
the get_acl and set_acl inode operations. We would need such handlers
for SGI_ACL_*, installed before xfs_xattr_trusted_handler in
xfs_xattr_handlers.

> The xattr code doesn't know anything about this so I suspect this is the
> reason for the inconsistency. A direct xattr update just updates the
> data on-disk and has no knowledge of the ACLs cached to the inode, the
> latter of which is probably what is returned after the setxattr.
>
>>   $ touch f
>>   $ setfacl -m u:agruenba:rw f
>>   $ ls -l f
>>   -rw-rw-r--+ 1 root root 0 Oct 23 15:04 f
>>   $ getfattr -m- -d f
>>   # file: f
>>   security.selinux="unconfined_u:object_r:user_tmp_t:s0"
>>   system.posix_acl_access=0sAgAAAAEABgD/////AgAGAOgDAAAEAAQA/////xAABgD/////IAAEAP////8=
>>   trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
>>
>>   $ chmod 0 f
>>   $ setfattr -n trusted.SGI_ACL_FILE -v
>> 0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
>> f
>>   $ ls -l f
>>   ----------+ 1 root root 0 Oct 23 15:04 /var/tmp/f
>>   $ getfacl f
>>   # file: f
>>   # owner: root
>>   # group: root
>>   user::---
>>   user:agruenba:rw-        #effective:---
>>   group::r--            #effective:---
>>   mask::---
>>   other::---
>>   $ getfattr -m- -d f
>>   # file: f
>>   security.selinux="unconfined_u:object_r:user_tmp_t:s0"
>>   system.posix_acl_access=0sAgAAAAEAAAD/////AgAGAOgDAAAEAAQA/////xAAAAD/////IAAAAP////8=
>>   trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
>>
>> Here, the file mode and the reported value of system.posix_acl_access
>> are both wrong; trusted.SGI_ACL_FILE corresponds to what's stored on
>> disk.
>>
>> Access to trusted.* attributes is limited to users capable of
>> CAP_SYS_ADMIN so ordinary users cannot cause this kind of damage, but
>> this still deserves fixing.
>>
>
> Not sure there's a real use case for this, but it looks like we could
> invalidate the cached ACLs if those xattrs are modified directly via the
> xattr interface.

POSIX ACLs should not have been exposed twice in the first place, but
those SGI_ACL_* xattrs have been around for a very long time and we
cannot get rid of them now. It's likely that some applications will
back up some or all of an inode's xattrs and later restore them.

> Care to test the (compile tested only) hunk below?

That won't update the file mode when setting a SGI_ACL_* attribute.

> An alternative could be to just disallow setting these xattrs directly.

Probably not because that would cause applications to fail in
unexpected new ways.

Thanks,
Andreas

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Brian Foster]

On Sat, Oct 24, 2015 at 03:58:04PM +0200, Andreas Gruenbacher wrote:
> On Sat, Oct 24, 2015 at 2:57 PM, Brian Foster <bfoster@redhat.com> wrote:
> > On Fri, Oct 23, 2015 at 03:52:54PM +0200, Andreas Gruenbacher wrote:
> >> Hello,
> >>
> >> The usual way of manipulating a file's POSIX ACL is through the
> >> system.posix_acl_{access,default} xattrs. Setting
> >> system.posix_acl_access also sets the permission bits in the file
> >> mode. The acls are cached in inode->i_acl and inode->i_default_acl.
> >>
> >> On XFS, POSIX ACLs are also exposed as trusted.SGI_ACL_{FILE,DEFAULT}
> >> xattrs in a different value format. However, setting these xattrs does
> >> not update inode->i_{,default_}acl, and setting trusted.SGI_ACL_FILE
> >> does not update the file mode; things can get out of sync:
> >>
> >
> > It looks like the posix_acl_* values are virtual xattrs on XFS. They get
> > translated to the SGI_ACL_* names before the acl code calls down into
> > the xattr code. The result is cached into the inode via set_cached_acl()
> > before the call returns.
> 
> The posix_acl_* attributes are handled by the vfs
> posix_acl_*_xattr_handler handlers which talk to the filesystem using
> the get_acl and set_acl inode operations. We would need such handlers
> for SGI_ACL_*, installed before xfs_xattr_trusted_handler in
> xfs_xattr_handlers.
> 

Yes, but the translation all occurs on the XFS side. I'm not following
the last bit... are you suggesting a special xattr handler that uses
"trusted.SGI_FILE" as a prefix? I'm not sure that matters either way so
long as those xattrs are trapped appropriately, but feel free to send a
patch. :)

> > The xattr code doesn't know anything about this so I suspect this is the
> > reason for the inconsistency. A direct xattr update just updates the
> > data on-disk and has no knowledge of the ACLs cached to the inode, the
> > latter of which is probably what is returned after the setxattr.
> >
> >>   $ touch f
> >>   $ setfacl -m u:agruenba:rw f
> >>   $ ls -l f
> >>   -rw-rw-r--+ 1 root root 0 Oct 23 15:04 f
> >>   $ getfattr -m- -d f
> >>   # file: f
> >>   security.selinux="unconfined_u:object_r:user_tmp_t:s0"
> >>   system.posix_acl_access=0sAgAAAAEABgD/////AgAGAOgDAAAEAAQA/////xAABgD/////IAAEAP////8=
> >>   trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
> >>
> >>   $ chmod 0 f
> >>   $ setfattr -n trusted.SGI_ACL_FILE -v
> >> 0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
> >> f
> >>   $ ls -l f
> >>   ----------+ 1 root root 0 Oct 23 15:04 /var/tmp/f
> >>   $ getfacl f
> >>   # file: f
> >>   # owner: root
> >>   # group: root
> >>   user::---
> >>   user:agruenba:rw-        #effective:---
> >>   group::r--            #effective:---
> >>   mask::---
> >>   other::---
> >>   $ getfattr -m- -d f
> >>   # file: f
> >>   security.selinux="unconfined_u:object_r:user_tmp_t:s0"
> >>   system.posix_acl_access=0sAgAAAAEAAAD/////AgAGAOgDAAAEAAQA/////xAAAAD/////IAAAAP////8=
> >>   trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
> >>
> >> Here, the file mode and the reported value of system.posix_acl_access
> >> are both wrong; trusted.SGI_ACL_FILE corresponds to what's stored on
> >> disk.
> >>
> >> Access to trusted.* attributes is limited to users capable of
> >> CAP_SYS_ADMIN so ordinary users cannot cause this kind of damage, but
> >> this still deserves fixing.
> >>
> >
> > Not sure there's a real use case for this, but it looks like we could
> > invalidate the cached ACLs if those xattrs are modified directly via the
> > xattr interface.
> 
> POSIX ACLs should not have been exposed twice in the first place, but
> those SGI_ACL_* xattrs have been around for a very long time and we
> cannot get rid of them now. It's likely that some applications will
> back up some or all of an inode's xattrs and later restore them.
> 

I wasn't suggesting to get rid of them.

> > Care to test the (compile tested only) hunk below?
> 
> That won't update the file mode when setting a SGI_ACL_* attribute.
> 

Hmm, perhaps this is not sufficient if the mode has to be updated as
well. I suppose we could try to do that as well in this path, but that
implies verification of the data (already in on-disk format) as well.
There's nothing stopping somebody from doing 'setattr -n
trusted.SGI_FILE_ACCESS -v 0 <file>,' after all. The previous patch
wasn't really intended to address that.

> > An alternative could be to just disallow setting these xattrs directly.
> 
> Probably not because that would cause applications to fail in
> unexpected new ways.
> 

I suppose a backup/restore application might want to set these, but I'm
not aware of any other sane usage given they're in a filesystem specific
format at this point. We'd probably have to take a look at xfsdump, see
how it handles this, then see if there's a clean way to run through
necessary acl bits if we're called via setxattr().

Brian

> Thanks,
> Andreas

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Brian Foster]

On Sat, Oct 24, 2015 at 11:22:55AM -0400, Brian Foster wrote:
> On Sat, Oct 24, 2015 at 03:58:04PM +0200, Andreas Gruenbacher wrote:
> > On Sat, Oct 24, 2015 at 2:57 PM, Brian Foster <bfoster@redhat.com> wrote:
> > > On Fri, Oct 23, 2015 at 03:52:54PM +0200, Andreas Gruenbacher wrote:
...
> > That won't update the file mode when setting a SGI_ACL_* attribute.
> > 
> 
> Hmm, perhaps this is not sufficient if the mode has to be updated as
> well. I suppose we could try to do that as well in this path, but that
> implies verification of the data (already in on-disk format) as well.
> There's nothing stopping somebody from doing 'setattr -n
> trusted.SGI_FILE_ACCESS -v 0 <file>,' after all. The previous patch
> wasn't really intended to address that.
> 
> > > An alternative could be to just disallow setting these xattrs directly.
> > 
> > Probably not because that would cause applications to fail in
> > unexpected new ways.
> > 
> 
> I suppose a backup/restore application might want to set these, but I'm
> not aware of any other sane usage given they're in a filesystem specific
> format at this point. We'd probably have to take a look at xfsdump, see
> how it handles this, then see if there's a clean way to run through
> necessary acl bits if we're called via setxattr().
> 

FWIW, the counter argument to this might be that since these are exposed
in filesystem specific format, the bug is that they are exposed in the
first place and any backup app that wants to support ACLs should do so
using appropriate APIs. That implies we would hide the internal xattrs
and disallow setting them directly.

I'm not sure what the right answer is tbh (or if the xattr exposure is
something more than historical accident). I'd have to think about it a
bit. Perhaps Dave or others have some thoughts as well...

Brian

> Brian
> 
> > Thanks,
> > Andreas
> 
> _______________________________________________
> 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: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Andreas Gruenbacher]

On Sat, Oct 24, 2015 at 5:22 PM, Brian Foster <bfoster@redhat.com> wrote:
> On Sat, Oct 24, 2015 at 03:58:04PM +0200, Andreas Gruenbacher wrote:
>> The posix_acl_* attributes are handled by the vfs
>> posix_acl_*_xattr_handler handlers which talk to the filesystem using
>> the get_acl and set_acl inode operations. We would need such handlers
>> for SGI_ACL_*, installed before xfs_xattr_trusted_handler in
>> xfs_xattr_handlers.
>>
>
> Yes, but the translation all occurs on the XFS side. I'm not following
> the last bit... are you suggesting a special xattr handler that uses
> "trusted.SGI_FILE" as a prefix?

Yes.

> I'm not sure that matters either way so
> long as those xattrs are trapped appropriately, but feel free to send a
> patch. :)

I'll send some patches.

Andreas

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

[PATCH 0/4] xfs: SGI ACL Fixes

[Andreas Gruenbacher]

Here are some fixes for the trusted.SGI_ACL_* attributes.  This adds some
more warts and it would be much better to get rid of those unnecessary
attributes instead; we probably can't though.

I have tested this manually but haven't run xfstests against that.  Please
review.

Thanks,
Andreas

Andreas Gruenbacher (4):
  xfs: Validate the length of on-disk ACLs
  xfs: SGI ACLs: Fix caching and mode setting
  xfs: SGI ACLs: Map uid/gid namespaces
  xfs: SGI ACLs: Prepare for richacls

 fs/xfs/libxfs/xfs_format.h |   8 +++-
 fs/xfs/xfs_acl.c           | 115 ++++++++++++++++++++++++++++++++++++++++-----
 fs/xfs/xfs_acl.h           |   3 ++
 fs/xfs/xfs_xattr.c         |   9 ++--
 fs/xfs/xfs_xattr.h         |  28 +++++++++++
 5 files changed, 147 insertions(+), 16 deletions(-)
 create mode 100644 fs/xfs/xfs_xattr.h

-- 
2.5.0

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

[PATCH 1/4] xfs: Validate the length of on-disk ACLs

[Andreas Gruenbacher]

In xfs_acl_from_disk, instead of trusting that xfs_acl.acl_cnt is correct,
make sure that the length of the attributes is correct as well.  Also, turn
the aclp parameter into a const pointer.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/xfs/libxfs/xfs_format.h |  8 ++++++--
 fs/xfs/xfs_acl.c           | 13 ++++++++-----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 9590a06..0e62682 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -1487,9 +1487,13 @@ struct xfs_acl {
 						sizeof(struct xfs_acl_entry) \
 		: 25)
 
-#define XFS_ACL_MAX_SIZE(mp) \
+#define XFS_ACL_SIZE(cnt) \
 	(sizeof(struct xfs_acl) + \
-		sizeof(struct xfs_acl_entry) * XFS_ACL_MAX_ENTRIES((mp)))
+		sizeof(struct xfs_acl_entry) * cnt)
+
+#define XFS_ACL_MAX_SIZE(mp) \
+	XFS_ACL_SIZE(XFS_ACL_MAX_ENTRIES((mp)))
+
 
 /* On-disk XFS extended attribute names */
 #define SGI_ACL_FILE		"SGI_ACL_FILE"
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 4b64167..763e365 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -37,16 +37,19 @@
 
 STATIC struct posix_acl *
 xfs_acl_from_disk(
-	struct xfs_acl	*aclp,
-	int		max_entries)
+	const struct xfs_acl	*aclp,
+	int			len,
+	int			max_entries)
 {
 	struct posix_acl_entry *acl_e;
 	struct posix_acl *acl;
-	struct xfs_acl_entry *ace;
+	const struct xfs_acl_entry *ace;
 	unsigned int count, i;
 
+	if (len < sizeof(*aclp))
+		return ERR_PTR(-EFSCORRUPTED);
 	count = be32_to_cpu(aclp->acl_cnt);
-	if (count > max_entries)
+	if (count > max_entries || XFS_ACL_SIZE(count) != len)
 		return ERR_PTR(-EFSCORRUPTED);
 
 	acl = posix_acl_alloc(count, GFP_KERNEL);
@@ -163,7 +166,7 @@ xfs_get_acl(struct inode *inode, int type)
 		goto out;
 	}
 
-	acl = xfs_acl_from_disk(xfs_acl, XFS_ACL_MAX_ENTRIES(ip->i_mount));
+	acl = xfs_acl_from_disk(xfs_acl, len, XFS_ACL_MAX_ENTRIES(ip->i_mount));
 	if (IS_ERR(acl))
 		goto out;
 
-- 
2.5.0

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

[PATCH 2/4] xfs: SGI ACLs: Fix caching and mode setting

[Andreas Gruenbacher]

POSIX ACLs on XFS are exposed as system.posix_acl_* as well as
trusted.SGI_ACL_*.  Setting the system attributes updates inode->i_mode,
inode->i_acl, and inode->i_default_acl as it should, but setting the trusted
attributes does not do that.  Fix that by adding xattr handlers for the two
trusted.SGI_ACL_* attributes.

The handlers must be installed before the trusted.* xattr handler in
xfs_xattr_handlers to take effect.

Other than before, trusted.SGI_ACL_* attribute values are now verified and
cannot be set to invalid values anymore.  Access to those attributes is still
limited to users capable of CAP_SYS_ADMIN, while the system.posix_acl_*
attributes can be read by anyone and set by the file owner.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/xfs/xfs_acl.c   | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_acl.h   |  3 +++
 fs/xfs/xfs_xattr.c |  4 ++-
 3 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 763e365..0eea7ee 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -305,3 +305,79 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
  set_acl:
 	return __xfs_set_acl(inode, type, acl);
 }
+
+static int
+xfs_xattr_acl_get(struct dentry *dentry, const char *name,
+		  void *value, size_t size, int type)
+{
+	struct inode *inode = d_inode(dentry);
+	struct posix_acl *acl;
+	int error;
+
+	if (S_ISLNK(inode->i_mode))
+		return -EOPNOTSUPP;
+
+	acl = get_acl(inode, type);
+	if (IS_ERR(acl))
+		return PTR_ERR(acl);
+	if (acl == NULL)
+		return -ENODATA;
+
+	error = XFS_ACL_SIZE(acl->a_count);
+	if (value) {
+		if (error > size)
+			error = -ERANGE;
+		else
+			xfs_acl_to_disk(value, acl);
+	}
+
+	posix_acl_release(acl);
+	return error;
+}
+
+static int
+xfs_xattr_acl_set(struct dentry *dentry, const char *name,
+		  const void *value, size_t size, int flags, int type)
+{
+	struct inode *inode = d_inode(dentry);
+	struct xfs_inode *ip = XFS_I(inode);
+	struct posix_acl *acl = NULL;
+	int error;
+
+	if (!inode->i_op->set_acl)
+		return -EOPNOTSUPP;
+
+	if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
+		return value ? -EACCES : 0;
+
+	if (value) {
+		acl = xfs_acl_from_disk(value, size, XFS_ACL_MAX_ENTRIES(ip->i_mount));
+		if (IS_ERR(acl))
+			return PTR_ERR(acl);
+
+		if (acl) {
+			error = posix_acl_valid(acl);
+			if (error)
+				goto out;
+		}
+	}
+
+	error = inode->i_op->set_acl(inode, acl, type);
+out:
+	posix_acl_release(acl);
+	return error;
+}
+
+const struct xattr_handler xfs_xattr_sgi_acl_file = {
+	.prefix = XATTR_TRUSTED_PREFIX SGI_ACL_FILE,
+	.flags	= ACL_TYPE_ACCESS,
+	.get	= xfs_xattr_acl_get,
+	.set	= xfs_xattr_acl_set,
+};
+
+const struct xattr_handler xfs_xattr_sgi_acl_default = {
+	.prefix = XATTR_TRUSTED_PREFIX SGI_ACL_DEFAULT,
+	.flags	= ACL_TYPE_DEFAULT,
+	.get	= xfs_xattr_acl_get,
+	.set	= xfs_xattr_acl_set,
+};
diff --git a/fs/xfs/xfs_acl.h b/fs/xfs/xfs_acl.h
index 3841b07..461dea6 100644
--- a/fs/xfs/xfs_acl.h
+++ b/fs/xfs/xfs_acl.h
@@ -27,6 +27,9 @@ extern struct posix_acl *xfs_get_acl(struct inode *inode, int type);
 extern int xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
 extern int posix_acl_access_exists(struct inode *inode);
 extern int posix_acl_default_exists(struct inode *inode);
+
+extern const struct xattr_handler xfs_xattr_sgi_acl_file;
+extern const struct xattr_handler xfs_xattr_sgi_acl_default;
 #else
 static inline struct posix_acl *xfs_get_acl(struct inode *inode, int type)
 {
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index c0368151..7534cb5 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -97,12 +97,14 @@ static const struct xattr_handler xfs_xattr_security_handler = {
 
 const struct xattr_handler *xfs_xattr_handlers[] = {
 	&xfs_xattr_user_handler,
-	&xfs_xattr_trusted_handler,
 	&xfs_xattr_security_handler,
 #ifdef CONFIG_XFS_POSIX_ACL
 	&posix_acl_access_xattr_handler,
 	&posix_acl_default_xattr_handler,
+	&xfs_xattr_sgi_acl_file,
+	&xfs_xattr_sgi_acl_default,
 #endif
+	&xfs_xattr_trusted_handler,
 	NULL
 };
 
-- 
2.5.0

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

Re: [PATCH 2/4] xfs: SGI ACLs: Fix caching and mode setting

[Brian Foster]

On Sat, Oct 24, 2015 at 11:16:07PM +0200, Andreas Gruenbacher wrote:
> POSIX ACLs on XFS are exposed as system.posix_acl_* as well as
> trusted.SGI_ACL_*.  Setting the system attributes updates inode->i_mode,
> inode->i_acl, and inode->i_default_acl as it should, but setting the trusted
> attributes does not do that.  Fix that by adding xattr handlers for the two
> trusted.SGI_ACL_* attributes.
> 
> The handlers must be installed before the trusted.* xattr handler in
> xfs_xattr_handlers to take effect.
> 
> Other than before, trusted.SGI_ACL_* attribute values are now verified and
> cannot be set to invalid values anymore.  Access to those attributes is still
> limited to users capable of CAP_SYS_ADMIN, while the system.posix_acl_*
> attributes can be read by anyone and set by the file owner.
> 

We should probably point out that the SGI_ACL_* xattrs are effectively
how ACLs are stored in XFS, thus direct usage expects the value to be in
valid on-disk format. The code mostly looks Ok to me, but this whole
thing still makes me cringe a bit.. :/ A few comments...

> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/xfs/xfs_acl.c   | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_acl.h   |  3 +++
>  fs/xfs/xfs_xattr.c |  4 ++-
>  3 files changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 763e365..0eea7ee 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -305,3 +305,79 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>   set_acl:
>  	return __xfs_set_acl(inode, type, acl);
>  }
> +
> +static int
> +xfs_xattr_acl_get(struct dentry *dentry, const char *name,
> +		  void *value, size_t size, int type)
> +{

I'm not a huge fan of using the 'type' name here, personally. I'd call
it xflags to be consistent with the other xattr handlers and add a brief
comment for both of these functions that explain we're hooking up into
the ACL interface from a layer below.

> +	struct inode *inode = d_inode(dentry);
> +	struct posix_acl *acl;
> +	int error;
> +
> +	if (S_ISLNK(inode->i_mode))
> +		return -EOPNOTSUPP;
> +
> +	acl = get_acl(inode, type);
> +	if (IS_ERR(acl))
> +		return PTR_ERR(acl);
> +	if (acl == NULL)
> +		return -ENODATA;
> +
> +	error = XFS_ACL_SIZE(acl->a_count);
> +	if (value) {
> +		if (error > size)
> +			error = -ERANGE;
> +		else
> +			xfs_acl_to_disk(value, acl);
> +	}
> +
> +	posix_acl_release(acl);
> +	return error;

With regard to the xattr_acl get function, why do we need to do anything
differently here from xfs_xattr_get()? Here we get the ACL, convert to
in-core format, then convert back to disk format into the buffer. I
don't think we need the extra validation in this scenario. In fact, I'd
expect this to return whatever might be on-disk from an older kernel,
etc. Can we just use xfs_xattr_get() for this case? Note that implies we
probably kill the abuse of xflags as well. If that's a problem for the
set case, just fixing up the flags and calling xfs_xattr_get() from here
might be another option.

> +}
> +
> +static int
> +xfs_xattr_acl_set(struct dentry *dentry, const char *name,
> +		  const void *value, size_t size, int flags, int type)
> +{
> +	struct inode *inode = d_inode(dentry);
> +	struct xfs_inode *ip = XFS_I(inode);
> +	struct posix_acl *acl = NULL;
> +	int error;
> +
> +	if (!inode->i_op->set_acl)
> +		return -EOPNOTSUPP;
> +
> +	if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
> +		return value ? -EACCES : 0;
> +

FWIW, this appears to be the only use of type here and it's already
handled down in the set_acl() path, so probably not necessary.

> +	if (value) {
> +		acl = xfs_acl_from_disk(value, size, XFS_ACL_MAX_ENTRIES(ip->i_mount));
> +		if (IS_ERR(acl))
> +			return PTR_ERR(acl);
> +

A comment around the validation here wouldn't hurt. E.g.,

/*
 * Data is expected in on-disk format. Therefore we convert to the
 * in-core ACL structures and validate as if this were set_acl().
 */

> +		if (acl) {
> +			error = posix_acl_valid(acl);
> +			if (error)
> +				goto out;
> +		}
> +	}
> +
> +	error = inode->i_op->set_acl(inode, acl, type);
> +out:
> +	posix_acl_release(acl);
> +	return error;
> +}
> +
> +const struct xattr_handler xfs_xattr_sgi_acl_file = {
> +	.prefix = XATTR_TRUSTED_PREFIX SGI_ACL_FILE,
> +	.flags	= ACL_TYPE_ACCESS,
> +	.get	= xfs_xattr_acl_get,
> +	.set	= xfs_xattr_acl_set,
> +};
> +
> +const struct xattr_handler xfs_xattr_sgi_acl_default = {
> +	.prefix = XATTR_TRUSTED_PREFIX SGI_ACL_DEFAULT,
> +	.flags	= ACL_TYPE_DEFAULT,
> +	.get	= xfs_xattr_acl_get,
> +	.set	= xfs_xattr_acl_set,
> +};

These should probably be in xfs_xattr.c with all of the other
xattr_handler definitions. We can kill the subsequent xfs_acl.h header
declarations (or replace them with the function declarations, at least)
as well. In fact on further thought, all of this should probably just
end up in xfs_xattr.c since these are technically xattr handlers.

Finally, another random thought... another way to approach this whole
thing might be just to redirect the SGI_FILE_* xattr calls to the
system.posix_acl_* calls. The SGI_FILE_* xattr data changes along with
the required conversions and whatnot, but then at least we expose data
in a more generic format. Isn't it the case that ACLs are generally set
via xattr calls, presumably in some specially defined format? This also
might get around the need for most of the bits in the subsequent
patches..

Brian

> diff --git a/fs/xfs/xfs_acl.h b/fs/xfs/xfs_acl.h
> index 3841b07..461dea6 100644
> --- a/fs/xfs/xfs_acl.h
> +++ b/fs/xfs/xfs_acl.h
> @@ -27,6 +27,9 @@ extern struct posix_acl *xfs_get_acl(struct inode *inode, int type);
>  extern int xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
>  extern int posix_acl_access_exists(struct inode *inode);
>  extern int posix_acl_default_exists(struct inode *inode);
> +
> +extern const struct xattr_handler xfs_xattr_sgi_acl_file;
> +extern const struct xattr_handler xfs_xattr_sgi_acl_default;
>  #else
>  static inline struct posix_acl *xfs_get_acl(struct inode *inode, int type)
>  {
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index c0368151..7534cb5 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -97,12 +97,14 @@ static const struct xattr_handler xfs_xattr_security_handler = {
>  
>  const struct xattr_handler *xfs_xattr_handlers[] = {
>  	&xfs_xattr_user_handler,
> -	&xfs_xattr_trusted_handler,
>  	&xfs_xattr_security_handler,
>  #ifdef CONFIG_XFS_POSIX_ACL
>  	&posix_acl_access_xattr_handler,
>  	&posix_acl_default_xattr_handler,
> +	&xfs_xattr_sgi_acl_file,
> +	&xfs_xattr_sgi_acl_default,
>  #endif
> +	&xfs_xattr_trusted_handler,
>  	NULL
>  };
>  
> -- 
> 2.5.0
> 
> _______________________________________________
> 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 2/4] xfs: SGI ACLs: Fix caching and mode setting

[Andreas Gruenbacher]

On Mon, Oct 26, 2015 at 3:02 PM, Brian Foster <bfoster@redhat.com> wrote:
> Finally, another random thought... another way to approach this whole
> thing might be just to redirect the SGI_FILE_* xattr calls to the
> system.posix_acl_* calls.

You mean silently changing the binary format of those attributes?
That's the worst thing we could possibly do.

> The SGI_FILE_* xattr data changes along with the required conversions
> and whatnot, but then at least we expose data in a more generic format.

I really doubt that we can get rid of those attributes.

The patches in this queue don't "change" the format of those
attributes, they only fix them for use in UID/GID  namespaces where
they are currently broken. Validation when setting those xattrs could
be removed, allowing sysadmins to set acls which the kernel and
xfs_repair would reject; it seems rather pointless though.

Thanks,
Andreas

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

Re: [PATCH 2/4] xfs: SGI ACLs: Fix caching and mode setting

[Brian Foster]

On Mon, Oct 26, 2015 at 04:39:20PM +0100, Andreas Gruenbacher wrote:
> On Mon, Oct 26, 2015 at 3:02 PM, Brian Foster <bfoster@redhat.com> wrote:
> > Finally, another random thought... another way to approach this whole
> > thing might be just to redirect the SGI_FILE_* xattr calls to the
> > system.posix_acl_* calls.
> 
> You mean silently changing the binary format of those attributes?
> That's the worst thing we could possibly do.
> 

It would change an implicit error/failure to an explicit one.
Regardless, I realized this particular approach is kind of pointless
anyways (as noted in my other mail).

Note that I'm not necessarily against what we're doing here, it might
very well be required. I'm just trying to step back and understand the
big picture before we go and put a band-aid on the immediate problem and
call it solved. To me, the ideal overall situation is that this
attribute is hidden and anybody that cares about ACLs can get/set them
through the appropriate ACL interface (the system.posix_acl_* xattrs).
The question is whether we can get anywhere close to that without
breaking things.

> > The SGI_FILE_* xattr data changes along with the required conversions
> > and whatnot, but then at least we expose data in a more generic format.
> 
> I really doubt that we can get rid of those attributes.
> 

The problem seems to boil down to the fact that the by-handle operations
(ATTRLIST_BY_HANDLE) don't include the system.posix_acl_* attributes,
and thus these aren't tracked by applications that use that interface
(such as xfsdump). I was hoping that whatever might have looked at the
SGI_ACL_* attrs would also have seen the posix_acl_* attr and thus we
could potentially get away with something like ignoring SGI_ACL_
operations (rather than failing them) without losing any information,
but that does not appear to be the case.

It still might be worth considering drawing a line in the sand (for
example, v5 filesystems or later) to deprecate the SGI_ACL_* xattrs. For
example:

- Fix the by-handle ATTR mechanisms to include the system.posix_acl_*
  xattrs.
- Hide the SGI_ACL_* xattrs from xattr lists.
- Add the SGI_ACL_* setxattr() validation as we're doing here for
  backwards compatibility, but include a deprecation log message warning
  in favor of using the above.
- At some point in the future, disallow the ability to get/set the
  SGI_ACL_* xattrs.

This also assumes there isn't some other reason we have SGI_ACL_*
exposed that I'm not aware of, which could certainly be the case.

Brian

> The patches in this queue don't "change" the format of those
> attributes, they only fix them for use in UID/GID  namespaces where
> they are currently broken. Validation when setting those xattrs could
> be removed, allowing sysadmins to set acls which the kernel and
> xfs_repair would reject; it seems rather pointless though.
> 
> Thanks,
> Andreas

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

[PATCH 3/4] xfs: SGI ACLs: Map uid/gid namespaces

[Andreas Gruenbacher]

Map uids and gids in the trusted.SGI_ACL_{FILE,DEFAULT} attributes between
the kernel and user-space namespaces.  This needs to be done in the
filesystem because the VFS is unaware of those attributes; for the standard
POSIX ACL attributes, the VFS takes care of that for us.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/xfs/xfs_acl.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 0eea7ee..64ffb85 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -39,7 +39,8 @@ STATIC struct posix_acl *
 xfs_acl_from_disk(
 	const struct xfs_acl	*aclp,
 	int			len,
-	int			max_entries)
+	int			max_entries,
+	struct user_namespace	*ns)
 {
 	struct posix_acl_entry *acl_e;
 	struct posix_acl *acl;
@@ -71,10 +72,10 @@ xfs_acl_from_disk(
 
 		switch (acl_e->e_tag) {
 		case ACL_USER:
-			acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
+			acl_e->e_uid = make_kuid(ns, be32_to_cpu(ace->ae_id));
 			break;
 		case ACL_GROUP:
-			acl_e->e_gid = xfs_gid_to_kgid(be32_to_cpu(ace->ae_id));
+			acl_e->e_gid = make_kgid(ns, be32_to_cpu(ace->ae_id));
 			break;
 		case ACL_USER_OBJ:
 		case ACL_GROUP_OBJ:
@@ -93,7 +94,10 @@ fail:
 }
 
 STATIC void
-xfs_acl_to_disk(struct xfs_acl *aclp, const struct posix_acl *acl)
+xfs_acl_to_disk(
+	struct xfs_acl		*aclp,
+	const struct posix_acl	*acl,
+	struct user_namespace	*ns)
 {
 	const struct posix_acl_entry *acl_e;
 	struct xfs_acl_entry *ace;
@@ -107,10 +111,10 @@ xfs_acl_to_disk(struct xfs_acl *aclp, const struct posix_acl *acl)
 		ace->ae_tag = cpu_to_be32(acl_e->e_tag);
 		switch (acl_e->e_tag) {
 		case ACL_USER:
-			ace->ae_id = cpu_to_be32(xfs_kuid_to_uid(acl_e->e_uid));
+			ace->ae_id = cpu_to_be32(from_kuid(ns, acl_e->e_uid));
 			break;
 		case ACL_GROUP:
-			ace->ae_id = cpu_to_be32(xfs_kgid_to_gid(acl_e->e_gid));
+			ace->ae_id = cpu_to_be32(from_kgid(ns, acl_e->e_gid));
 			break;
 		default:
 			ace->ae_id = cpu_to_be32(ACL_UNDEFINED_ID);
@@ -166,7 +170,8 @@ xfs_get_acl(struct inode *inode, int type)
 		goto out;
 	}
 
-	acl = xfs_acl_from_disk(xfs_acl, len, XFS_ACL_MAX_ENTRIES(ip->i_mount));
+	acl = xfs_acl_from_disk(xfs_acl, len, XFS_ACL_MAX_ENTRIES(ip->i_mount),
+				&init_user_ns);
 	if (IS_ERR(acl))
 		goto out;
 
@@ -205,7 +210,7 @@ __xfs_set_acl(struct inode *inode, int type, struct posix_acl *acl)
 		if (!xfs_acl)
 			return -ENOMEM;
 
-		xfs_acl_to_disk(xfs_acl, acl);
+		xfs_acl_to_disk(xfs_acl, acl, &init_user_ns);
 
 		/* subtract away the unused acl entries */
 		len -= sizeof(struct xfs_acl_entry) *
@@ -325,10 +330,11 @@ xfs_xattr_acl_get(struct dentry *dentry, const char *name,
 
 	error = XFS_ACL_SIZE(acl->a_count);
 	if (value) {
+		struct user_namespace *user_ns = current_user_ns();
 		if (error > size)
 			error = -ERANGE;
 		else
-			xfs_acl_to_disk(value, acl);
+			xfs_acl_to_disk(value, acl, user_ns);
 	}
 
 	posix_acl_release(acl);
@@ -351,7 +357,10 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
 		return value ? -EACCES : 0;
 
 	if (value) {
-		acl = xfs_acl_from_disk(value, size, XFS_ACL_MAX_ENTRIES(ip->i_mount));
+		struct user_namespace *user_ns = current_user_ns();
+		acl = xfs_acl_from_disk(value, size,
+					XFS_ACL_MAX_ENTRIES(ip->i_mount),
+					user_ns);
 		if (IS_ERR(acl))
 			return PTR_ERR(acl);
 
-- 
2.5.0

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

Re: [PATCH 3/4] xfs: SGI ACLs: Map uid/gid namespaces

[Dave Chinner]

On Sat, Oct 24, 2015 at 11:16:08PM +0200, Andreas Gruenbacher wrote:
> Map uids and gids in the trusted.SGI_ACL_{FILE,DEFAULT} attributes between
> the kernel and user-space namespaces.  This needs to be done in the
> filesystem because the VFS is unaware of those attributes; for the standard
> POSIX ACL attributes, the VFS takes care of that for us.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/xfs/xfs_acl.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 0eea7ee..64ffb85 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -39,7 +39,8 @@ STATIC struct posix_acl *
>  xfs_acl_from_disk(
>  	const struct xfs_acl	*aclp,
>  	int			len,
> -	int			max_entries)
> +	int			max_entries,
> +	struct user_namespace	*ns)
>  {
>  	struct posix_acl_entry *acl_e;
>  	struct posix_acl *acl;
> @@ -71,10 +72,10 @@ xfs_acl_from_disk(
>  
>  		switch (acl_e->e_tag) {
>  		case ACL_USER:
> -			acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
> +			acl_e->e_uid = make_kuid(ns, be32_to_cpu(ace->ae_id));

Please don't replace the xfs wrappers with the horribly named
generic functions. Pass the namespace to xfs_uid_to_kuid(), and
modify them, please. That way people who don't deal with namespaces
every day can tell exactly what format conversion is taking place
just by reading the code...

This namespace stuff is awful twisty. The posix layer does a user-ns
to init-ns conversion and here we do a no-op init-ns to init-ns
conversion. That needs comments in the code to explain exactly why
one path needs user-ns conversion and the other doesn't, because I'm
sure as hell not going to remember why these code paths are
different in 6 months time.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 3/4] xfs: SGI ACLs: Map uid/gid namespaces

[Andreas Gruenbacher]

On Mon, Oct 26, 2015 at 10:46 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Sat, Oct 24, 2015 at 11:16:08PM +0200, Andreas Gruenbacher wrote:
>> @@ -71,10 +72,10 @@ xfs_acl_from_disk(
>>
>>               switch (acl_e->e_tag) {
>>               case ACL_USER:
>> -                     acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
>> +                     acl_e->e_uid = make_kuid(ns, be32_to_cpu(ace->ae_id));
>
> Please don't replace the xfs wrappers with the horribly named
> generic functions. Pass the namespace to xfs_uid_to_kuid(), and
> modify them, please. That way people who don't deal with namespaces
> every day can tell exactly what format conversion is taking place
> just by reading the code...

We would effectively end up with:

  #define xfs_kuid_to_uid from_kuid
  #define xfs_kgid_to_gid from_kgid
  #define xfs_uid_to_kuid make_kuid
  #define xfs_gid_to_kgid make_kgid

Are you sure you really want that?

> This namespace stuff is awful twisty. The posix layer does a user-ns
> to init-ns conversion and here we do a no-op init-ns to init-ns
> conversion. That needs comments in the code to explain exactly why
> one path needs user-ns conversion and the other doesn't, because I'm
> sure as hell not going to remember why these code paths are
> different in 6 months time.

Okay.

Thanks,
Andreas

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

Re: [PATCH 3/4] xfs: SGI ACLs: Map uid/gid namespaces

[Dave Chinner]

On Tue, Oct 27, 2015 at 04:55:22PM +0100, Andreas Gruenbacher wrote:
> On Mon, Oct 26, 2015 at 10:46 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Sat, Oct 24, 2015 at 11:16:08PM +0200, Andreas Gruenbacher wrote:
> >> @@ -71,10 +72,10 @@ xfs_acl_from_disk(
> >>
> >>               switch (acl_e->e_tag) {
> >>               case ACL_USER:
> >> -                     acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
> >> +                     acl_e->e_uid = make_kuid(ns, be32_to_cpu(ace->ae_id));
> >
> > Please don't replace the xfs wrappers with the horribly named
> > generic functions. Pass the namespace to xfs_uid_to_kuid(), and
> > modify them, please. That way people who don't deal with namespaces
> > every day can tell exactly what format conversion is taking place
> > just by reading the code...
> 
> We would effectively end up with:
> 
>   #define xfs_kuid_to_uid from_kuid
>   #define xfs_kgid_to_gid from_kgid
>   #define xfs_uid_to_kuid make_kuid
>   #define xfs_gid_to_kgid make_kgid

No, you'd just add the namespace pointer to the static inline
functions we already have, and add a comment stating when the caller
should pass the init_ns vs user_ns.

But now that I've thought about this a bit more, I don't think that
the changes you've made are correct - we shouldn't be doing uid/gid
namespace conversion in disk formating functions. That is, the
conversion of user/group types should be done at the incoming layers
(i.e. at VFS/ioctl layers), not deep in the guts of the XFS code.

i.e. the disk formating functions should be passed uids/gids that
are already mapped to the init namespace, because there is no
guarantee that the formatting occurs in the same user context as the
syscall (e.g. low level disk formatting work gets deferred to a work
queue).

> Are you sure you really want that?

Why do you think we added the wrappers in the first place? It was
because the ns maintainer refused to follow standard, self
documenting type conversion naming conventions of <type>_to_<type>
so we added them ourselves. The user namespace code is horrible
enough without adding confusing type change functions everywhere...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 3/4] xfs: SGI ACLs: Map uid/gid namespaces

[Andreas Gruenbacher]

On Tue, Oct 27, 2015 at 8:55 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Tue, Oct 27, 2015 at 04:55:22PM +0100, Andreas Gruenbacher wrote:
>> On Mon, Oct 26, 2015 at 10:46 PM, Dave Chinner <david@fromorbit.com> wrote:
>> > On Sat, Oct 24, 2015 at 11:16:08PM +0200, Andreas Gruenbacher wrote:
>> >> @@ -71,10 +72,10 @@ xfs_acl_from_disk(
>> >>
>> >>               switch (acl_e->e_tag) {
>> >>               case ACL_USER:
>> >> -                     acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
>> >> +                     acl_e->e_uid = make_kuid(ns, be32_to_cpu(ace->ae_id));
>> >
>> > Please don't replace the xfs wrappers with the horribly named
>> > generic functions. Pass the namespace to xfs_uid_to_kuid(), and
>> > modify them, please. That way people who don't deal with namespaces
>> > every day can tell exactly what format conversion is taking place
>> > just by reading the code...
>>
>> We would effectively end up with:
>>
>>   #define xfs_kuid_to_uid from_kuid
>>   #define xfs_kgid_to_gid from_kgid
>>   #define xfs_uid_to_kuid make_kuid
>>   #define xfs_gid_to_kgid make_kgid
>
> No, you'd just add the namespace pointer to the static inline
> functions we already have, and add a comment stating when the caller
> should pass the init_ns vs user_ns.

Yep, same result though.

> But now that I've thought about this a bit more, I don't think that
> the changes you've made are correct - we shouldn't be doing uid/gid
> namespace conversion in disk formating functions. That is, the
> conversion of user/group types should be done at the incoming layers
> (i.e. at VFS/ioctl layers), not deep in the guts of the XFS code.

There are two kinds of code paths where we need to convert between the
SGI_ACL and the kernel's in-memory representation (struct posix_acl
*): one is in the get_acl and set_acl iops, converting to/from the
actual on-disk attrbutes, and the actual IDs stay the same. The other
is in the get and set SGI_ACL xattr handlers which are invoked from
the getxattr and setxattr iops. The conversion there is to/from the
user-facing SGI_ACL xattrs, and ID mappings may be in effect.

The VFS doesn't know anything about the SGI_ACL attributes, so XFS
will have to do the ID mappings itself. We could convert the IDS
in-place in separate pre- / post-passes over the xattr value and leave
xfs_acl_from_disk and xfs_acl_to_disk alone. That's what the VFS does
for POSIX ACLs. The problem there is that the setxattr iop and set
xattr handler give us a const void * value; we cannot modify it
without casting const away. Hence the additional namespace parameter
to xfs_acl_from_disk and xfs_acl_to_disk instead.

Only converting the IDs in place without going through the get_acl and
set_acl iops would not work very well because then the i_acl,
i_default_acl, and the permissions in i_mode wouldn't be updated
appropriately.

> i.e. the disk formating functions should be passed uids/gids that
> are already mapped to the init namespace, because there is no
> guarantee that the formatting occurs in the same user context as the
> syscall (e.g. low level disk formatting work gets deferred to a work
> queue).

There very well is a guarantee that the getxattr and setxattr
operations and thus the get and set xattr handler ops occur in the
appropriate context. The actual on-disk format conversions happen to
and from init_user_ns and they are effectively type casts.

>> Are you sure you really want that?
>
> Why do you think we added the wrappers in the first place? It was
> because the ns maintainer refused to follow standard, self
> documenting type conversion naming conventions of <type>_to_<type>
> so we added them ourselves. The user namespace code is horrible
> enough without adding confusing type change functions everywhere...

Well, so far these functions were at least hiding the &init_user_ns
part, they didn't just introduce XFS specific names for generic
things. It would be sad if every subsystem introduced their own names
when they don't like the generic ones.

Thanks,
Andreas

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

Re: [PATCH 3/4] xfs: SGI ACLs: Map uid/gid namespaces

[Dave Chinner]

On Tue, Oct 27, 2015 at 10:10:26PM +0100, Andreas Gruenbacher wrote:
> On Tue, Oct 27, 2015 at 8:55 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Tue, Oct 27, 2015 at 04:55:22PM +0100, Andreas Gruenbacher wrote:
> > But now that I've thought about this a bit more, I don't think that
> > the changes you've made are correct - we shouldn't be doing uid/gid
> > namespace conversion in disk formating functions. That is, the
> > conversion of user/group types should be done at the incoming layers
> > (i.e. at VFS/ioctl layers), not deep in the guts of the XFS code.
> 
> There are two kinds of code paths where we need to convert between the
> SGI_ACL and the kernel's in-memory representation (struct posix_acl
> *): one is in the get_acl and set_acl iops, converting to/from the
> actual on-disk attrbutes, and the actual IDs stay the same. The other
> is in the get and set SGI_ACL xattr handlers which are invoked from
> the getxattr and setxattr iops. The conversion there is to/from the
> user-facing SGI_ACL xattrs, and ID mappings may be in effect.

And then we have the ioctls XFS_IOC_ATTRLIST_BY_HANDLE and
XFS_IOC_ATTRMULTI_BY_HANDLE which can directly access the ACL xattrs
without even going through the the getxattr and setxattr iops. They
go direct to xfs_attr_get/xfs_attr_set() and set/return the xattrs
with *exactly* what the caller/on-disk xattr provides.

IOWs, you've only addressed one possible vector for direct access to
the SGI_ACL xattrs....

> The VFS doesn't know anything about the SGI_ACL attributes, so XFS
> will have to do the ID mappings itself. We could convert the IDS
> in-place in separate pre- / post-passes over the xattr value and leave
> xfs_acl_from_disk and xfs_acl_to_disk alone. That's what the VFS does
> for POSIX ACLs. The problem there is that the setxattr iop and set
> xattr handler give us a const void * value; we cannot modify it
> without casting const away. Hence the additional namespace parameter
> to xfs_acl_from_disk and xfs_acl_to_disk instead.

Yes, I read your patches - you take a low level disk formating
function and make it also work as a high level conversion function
through a different interface that has no higher level conversion
interface. i.e. you're changing the user visible behaviour of
writing a specific xattr.

But I think it's also wrong: users are not supposed to manipulate
SGI_ACL_FILE/SGI_ACL_DEFAULT xattrs directly.  Just because root can
see them doesn't mean users should be touching them - users should
be using the kernel posix ACL interface/namespace to modify their
ACLs.

OTOH, xfsdump/restore require direct, unfiltered access to set
uid/gid/mode/xattrs exactly as they are found. Changing that
behaviour like your change does will break xfsdump/restore and
breaking userspace is not a negotiable option.

> > Why do you think we added the wrappers in the first place? It was
> > because the ns maintainer refused to follow standard, self
> > documenting type conversion naming conventions of <type>_to_<type>
> > so we added them ourselves. The user namespace code is horrible
> > enough without adding confusing type change functions everywhere...
> 
> Well, so far these functions were at least hiding the &init_user_ns
> part, they didn't just introduce XFS specific names for generic
> things.

That was precisely the reason they were introduced - the higher
levels should already be doing namespace conversions before anything
gets to the filesystem disk formatting routines. Hence the
&init_user_ns bullshit for getting a raw uid is just noise.

> It would be sad if every subsystem introduced their own names
> when they don't like the generic ones.

It was a symptom of the much larger problem: the userns maintainer
refusing or ignoring *all* requests to change anything or answer any
questions about how things like dump/restore is supposed to work
across user namespaces. Hence, for better or for worse, we were
forced to make up our own rules on how dump/restore and mapped ids
are supposed to interact....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 3/4] xfs: SGI ACLs: Map uid/gid namespaces

[Andreas Gruenbacher]

On Tue, Oct 27, 2015 at 11:37 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Tue, Oct 27, 2015 at 10:10:26PM +0100, Andreas Gruenbacher wrote:
>> On Tue, Oct 27, 2015 at 8:55 PM, Dave Chinner <david@fromorbit.com> wrote:
>> > On Tue, Oct 27, 2015 at 04:55:22PM +0100, Andreas Gruenbacher wrote:
>> > But now that I've thought about this a bit more, I don't think that
>> > the changes you've made are correct - we shouldn't be doing uid/gid
>> > namespace conversion in disk formating functions. That is, the
>> > conversion of user/group types should be done at the incoming layers
>> > (i.e. at VFS/ioctl layers), not deep in the guts of the XFS code.
>>
>> There are two kinds of code paths where we need to convert between the
>> SGI_ACL and the kernel's in-memory representation (struct posix_acl
>> *): one is in the get_acl and set_acl iops, converting to/from the
>> actual on-disk attrbutes, and the actual IDs stay the same. The other
>> is in the get and set SGI_ACL xattr handlers which are invoked from
>> the getxattr and setxattr iops. The conversion there is to/from the
>> user-facing SGI_ACL xattrs, and ID mappings may be in effect.
>
> And then we have the ioctls XFS_IOC_ATTRLIST_BY_HANDLE and
> XFS_IOC_ATTRMULTI_BY_HANDLE which can directly access the ACL xattrs
> without even going through the the getxattr and setxattr iops. They
> go direct to xfs_attr_get/xfs_attr_set() and set/return the xattrs
> with *exactly* what the caller/on-disk xattr provides.
>
> IOWs, you've only addressed one possible vector for direct access to
> the SGI_ACL xattrs....

Ah, I've missed those ioctls.

>> The VFS doesn't know anything about the SGI_ACL attributes, so XFS
>> will have to do the ID mappings itself. We could convert the IDS
>> in-place in separate pre- / post-passes over the xattr value and leave
>> xfs_acl_from_disk and xfs_acl_to_disk alone. That's what the VFS does
>> for POSIX ACLs. The problem there is that the setxattr iop and set
>> xattr handler give us a const void * value; we cannot modify it
>> without casting const away. Hence the additional namespace parameter
>> to xfs_acl_from_disk and xfs_acl_to_disk instead.
>
> Yes, I read your patches - you take a low level disk formating
> function and make it also work as a high level conversion function
> through a different interface that has no higher level conversion
> interface. i.e. you're changing the user visible behaviour of
> writing a specific xattr.
>
> But I think it's also wrong: users are not supposed to manipulate
> SGI_ACL_FILE/SGI_ACL_DEFAULT xattrs directly.  Just because root can
> see them doesn't mean users should be touching them - users should
> be using the kernel posix ACL interface/namespace to modify their
> ACLs.
>
> OTOH, xfsdump/restore require direct, unfiltered access to set
> uid/gid/mode/xattrs exactly as they are found.

Huh?

First, do we even agree that setting the SGI_ACL attributes, through
whatever interface, must not leave inode->i_acl or inode->i_default
stale?

Second, the system.posix_acl_access and trusted.SGI_ACL_FILE
attributes both contain the Access ACL, which by definition also
contains the file mode permission bits. When any of those two
attributes is set, what sense would it make to not also set the file
mode permission bits --- and to allow them to become inconsistent with
the Acess ACL? Certainly none in the xfsdump/restore case because in
the dump, the mode and Access ACL will agree, anyway.

> Changing that behaviour like your change does will break xfsdump/restore

I would be most surprised.

Thanks,
Andreas

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

[PATCH 4/4] xfs: SGI ACLs: Prepare for richacls

[Andreas Gruenbacher]

In case an inode has trusted.SGI_ACL_* attributes but POSIX ACLs are not
enabled, treat those attributes as normal trusted attributes and bypass the
get_acl and set_acl inode operations to prevent corrupting inode->i_mode,
inode->i_acl, or inode->i_default_acl.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/xfs/xfs_acl.c   |  5 +++++
 fs/xfs/xfs_xattr.c |  5 +++--
 fs/xfs/xfs_xattr.h | 28 ++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)
 create mode 100644 fs/xfs/xfs_xattr.h

diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 64ffb85..bee1493 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -21,6 +21,7 @@
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
 #include "xfs_inode.h"
+#include "xfs_xattr.h"
 #include "xfs_acl.h"
 #include "xfs_attr.h"
 #include "xfs_trace.h"
@@ -319,6 +320,8 @@ xfs_xattr_acl_get(struct dentry *dentry, const char *name,
 	struct posix_acl *acl;
 	int error;
 
+	if (!IS_POSIXACL(inode))
+		return xfs_xattr_get(dentry, name, value, size, type);
 	if (S_ISLNK(inode->i_mode))
 		return -EOPNOTSUPP;
 
@@ -350,6 +353,8 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
 	struct posix_acl *acl = NULL;
 	int error;
 
+	if (!IS_POSIXACL(inode))
+		return xfs_xattr_set(dentry, name, value, size, flags, type);
 	if (!inode->i_op->set_acl)
 		return -EOPNOTSUPP;
 
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index 7534cb5..78540c3 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -25,13 +25,14 @@
 #include "xfs_inode.h"
 #include "xfs_attr.h"
 #include "xfs_attr_leaf.h"
+#include "xfs_xattr.h"
 #include "xfs_acl.h"
 
 #include <linux/posix_acl_xattr.h>
 #include <linux/xattr.h>
 
 
-static int
+int
 xfs_xattr_get(struct dentry *dentry, const char *name,
 		void *value, size_t size, int xflags)
 {
@@ -53,7 +54,7 @@ xfs_xattr_get(struct dentry *dentry, const char *name,
 	return asize;
 }
 
-static int
+int
 xfs_xattr_set(struct dentry *dentry, const char *name, const void *value,
 		size_t size, int flags, int xflags)
 {
diff --git a/fs/xfs/xfs_xattr.h b/fs/xfs/xfs_xattr.h
new file mode 100644
index 0000000..69560052
--- /dev/null
+++ b/fs/xfs/xfs_xattr.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2001-2005 Silicon Graphics, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+#ifndef __XFS_XATTR_H__
+#define __XFS_XATTR_H__
+
+struct dentry;
+
+extern int xfs_xattr_get(struct dentry *dentry, const char *name, void *value,
+			 size_t size, int xflags);
+extern int xfs_xattr_set(struct dentry *dentry, const char *name,
+			 const void *value, size_t size, int flags, int xflags);
+
+#endif	/* __XFS_XATTR_H__ */
-- 
2.5.0

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

Re: [PATCH 4/4] xfs: SGI ACLs: Prepare for richacls

[Andreas Gruenbacher]

On Sat, Oct 24, 2015 at 11:16 PM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> In case an inode has trusted.SGI_ACL_* attributes but POSIX ACLs are not
> enabled, treat those attributes as normal trusted attributes and bypass the
> get_acl and set_acl inode operations to prevent corrupting inode->i_mode,
> inode->i_acl, or inode->i_default_acl.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/xfs/xfs_acl.c   |  5 +++++
>  fs/xfs/xfs_xattr.c |  5 +++--
>  fs/xfs/xfs_xattr.h | 28 ++++++++++++++++++++++++++++
>  3 files changed, 36 insertions(+), 2 deletions(-)
>  create mode 100644 fs/xfs/xfs_xattr.h
>
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 64ffb85..bee1493 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -21,6 +21,7 @@
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
> +#include "xfs_xattr.h"
>  #include "xfs_acl.h"
>  #include "xfs_attr.h"
>  #include "xfs_trace.h"
> @@ -319,6 +320,8 @@ xfs_xattr_acl_get(struct dentry *dentry, const char *name,
>         struct posix_acl *acl;
>         int error;
>
> +       if (!IS_POSIXACL(inode))
> +               return xfs_xattr_get(dentry, name, value, size, type);

This doesn't give us UID/GID mapping. In-place rewriting as the VFS
does would help, but the value is a const * here so we cannot really
modify it.

>         if (S_ISLNK(inode->i_mode))
>                 return -EOPNOTSUPP;
>


Andreas

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

Re: [PATCH 0/4] xfs: SGI ACL Fixes

[Brian Foster]

On Sat, Oct 24, 2015 at 11:16:05PM +0200, Andreas Gruenbacher wrote:
> Here are some fixes for the trusted.SGI_ACL_* attributes.  This adds some
> more warts and it would be much better to get rid of those unnecessary
> attributes instead; we probably can't though.
> 
> I have tested this manually but haven't run xfstests against that.  Please
> review.
> 

Thanks... I sent some comments on the core patch. Those aside, I'd still
like to see some other opinions on the best way to handle this. I'd
suggest we get some agreement there before getting too far with this.
The options seem to be:

1.) Hide the internal xattrs and disallow setting them. This is probably
the most simple fix, but removes the entries after they had previously
been exposed.

2.) Redirect the internal xattr handlers through the system.posix_acl_*
handlers. This (presumably) simplifies the XFS implementation and
eliminates duplicating some of the VFS acl bits in the fs. This
preserves xattr existence but changes the exposed format to the generic
ACL format. Therefore, it very well could be pointless in favor of
option 1 or 3.

3.) Make the internal xattr handlers work correctly with the format as
exposed today. This is what this series aims to do and requires some of
the additional namespace quirkiness and whatnot by expecting XFS on-disk
format data from userspace.

Thoughts?

Brian

> Thanks,
> Andreas
> 
> Andreas Gruenbacher (4):
>   xfs: Validate the length of on-disk ACLs
>   xfs: SGI ACLs: Fix caching and mode setting
>   xfs: SGI ACLs: Map uid/gid namespaces
>   xfs: SGI ACLs: Prepare for richacls
> 
>  fs/xfs/libxfs/xfs_format.h |   8 +++-
>  fs/xfs/xfs_acl.c           | 115 ++++++++++++++++++++++++++++++++++++++++-----
>  fs/xfs/xfs_acl.h           |   3 ++
>  fs/xfs/xfs_xattr.c         |   9 ++--
>  fs/xfs/xfs_xattr.h         |  28 +++++++++++
>  5 files changed, 147 insertions(+), 16 deletions(-)
>  create mode 100644 fs/xfs/xfs_xattr.h
> 
> -- 
> 2.5.0
> 

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Dave Chinner]

On Sat, Oct 24, 2015 at 11:22:55AM -0400, Brian Foster wrote:
> On Sat, Oct 24, 2015 at 03:58:04PM +0200, Andreas Gruenbacher wrote:
> > On Sat, Oct 24, 2015 at 2:57 PM, Brian Foster <bfoster@redhat.com> wrote:
> > > An alternative could be to just disallow setting these xattrs directly.
> > 
> > Probably not because that would cause applications to fail in
> > unexpected new ways.
> > 
> 
> I suppose a backup/restore application might want to set these, but I'm
> not aware of any other sane usage given they're in a filesystem specific
> format at this point. We'd probably have to take a look at xfsdump, see
> how it handles this, then see if there's a clean way to run through
> necessary acl bits if we're called via setxattr().

xfsrestore restores all the xattrs via setxattr(). It does not care
what they contain, it just restores them with the appropriate
namespace flags (ATTR_ROOT, ATTR_SECURE or 0) that they had when
read by xfsdump. So we cannot disable this functionality without
breaking dump/restore.

Really, I'm struggling to understand what the problem is with XFS
doing translation to it's own special xattr names for ACLs
underneath the posix layer. Yes, there's a caching issue when
someone directly manipulates the underlying xattr, but you need root
to shoot yourself in the foot that way, and that is easily
solveable.

What other problems are there?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Andreas Gruenbacher]

On Mon, Oct 26, 2015 at 10:32 PM, Dave Chinner <david@fromorbit.com> wrote:
> Really, I'm struggling to understand what the problem is with XFS
> doing translation to it's own special xattr names for ACLs
> underneath the posix layer.

Right now, setting one of the SGI_ACL attributes leads to stale i_acl
/ i_default_acl fields and in the case of SGI_ACL_FILE, possibly to
outdated permissions in i_mode. You would get different information
from getfacl than what's stored on disk.

> Yes, there's a caching issue when someone directly manipulates
> the underlying xattr,

"Directly manipulating" could be doing a setxattr of an attribute that
was previously retrieved by getxattr, like restoring a backup.

> but you need root to shoot yourself in the foot that way, and that is easily
> solveable.

What do you mean, it's easily solvable?

Thanks,
Andreas

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Dave Chinner]

On Tue, Oct 27, 2015 at 12:52:10AM +0100, Andreas Gruenbacher wrote:
> On Mon, Oct 26, 2015 at 10:32 PM, Dave Chinner <david@fromorbit.com> wrote:
> > Really, I'm struggling to understand what the problem is with XFS
> > doing translation to it's own special xattr names for ACLs
> > underneath the posix layer.
> 
> Right now, setting one of the SGI_ACL attributes leads to stale i_acl
> / i_default_acl fields and in the case of SGI_ACL_FILE, possibly to
> outdated permissions in i_mode. You would get different information
> from getfacl than what's stored on disk.

That's because we're not marking the cached acl as stale when
setting the acl directly...

> > Yes, there's a caching issue when someone directly manipulates
> > the underlying xattr,
> 
> "Directly manipulating" could be doing a setxattr of an attribute that
> was previously retrieved by getxattr, like restoring a backup.

Sure, that's what xfsdump/restore effectively does.

> > but you need root to shoot yourself in the foot that way, and that is easily
> > solveable.
> 
> What do you mean, it's easily solvable?

forget_all_cached_acls()

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Andreas Gruenbacher]

On Tue, Oct 27, 2015 at 6:30 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Tue, Oct 27, 2015 at 12:52:10AM +0100, Andreas Gruenbacher wrote:
>> On Mon, Oct 26, 2015 at 10:32 PM, Dave Chinner <david@fromorbit.com> wrote:
>> > Really, I'm struggling to understand what the problem is with XFS
>> > doing translation to it's own special xattr names for ACLs
>> > underneath the posix layer.
>>
>> Right now, setting one of the SGI_ACL attributes leads to stale i_acl
>> / i_default_acl fields and in the case of SGI_ACL_FILE, possibly to
>> outdated permissions in i_mode. You would get different information
>> from getfacl than what's stored on disk.
>
> That's because we're not marking the cached acl as stale when
> setting the acl directly...
>
>> > Yes, there's a caching issue when someone directly manipulates
>> > the underlying xattr,
>>
>> "Directly manipulating" could be doing a setxattr of an attribute that
>> was previously retrieved by getxattr, like restoring a backup.
>
> Sure, that's what xfsdump/restore effectively does.
>
>> > but you need root to shoot yourself in the foot that way, and that is easily
>> > solveable.
>>
>> What do you mean, it's easily solvable?
>
> forget_all_cached_acls()

Brian has already suggested that in this thread. Still leaves the
i_mode permission bits stale and is broken wrt. uid/gid namespaces.

Andreas

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Dave Chinner]

On Tue, Oct 27, 2015 at 11:56:46AM +0100, Andreas Gruenbacher wrote:
> On Tue, Oct 27, 2015 at 6:30 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Tue, Oct 27, 2015 at 12:52:10AM +0100, Andreas Gruenbacher wrote:
> >> On Mon, Oct 26, 2015 at 10:32 PM, Dave Chinner <david@fromorbit.com> wrote:
> >> > Really, I'm struggling to understand what the problem is with XFS
> >> > doing translation to it's own special xattr names for ACLs
> >> > underneath the posix layer.
> >>
> >> Right now, setting one of the SGI_ACL attributes leads to stale i_acl
> >> / i_default_acl fields and in the case of SGI_ACL_FILE, possibly to
> >> outdated permissions in i_mode. You would get different information
> >> from getfacl than what's stored on disk.
> >
> > That's because we're not marking the cached acl as stale when
> > setting the acl directly...
> >
> >> > Yes, there's a caching issue when someone directly manipulates
> >> > the underlying xattr,
> >>
> >> "Directly manipulating" could be doing a setxattr of an attribute that
> >> was previously retrieved by getxattr, like restoring a backup.
> >
> > Sure, that's what xfsdump/restore effectively does.
> >
> >> > but you need root to shoot yourself in the foot that way, and that is easily
> >> > solveable.
> >>
> >> What do you mean, it's easily solvable?
> >
> > forget_all_cached_acls()
> 
> Brian has already suggested that in this thread. Still leaves the
> i_mode permission bits stale and is broken wrt. uid/gid namespaces.

But for xfsrestore we don't want to do that because it's already
set the mode correctly. Indeed, we order operations in xfs_restore
to prevent the kernel from fucking with the inode modes and capabilities
and giving use the incorrect result once the backup is complete. e.g.:

struct stream_context {
        bstat_t    sc_bstat;
        char       sc_path[2 * MAXPATHLEN];
        int   sc_fd;
        int   sc_hsmflags;

        /*
         * we have to set the owner before we set extended attributes otherwise
         * capabilities will not be restored correctly as setting the owner with
         * fchmod will strip the capability attribute from the file. Hence we
         * need to do this before restoring xattrs and record it so we don't do
         * it again on completion of file restoration.
         */
        bool_t     sc_ownerset;
};


Further, user namespaces are irrelevant here - you can't run
xfsdump/restore outside the init_ns.  xfsdump requires access to the
handle interface, which is unsafe to use inside a user ns because it
allows complete access to any inode in the filesystem without
limitations. xfs_restore requires unfettered access to directly
manipulate the uid/gid/security attrs of inodes, which once again is
something that isn't allowed inside user namespaces.

Setting Posix acls by directly poking the on-disk attr format rather
than going through the proper kernel ACL namespace is not a *general
purpose user interface*.  Thi exists for backup/restore utilities to
do things like restore ACLs and security labels simply by treating
them as opaque xattrs.  If a user sets ACLs using this low level
"opaque xattr" method, then they get to keep all the broken bits to
themselves.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Andreas Gruenbacher]

On Tue, Oct 27, 2015 at 9:18 PM, Dave Chinner <david@fromorbit.com> wrote:
> Further, user namespaces are irrelevant here - you can't run
> xfsdump/restore outside the init_ns.  xfsdump requires access to the
> handle interface, which is unsafe to use inside a user ns because it
> allows complete access to any inode in the filesystem without
> limitations. xfs_restore requires unfettered access to directly
> manipulate the uid/gid/security attrs of inodes, which once again is
> something that isn't allowed inside user namespaces.
>
> Setting Posix acls by directly poking the on-disk attr format rather
> than going through the proper kernel ACL namespace is not a *general
> purpose user interface*.  Thi exists for backup/restore utilities to
> do things like restore ACLs and security labels simply by treating
> them as opaque xattrs.  If a user sets ACLs using this low level
> "opaque xattr" method, then they get to keep all the broken bits to
> themselves.

Any process capable of CAP_SYS_ADMIN can getxattr and setxattr those
xattrs, it doesn't matter whether it's xfsdump/restore or something
else. Some will mess with those attributes simply because listxattr
lists them and so they will get and set them like all other
attributes. Setting those attributes must not leave the system in an
inconsistent state, independent of what xfsdump/restore happens to do
with them.

Thanks,
Andreas

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Dave Chinner]

On Tue, Oct 27, 2015 at 10:39:51PM +0100, Andreas Gruenbacher wrote:
> On Tue, Oct 27, 2015 at 9:18 PM, Dave Chinner <david@fromorbit.com> wrote:
> > Further, user namespaces are irrelevant here - you can't run
> > xfsdump/restore outside the init_ns.  xfsdump requires access to the
> > handle interface, which is unsafe to use inside a user ns because it
> > allows complete access to any inode in the filesystem without
> > limitations. xfs_restore requires unfettered access to directly
> > manipulate the uid/gid/security attrs of inodes, which once again is
> > something that isn't allowed inside user namespaces.
> >
> > Setting Posix acls by directly poking the on-disk attr format rather
> > than going through the proper kernel ACL namespace is not a *general
> > purpose user interface*.  Thi exists for backup/restore utilities to
> > do things like restore ACLs and security labels simply by treating
> > them as opaque xattrs.  If a user sets ACLs using this low level
> > "opaque xattr" method, then they get to keep all the broken bits to
> > themselves.
> 
> Any process capable of CAP_SYS_ADMIN can getxattr and setxattr those

CAP_SYS_ADMIN = enough rope to hang yourself.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Brian Foster]

On Tue, Oct 27, 2015 at 04:30:45PM +1100, Dave Chinner wrote:
> On Tue, Oct 27, 2015 at 12:52:10AM +0100, Andreas Gruenbacher wrote:
> > On Mon, Oct 26, 2015 at 10:32 PM, Dave Chinner <david@fromorbit.com> wrote:
> > > Really, I'm struggling to understand what the problem is with XFS
> > > doing translation to it's own special xattr names for ACLs
> > > underneath the posix layer.
> > 
> > Right now, setting one of the SGI_ACL attributes leads to stale i_acl
> > / i_default_acl fields and in the case of SGI_ACL_FILE, possibly to
> > outdated permissions in i_mode. You would get different information
> > from getfacl than what's stored on disk.
> 
> That's because we're not marking the cached acl as stale when
> setting the acl directly...
> 
> > > Yes, there's a caching issue when someone directly manipulates
> > > the underlying xattr,
> > 
> > "Directly manipulating" could be doing a setxattr of an attribute that
> > was previously retrieved by getxattr, like restoring a backup.
> 
> Sure, that's what xfsdump/restore effectively does.
> 
> > > but you need root to shoot yourself in the foot that way, and that is easily
> > > solveable.
> > 
> > What do you mean, it's easily solvable?
> 
> forget_all_cached_acls()
> 

This is essentially what the test patch I posted up thread does. Andreas
points out this is not sufficient because it doesn't update i_mode when
it is necessary based on the ACLs. To demonstrate:

Set an ACL and observe i_mode:

# touch /mnt/file
# ls -ali /mnt/file 
99 -rw-r--r-- 1 root root 0 Oct 27 06:48 /mnt/file
# setfacl -m u:fsgqa:rwx /mnt/file 
# ls -ali /mnt/file 
99 -rw-rwxr--+ 1 root root 0 Oct 27 06:48 /mnt/file

Grab the underlying xattr data:

# getfattr -m - -d /mnt/file 
getfattr: Removing leading '/' from absolute path names
# file: mnt/file
system.posix_acl_access=0sAgAAAAEABgD/////AgAHAOgDAAAEAAQA/////xAABwD/////IAAEAP////8=
trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAcAAAAAAAT/////AAQAAAAAABD/////AAcAAAAAACD/////AAQAAA==

Remove the ACL, observe i_mode reset:

# setfacl -x u:fsgqa /mnt/file 
# ls -ali /mnt/file 
99 -rw-r--r--+ 1 root root 0 Oct 27 06:49 /mnt/file

Set the xattr directly to re-add the ACL (we know the change is not
going to show up here due to the caching problem):

# setfattr -n trusted.SGI_ACL_FILE -v 0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAcAAAAAAAT/////AAQAAAAAABD/////AAcAAAAAACD/////AAQAAA== /mnt/file
# ls -ali /mnt/file 
99 -rw-r--r--+ 1 root root 0 Oct 27 06:49 /mnt/file

Remount, observe the ACL is back yet i_mode is still wrong:

# umount /mnt ; mount /dev/test/scratch /mnt/
# ls -ali /mnt/file 
99 -rw-r--r--+ 1 root root 0 Oct 27 06:49 /mnt/file
# getfacl /mnt/file 
getfacl: Removing leading '/' from absolute path names
# file: mnt/file
# owner: root
# group: root
user::rw-
user:fsgqa:rwx
group::r--
mask::rwx
other::r--

So it's permanently inconsistent afaics, at least until somebody uses
the proper ACL interface on that file. I'm not familiar enough with how
this mechanism works to know what the consequences of the i_mode
inconsistency are (e.g., after a remount)... Andreas?

Andreas,

Dave and I discussed this a bit on IRC yesterday, along with things like
the idea of starting to use the system.posix_acl_* names on disk
directly, just converting the xattr data, and transitioning the
trusted.SGI_ACL_* ACLs to be the virtual ACLs (as posix_acl_* are now).
The challenge with this is that we still need to provide backwards
compatibility for the SGI_ACL_* xattrs on disk for quite some time,
probably need a feature bit for the new xattr name, etc. Dave believes
it's not worth doing that to fix this problem. My preference is to fix
it however appropriate (with a feature bit, etc.) so we can eventually
kill the old mechanism, however long it must stay around.

I think we missed the fact that i_mode is not updated as demonstrated
above. If the consequences of that are explicit breakage (?), that would
make me feel even less bad about just disabling/changing a broken
mechanism, but we probably need more information on that. Regardless, I
think the lowest common denominator here is just to do what we're doing
in this series as is (with suggested code cleanups and whatnot) and fix
the SGI_ACL_* handlers to do the right thing, unless Dave chimes in
otherwise.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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