| Message ID | 1444604337-17651-25-git-send-email-andreas.gruenbacher@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Oct 12, 2015 at 12:58:35AM +0200, Andreas Gruenbacher wrote: > From: Andreas Gruenbacher <agruenba@redhat.com> > > The richacl feature flag (mkfs.xfs -m richacl=1) determines whether an xfs > filesystem supports posix acls or richacls. Richacls are stored in > "system.richacl" xattrs. > > If richacls are not compiled into the kernel, mounting richacl filesystems > will fail. > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > fs/ext4/richacl.h | 1 - > fs/xfs/Kconfig | 11 ++++++ > fs/xfs/Makefile | 1 + > fs/xfs/libxfs/xfs_format.h | 16 +++++++- > fs/xfs/xfs_iops.c | 42 +++++++++++++++----- > fs/xfs/xfs_richacl.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++ > fs/xfs/xfs_richacl.h | 34 ++++++++++++++++ > fs/xfs/xfs_super.c | 14 ++++++- > fs/xfs/xfs_super.h | 9 ++++- > fs/xfs/xfs_xattr.c | 4 ++ > 10 files changed, 215 insertions(+), 14 deletions(-) > create mode 100644 fs/xfs/xfs_richacl.c > create mode 100644 fs/xfs/xfs_richacl.h > > diff --git a/fs/ext4/richacl.h b/fs/ext4/richacl.h > index fc7826f..f26fbdd 100644 > --- a/fs/ext4/richacl.h > +++ b/fs/ext4/richacl.h > @@ -24,7 +24,6 @@ > > extern struct richacl *ext4_get_richacl(struct inode *); > extern int ext4_set_richacl(struct inode *, struct richacl *); > - > extern int ext4_init_richacl(handle_t *, struct inode *, struct inode *); > > #else /* CONFIG_EXT4_FS_RICHACL */ stray hunk. > diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig > index 5d47b4d..05dd312 100644 > --- a/fs/xfs/Kconfig > +++ b/fs/xfs/Kconfig > @@ -52,6 +52,17 @@ config XFS_POSIX_ACL > > If you don't know what Access Control Lists are, say N. > > +config XFS_RICHACL > + bool "XFS Rich Access Control Lists (EXPERIMENTAL)" > + depends on XFS_FS > + select FS_RICHACL > + help > + Richacls are an implementation of NFSv4 ACLs, extended by file masks > + to cleanly integrate into the POSIX file permission model. To learn > + more about them, see http://www.bestbits.at/richacl/. > + > + If you don't know what Richacls are, say N. > + So now we have multiple compile time ACL configs that we have to test. Personally, I see no reason for making either posix or rich acls compile time dependent. How about we just change CONFIG_FS_XFS to have "select FS_POSIX_ACL" and "select FS_RICHACL" unconditionally, and then we can get rid of all the conditional code? > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 9590a06..8c6da45 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -461,10 +461,18 @@ xfs_sb_has_ro_compat_feature( > #define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */ > #define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */ > #define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */ > + > +#ifdef CONFIG_XFS_RICHACL > +#define XFS_SB_FEAT_INCOMPAT_RICHACL (1 << 3) /* richacls */ > +#else > +#define XFS_SB_FEAT_INCOMPAT_RICHACL 0 > +#endif > + Regardless of whether we build in richacl support, this is wrong. Feature bits are on-disk format definitions, so it will always have this value whether or not the kernel supports the feature. > @@ -722,7 +737,10 @@ xfs_setattr_nonsize( > * Posix ACL code seems to care about this issue either. > */ > if ((mask & ATTR_MODE) && !(flags & XFS_ATTR_NOACL)) { > - error = posix_acl_chmod(inode, inode->i_mode); > + if (XFS_IS_RICHACL(inode)) > + error = richacl_chmod(inode, inode->i_mode); > + else > + error = posix_acl_chmod(inode, inode->i_mode); > if (error) > return error; > } This sort of thing could do with a helper like: static inline int xfs_acl_chmod(struct inode *inode, int mode) { if (IS_RICHACL(inode)) return richacl_chmod(inode, mode); return posix_acl_chmod(inode, mode); } > +#include "xfs.h" > +#include "xfs_format.h" > +#include "xfs_log_format.h" > +#include "xfs_inode.h" > +#include "xfs_attr.h" > + > +#include <linux/richacl_xattr.h> > + > +struct richacl * > +xfs_get_richacl(struct inode *inode) > +{ ... If we always build in ACL support, this can all go in xfs_acl.c. > + struct xfs_inode *ip = XFS_I(inode); > + struct richacl *acl; > + int size = XATTR_SIZE_MAX; > + void *value; > + int error; > + > + value = kmem_zalloc_large(size, KM_SLEEP); > + if (!value) > + return ERR_PTR(-ENOMEM); > + > + error = xfs_attr_get(ip, XATTR_NAME_RICHACL, value, &size, ATTR_ROOT); > + if (error) { > + /* > + * If the attribute doesn't exist make sure we have a negative > + * cache entry, for any other error assume it is transient and > + * leave the cache entry as ACL_NOT_CACHED. > + */ > + acl = (error == -ENOATTR) ? NULL : ERR_PTR(error); I rather dislike ternaries in code like this - it's hard to read the logic. Initalise acl to NULL, and then this simply becomes: if (error != -ENOATTR) acl = ERR_PTR(error); > + } else > + acl = richacl_from_xattr(&init_user_ns, value, size); > + > + if (!IS_ERR(acl)) > + set_cached_richacl(inode, acl); > + kfree(value); > + > + return acl; > +} > + > +int > +xfs_set_richacl(struct inode *inode, struct richacl *acl) > +{ > + struct xfs_inode *ip = XFS_I(inode); > + umode_t mode = inode->i_mode; > + int error; > + > + if (acl) { > + /* Don't allow acls with unmapped identifiers. */ > + if (richacl_has_unmapped_identifiers(acl)) > + return -EINVAL; > + > + if (richacl_equiv_mode(acl, &mode) == 0) { > + xfs_set_mode(inode, mode); > + acl = NULL; > + } Comment needed here - why does this now seem to accidentally fall through to removing the ACL? Also, why is this in the XFS specific code when it's generic richacl functionality that is being checked? Also, I really dislike the API where passing a NULL acl means to "set this acl" actually means "remove the existing ACL". Why no ->remove_acl method called from the generic code? > +#ifndef __FS_XFS_RICHACL_H > +#define __FS_XFS_RICHACL_H > + > +#include <linux/richacl.h> > + > +#ifdef CONFIG_XFS_RICHACL > + > +#define XFS_IS_RICHACL(inode) IS_RICHACL(inode) No. Just use IS_RICHACL() everywhere, and whatever build conditional that uses. > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1500,7 +1500,19 @@ xfs_fs_fill_super( > sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits); > sb->s_max_links = XFS_MAXLINK; > sb->s_time_gran = 1; > - set_posix_acl_flag(sb); > + > + if (xfs_sb_version_hasrichacl(&mp->m_sb)) { > +#ifdef CONFIG_XFS_RICHACL > + sb->s_flags |= MS_RICHACL; > +#else > + error = -EOPNOTSUPP; > + goto out_free_sb; > +#endif > + } else { > +#ifdef CONFIG_XFS_POSIX_ACL > + sb->s_flags |= MS_POSIXACL; > +#endif The whole point of having set_posix_acl_flag() is that it keeps the ifdef hackery out of the code. If we are going to keep build time config options for these, then please keep the wrappers to hide this mess... Cheers, Dave.
2015-10-12 2:10 GMT+02:00 Dave Chinner <david@fromorbit.com>: > On Mon, Oct 12, 2015 at 12:58:35AM +0200, Andreas Gruenbacher wrote: >> diff --git a/fs/ext4/richacl.h b/fs/ext4/richacl.h >> index fc7826f..f26fbdd 100644 >> --- a/fs/ext4/richacl.h >> +++ b/fs/ext4/richacl.h >> @@ -24,7 +24,6 @@ >> >> extern struct richacl *ext4_get_richacl(struct inode *); >> extern int ext4_set_richacl(struct inode *, struct richacl *); >> - >> extern int ext4_init_richacl(handle_t *, struct inode *, struct inode *); >> >> #else /* CONFIG_EXT4_FS_RICHACL */ > > stray hunk. Oops, thanks. >> diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig >> index 5d47b4d..05dd312 100644 >> --- a/fs/xfs/Kconfig >> +++ b/fs/xfs/Kconfig >> @@ -52,6 +52,17 @@ config XFS_POSIX_ACL >> >> If you don't know what Access Control Lists are, say N. >> >> +config XFS_RICHACL >> + bool "XFS Rich Access Control Lists (EXPERIMENTAL)" >> + depends on XFS_FS >> + select FS_RICHACL >> + help >> + Richacls are an implementation of NFSv4 ACLs, extended by file masks >> + to cleanly integrate into the POSIX file permission model. To learn >> + more about them, see http://www.bestbits.at/richacl/. >> + >> + If you don't know what Richacls are, say N. >> + > > So now we have multiple compile time ACL configs that we have to > test. Personally, I see no reason for making either posix or rich > acls compile time dependent. How about we just change CONFIG_FS_XFS > to have "select FS_POSIX_ACL" and "select FS_RICHACL" > unconditionally, and then we can get rid of all the conditional > code? I'm sure neither kind of ACLs makes sense on many tiny systems, it should be possible to disable them. XFS may not make sense on those kinds of systems either, that is not my call to make though. >> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h >> index 9590a06..8c6da45 100644 >> --- a/fs/xfs/libxfs/xfs_format.h >> +++ b/fs/xfs/libxfs/xfs_format.h >> @@ -461,10 +461,18 @@ xfs_sb_has_ro_compat_feature( >> #define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */ >> #define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */ >> #define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */ >> + >> +#ifdef CONFIG_XFS_RICHACL >> +#define XFS_SB_FEAT_INCOMPAT_RICHACL (1 << 3) /* richacls */ >> +#else >> +#define XFS_SB_FEAT_INCOMPAT_RICHACL 0 >> +#endif >> + > > Regardless of whether we build in richacl support, this is wrong. > Feature bits are on-disk format definitions, so it will always have > this value whether or not the kernel supports the feature. This is so that xfs_mount_validate_sb() will complain when mounting a richacl filesystem on a kernel which doesn't have richacl suport compiled in. The same effect can be had with extra code there of course. >> @@ -722,7 +737,10 @@ xfs_setattr_nonsize( >> * Posix ACL code seems to care about this issue either. >> */ >> if ((mask & ATTR_MODE) && !(flags & XFS_ATTR_NOACL)) { >> - error = posix_acl_chmod(inode, inode->i_mode); >> + if (XFS_IS_RICHACL(inode)) >> + error = richacl_chmod(inode, inode->i_mode); >> + else >> + error = posix_acl_chmod(inode, inode->i_mode); >> if (error) >> return error; >> } > > This sort of thing could do with a helper like: > > static inline int > xfs_acl_chmod(struct inode *inode, int mode) > { > if (IS_RICHACL(inode)) > return richacl_chmod(inode, mode); > return posix_acl_chmod(inode, mode); > } Alright. >> +#include "xfs.h" >> +#include "xfs_format.h" >> +#include "xfs_log_format.h" >> +#include "xfs_inode.h" >> +#include "xfs_attr.h" >> + >> +#include <linux/richacl_xattr.h> >> + >> +struct richacl * >> +xfs_get_richacl(struct inode *inode) >> +{ > > ... > > If we always build in ACL support, this can all go in xfs_acl.c. > >> + struct xfs_inode *ip = XFS_I(inode); >> + struct richacl *acl; >> + int size = XATTR_SIZE_MAX; >> + void *value; >> + int error; >> + >> + value = kmem_zalloc_large(size, KM_SLEEP); >> + if (!value) >> + return ERR_PTR(-ENOMEM); >> + >> + error = xfs_attr_get(ip, XATTR_NAME_RICHACL, value, &size, ATTR_ROOT); >> + if (error) { >> + /* >> + * If the attribute doesn't exist make sure we have a negative >> + * cache entry, for any other error assume it is transient and >> + * leave the cache entry as ACL_NOT_CACHED. >> + */ >> + acl = (error == -ENOATTR) ? NULL : ERR_PTR(error); > > I rather dislike ternaries in code like this - it's hard to read the > logic. Initalise acl to NULL, and then this simply becomes: > > if (error != -ENOATTR) > acl = ERR_PTR(error); As you prefer. >> + } else >> + acl = richacl_from_xattr(&init_user_ns, value, size); >> + >> + if (!IS_ERR(acl)) >> + set_cached_richacl(inode, acl); >> + kfree(value); >> + >> + return acl; >> +} >> + >> +int >> +xfs_set_richacl(struct inode *inode, struct richacl *acl) >> +{ >> + struct xfs_inode *ip = XFS_I(inode); >> + umode_t mode = inode->i_mode; >> + int error; >> + >> + if (acl) { >> + /* Don't allow acls with unmapped identifiers. */ >> + if (richacl_has_unmapped_identifiers(acl)) >> + return -EINVAL; >> + >> + if (richacl_equiv_mode(acl, &mode) == 0) { >> + xfs_set_mode(inode, mode); >> + acl = NULL; >> + } > > Comment needed here - why does this now seem to accidentally fall > through to removing the ACL? Setting an ACL that is equivalent to a file mode is the same as removing any existing ACLs and doing a chmod to that equivalent file mode. It's the sams as with POSIX ACLs, and the code is the same as well. I'll add a comment though. > Also, why is this in the XFS specific > code when it's generic richacl functionality that is being checked? This turns into two non-atomic operations if we move it into generic code. > Also, I really dislike the API where passing a NULL acl means to > "set this acl" actually means "remove the existing ACL". Why no > ->remove_acl method called from the generic code? It's not uncommon, it saves inode operations and wiring-up code. >> +#ifndef __FS_XFS_RICHACL_H >> +#define __FS_XFS_RICHACL_H >> + >> +#include <linux/richacl.h> >> + >> +#ifdef CONFIG_XFS_RICHACL >> + >> +#define XFS_IS_RICHACL(inode) IS_RICHACL(inode) > > No. Just use IS_RICHACL() everywhere, and whatever build > conditional that uses. Okay. >> --- a/fs/xfs/xfs_super.c >> +++ b/fs/xfs/xfs_super.c >> @@ -1500,7 +1500,19 @@ xfs_fs_fill_super( >> sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits); >> sb->s_max_links = XFS_MAXLINK; >> sb->s_time_gran = 1; >> - set_posix_acl_flag(sb); >> + >> + if (xfs_sb_version_hasrichacl(&mp->m_sb)) { >> +#ifdef CONFIG_XFS_RICHACL >> + sb->s_flags |= MS_RICHACL; >> +#else >> + error = -EOPNOTSUPP; >> + goto out_free_sb; >> +#endif >> + } else { >> +#ifdef CONFIG_XFS_POSIX_ACL >> + sb->s_flags |= MS_POSIXACL; >> +#endif > > The whole point of having set_posix_acl_flag() is that it keeps > the ifdef hackery out of the code. If we are going to keep build > time config options for these, then please keep the wrappers to hide > this mess... That can indeed go away since xfs_mount_validate_sb() checks the mount flags already. Thanks, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 12, 2015 at 03:51:15AM +0200, Andreas Grünbacher wrote: > 2015-10-12 2:10 GMT+02:00 Dave Chinner <david@fromorbit.com>: > > On Mon, Oct 12, 2015 at 12:58:35AM +0200, Andreas Gruenbacher wrote: > >> diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig > >> index 5d47b4d..05dd312 100644 > >> --- a/fs/xfs/Kconfig > >> +++ b/fs/xfs/Kconfig > >> @@ -52,6 +52,17 @@ config XFS_POSIX_ACL > >> > >> If you don't know what Access Control Lists are, say N. > >> > >> +config XFS_RICHACL > >> + bool "XFS Rich Access Control Lists (EXPERIMENTAL)" > >> + depends on XFS_FS > >> + select FS_RICHACL > >> + help > >> + Richacls are an implementation of NFSv4 ACLs, extended by file masks > >> + to cleanly integrate into the POSIX file permission model. To learn > >> + more about them, see http://www.bestbits.at/richacl/. > >> + > >> + If you don't know what Richacls are, say N. > >> + > > > > So now we have multiple compile time ACL configs that we have to > > test. Personally, I see no reason for making either posix or rich > > acls compile time dependent. How about we just change CONFIG_FS_XFS > > to have "select FS_POSIX_ACL" and "select FS_RICHACL" > > unconditionally, and then we can get rid of all the conditional > > code? > > I'm sure neither kind of ACLs makes sense on many tiny systems, it > should be possible to disable them. XFS may not make sense on those > kinds of systems either, that is not my call to make though. Well, the smallest systems that use XFS are typically 1-2 disk NAS boxes, and I can see them wanting richacls. As it is, the xfs kernel module is almost 1MB of object code, so it you have a small embedded box you don't want XFS. Almost all distros turn on ACL support I'm not sure that it makes sense to have these config options in XFS anymore.... > >> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > >> index 9590a06..8c6da45 100644 > >> --- a/fs/xfs/libxfs/xfs_format.h > >> +++ b/fs/xfs/libxfs/xfs_format.h > >> @@ -461,10 +461,18 @@ xfs_sb_has_ro_compat_feature( > >> #define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */ > >> #define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */ > >> #define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */ > >> + > >> +#ifdef CONFIG_XFS_RICHACL > >> +#define XFS_SB_FEAT_INCOMPAT_RICHACL (1 << 3) /* richacls */ > >> +#else > >> +#define XFS_SB_FEAT_INCOMPAT_RICHACL 0 > >> +#endif > >> + > > > > Regardless of whether we build in richacl support, this is wrong. > > Feature bits are on-disk format definitions, so it will always have > > this value whether or not the kernel supports the feature. > > This is so that xfs_mount_validate_sb() will complain when mounting a > richacl filesystem on a kernel which doesn't have richacl suport > compiled in. The same effect can be had with extra code there of > course. If the kernel doesn't know about a feature, then it will report "unknown feature". However, as of this patch set, the kernel will know about the richacl feature, and so the correct error message is to say "Rich ACLs not supported by this kernel". Also, from a disk format perspective, this is a this is a read-only compat feature, as kernels that don't have richacl support are still able to mount and walk the filesystem without errors occurring. It's only when allowing modifications are made that problems will arise, so why did you make it an incompat feature? > >> +int > >> +xfs_set_richacl(struct inode *inode, struct richacl *acl) > >> +{ > >> + struct xfs_inode *ip = XFS_I(inode); > >> + umode_t mode = inode->i_mode; > >> + int error; > >> + > >> + if (acl) { > >> + /* Don't allow acls with unmapped identifiers. */ > >> + if (richacl_has_unmapped_identifiers(acl)) > >> + return -EINVAL; > >> + > >> + if (richacl_equiv_mode(acl, &mode) == 0) { > >> + xfs_set_mode(inode, mode); > >> + acl = NULL; > >> + } > > > > Comment needed here - why does this now seem to accidentally fall > > through to removing the ACL? > > Setting an ACL that is equivalent to a file mode is the same as > removing any existing ACLs and doing a chmod to that equivalent file > mode. It's the sams as with POSIX ACLs, and the code is the same as > well. I'll add a comment though. > > > Also, why is this in the XFS specific > > code when it's generic richacl functionality that is being checked? > > This turns into two non-atomic operations if we move it into generic code. I don't follow you - this isn't an atomic operation to begin with... > > Also, I really dislike the API where passing a NULL acl means to > > "set this acl" actually means "remove the existing ACL". Why no > > ->remove_acl method called from the generic code? > > It's not uncommon, it saves inode operations and wiring-up code. I know it's common. All it does is put extra branches in the filesystem code to do this, because remove is a different operation to set. The API sucks, and we're not limited on inode operations, and the operator overloading makes the filesystem code unnecessarily complex as it has to detect when to branch out ot remove or not... Cheers, Dave.
On Mon, Oct 12, 2015 at 6:05 AM, Dave Chinner <david@fromorbit.com> wrote: > On Mon, Oct 12, 2015 at 03:51:15AM +0200, Andreas Grünbacher wrote: >> 2015-10-12 2:10 GMT+02:00 Dave Chinner <david@fromorbit.com>: >> > On Mon, Oct 12, 2015 at 12:58:35AM +0200, Andreas Gruenbacher wrote: >> >> diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig >> >> index 5d47b4d..05dd312 100644 >> >> --- a/fs/xfs/Kconfig >> >> +++ b/fs/xfs/Kconfig >> >> @@ -52,6 +52,17 @@ config XFS_POSIX_ACL >> >> >> >> If you don't know what Access Control Lists are, say N. >> >> >> >> +config XFS_RICHACL >> >> + bool "XFS Rich Access Control Lists (EXPERIMENTAL)" >> >> + depends on XFS_FS >> >> + select FS_RICHACL >> >> + help >> >> + Richacls are an implementation of NFSv4 ACLs, extended by file masks >> >> + to cleanly integrate into the POSIX file permission model. To learn >> >> + more about them, see http://www.bestbits.at/richacl/. >> >> + >> >> + If you don't know what Richacls are, say N. >> >> + >> > >> > So now we have multiple compile time ACL configs that we have to >> > test. Personally, I see no reason for making either posix or rich >> > acls compile time dependent. How about we just change CONFIG_FS_XFS >> > to have "select FS_POSIX_ACL" and "select FS_RICHACL" >> > unconditionally, and then we can get rid of all the conditional >> > code? >> >> I'm sure neither kind of ACLs makes sense on many tiny systems, it >> should be possible to disable them. XFS may not make sense on those >> kinds of systems either, that is not my call to make though. > > Well, the smallest systems that use XFS are typically 1-2 disk NAS > boxes, and I can see them wanting richacls. As it is, the xfs kernel > module is almost 1MB of object code, so it you have a small embedded > box you don't want XFS. Almost all distros turn on ACL support I'm > not sure that it makes sense to have these config options in XFS > anymore.... You have me convinced about removing CONFIG_XFS_RICHACL. >> >> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h >> >> index 9590a06..8c6da45 100644 >> >> --- a/fs/xfs/libxfs/xfs_format.h >> >> +++ b/fs/xfs/libxfs/xfs_format.h >> >> @@ -461,10 +461,18 @@ xfs_sb_has_ro_compat_feature( >> >> #define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */ >> >> #define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */ >> >> #define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */ >> >> + >> >> +#ifdef CONFIG_XFS_RICHACL >> >> +#define XFS_SB_FEAT_INCOMPAT_RICHACL (1 << 3) /* richacls */ >> >> +#else >> >> +#define XFS_SB_FEAT_INCOMPAT_RICHACL 0 >> >> +#endif >> >> + >> > >> > Regardless of whether we build in richacl support, this is wrong. >> > Feature bits are on-disk format definitions, so it will always have >> > this value whether or not the kernel supports the feature. >> >> This is so that xfs_mount_validate_sb() will complain when mounting a >> richacl filesystem on a kernel which doesn't have richacl suport >> compiled in. The same effect can be had with extra code there of >> course. > > If the kernel doesn't know about a feature, then it will report > "unknown feature". However, as of this patch set, the kernel will > know about the richacl feature, and so the correct error message > is to say "Rich ACLs not supported by this kernel". > > Also, from a disk format perspective, this is a this is a read-only > compat feature, as kernels that don't have richacl support are still > able to mount and walk the filesystem without errors occurring. It's > only when allowing modifications are made that problems will arise, > so why did you make it an incompat feature? As a read-only compat flag, kernels that cannot enforce richacls would still be able to mount richacl file systems read-only. That's a problem when acls are used to forbid read/execute access. >> >> +int >> >> +xfs_set_richacl(struct inode *inode, struct richacl *acl) >> >> +{ >> >> + struct xfs_inode *ip = XFS_I(inode); >> >> + umode_t mode = inode->i_mode; >> >> + int error; >> >> + >> >> + if (acl) { >> >> + /* Don't allow acls with unmapped identifiers. */ >> >> + if (richacl_has_unmapped_identifiers(acl)) >> >> + return -EINVAL; >> >> + >> >> + if (richacl_equiv_mode(acl, &mode) == 0) { >> >> + xfs_set_mode(inode, mode); >> >> + acl = NULL; >> >> + } >> > >> > Comment needed here - why does this now seem to accidentally fall >> > through to removing the ACL? >> >> Setting an ACL that is equivalent to a file mode is the same as >> removing any existing ACLs and doing a chmod to that equivalent file >> mode. It's the sams as with POSIX ACLs, and the code is the same as >> well. I'll add a comment though. >> >> > Also, why is this in the XFS specific >> > code when it's generic richacl functionality that is being checked? >> >> This turns into two non-atomic operations if we move it into generic code. > > I don't follow you - this isn't an atomic operation to begin with... Right, I shouldn't have used the term atomic. It's one inode operation under i_mutex, and the filesystem can pack it into one transaction. Thanks, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 12, 2015 at 6:05 AM, Dave Chinner <david@fromorbit.com> wrote: > On Mon, Oct 12, 2015 at 03:51:15AM +0200, Andreas Grünbacher wrote: >> 2015-10-12 2:10 GMT+02:00 Dave Chinner <david@fromorbit.com>: >> > Also, I really dislike the API where passing a NULL acl means to >> > "set this acl" actually means "remove the existing ACL". Why no >> > ->remove_acl method called from the generic code? >> >> It's not uncommon, it saves inode operations and wiring-up code. > > I know it's common. All it does is put extra branches in the > filesystem code to do this, because remove is a different operation > to set. The API sucks, and we're not limited on inode operations, > and the operator overloading makes the filesystem code unnecessarily > complex as it has to detect when to branch out ot remove or not... I've tried it out. The filesystem code could be simplified (see the richacl-wip [*] branch until the next posting). Adding a remove_richacl inode operation on top of that really doesn't help. Thanks, Andreas [*] git://git.kernel.org/pub/scm/linux/kernel/git/agruen/linux-richacl.git richacl-wip -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015-10-12 01:57, Andreas Gruenbacher wrote: > On Mon, Oct 12, 2015 at 6:05 AM, Dave Chinner <david@fromorbit.com> wrote: >> On Mon, Oct 12, 2015 at 03:51:15AM +0200, Andreas Grünbacher wrote: >>> 2015-10-12 2:10 GMT+02:00 Dave Chinner <david@fromorbit.com>: >>>> On Mon, Oct 12, 2015 at 12:58:35AM +0200, Andreas Gruenbacher wrote: >>>>> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h >>>>> index 9590a06..8c6da45 100644 >>>>> --- a/fs/xfs/libxfs/xfs_format.h >>>>> +++ b/fs/xfs/libxfs/xfs_format.h >>>>> @@ -461,10 +461,18 @@ xfs_sb_has_ro_compat_feature( >>>>> #define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */ >>>>> #define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */ >>>>> #define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */ >>>>> + >>>>> +#ifdef CONFIG_XFS_RICHACL >>>>> +#define XFS_SB_FEAT_INCOMPAT_RICHACL (1 << 3) /* richacls */ >>>>> +#else >>>>> +#define XFS_SB_FEAT_INCOMPAT_RICHACL 0 >>>>> +#endif >>>>> + >>>> >>>> Regardless of whether we build in richacl support, this is wrong. >>>> Feature bits are on-disk format definitions, so it will always have >>>> this value whether or not the kernel supports the feature. >>> >>> This is so that xfs_mount_validate_sb() will complain when mounting a >>> richacl filesystem on a kernel which doesn't have richacl suport >>> compiled in. The same effect can be had with extra code there of >>> course. >> >> If the kernel doesn't know about a feature, then it will report >> "unknown feature". However, as of this patch set, the kernel will >> know about the richacl feature, and so the correct error message >> is to say "Rich ACLs not supported by this kernel". >> >> Also, from a disk format perspective, this is a this is a read-only >> compat feature, as kernels that don't have richacl support are still >> able to mount and walk the filesystem without errors occurring. It's >> only when allowing modifications are made that problems will arise, >> so why did you make it an incompat feature? > > As a read-only compat flag, kernels that cannot enforce richacls would > still be able to mount richacl file systems read-only. That's a > problem when acls are used to forbid read/execute access. It's also an irrelevant problem, anyone with a minimal knowledge of the filesystem's on-disk layout can unset the feature bit by hand and force it to be mounted anyway, thus bypassing the ACL's (this is the case for any filesystem, not just XFS). If someone has access to the hardware, they have access to the data stored on it, period, irrespective of what the data says about how it should be accessed. The 3 most common cases for trying to mount a filesystem with this on a kernel that doesn't support it are: a. Someone is trying to recover data on their own system using something like SystemRescueCD. b. Someone is trying to recover data from a non-functional system that they own or have been authorized to access for this purpose by connecting the disk to another system. c. Someone is trying to bisect a kernel bug or track down what config option is causing them issues. All three of these cases _need_ to keep working properly without needing to manually twiddle with compat bits, otherwise it _will_ cause a lot of people to advocate not using richacls.
diff --git a/fs/ext4/richacl.h b/fs/ext4/richacl.h index fc7826f..f26fbdd 100644 --- a/fs/ext4/richacl.h +++ b/fs/ext4/richacl.h @@ -24,7 +24,6 @@ extern struct richacl *ext4_get_richacl(struct inode *); extern int ext4_set_richacl(struct inode *, struct richacl *); - extern int ext4_init_richacl(handle_t *, struct inode *, struct inode *); #else /* CONFIG_EXT4_FS_RICHACL */ diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig index 5d47b4d..05dd312 100644 --- a/fs/xfs/Kconfig +++ b/fs/xfs/Kconfig @@ -52,6 +52,17 @@ config XFS_POSIX_ACL If you don't know what Access Control Lists are, say N. +config XFS_RICHACL + bool "XFS Rich Access Control Lists (EXPERIMENTAL)" + depends on XFS_FS + select FS_RICHACL + help + Richacls are an implementation of NFSv4 ACLs, extended by file masks + to cleanly integrate into the POSIX file permission model. To learn + more about them, see http://www.bestbits.at/richacl/. + + If you don't know what Richacls are, say N. + config XFS_RT bool "XFS Realtime subvolume support" depends on XFS_FS diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile index a096841..7df9ae3 100644 --- a/fs/xfs/Makefile +++ b/fs/xfs/Makefile @@ -118,6 +118,7 @@ xfs-$(CONFIG_XFS_QUOTA) += xfs_dquot.o \ xfs-$(CONFIG_XFS_RT) += xfs_rtalloc.o xfs-$(CONFIG_XFS_POSIX_ACL) += xfs_acl.o +xfs-$(CONFIG_XFS_RICHACL) += xfs_richacl.o xfs-$(CONFIG_PROC_FS) += xfs_stats.o xfs-$(CONFIG_SYSCTL) += xfs_sysctl.o xfs-$(CONFIG_COMPAT) += xfs_ioctl32.o diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index 9590a06..8c6da45 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -461,10 +461,18 @@ xfs_sb_has_ro_compat_feature( #define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */ #define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */ #define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */ + +#ifdef CONFIG_XFS_RICHACL +#define XFS_SB_FEAT_INCOMPAT_RICHACL (1 << 3) /* richacls */ +#else +#define XFS_SB_FEAT_INCOMPAT_RICHACL 0 +#endif + #define XFS_SB_FEAT_INCOMPAT_ALL \ (XFS_SB_FEAT_INCOMPAT_FTYPE| \ XFS_SB_FEAT_INCOMPAT_SPINODES| \ - XFS_SB_FEAT_INCOMPAT_META_UUID) + XFS_SB_FEAT_INCOMPAT_META_UUID| \ + XFS_SB_FEAT_INCOMPAT_RICHACL) #define XFS_SB_FEAT_INCOMPAT_UNKNOWN ~XFS_SB_FEAT_INCOMPAT_ALL static inline bool @@ -530,6 +538,12 @@ static inline bool xfs_sb_version_hasmetauuid(struct xfs_sb *sbp) (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_META_UUID); } +static inline bool xfs_sb_version_hasrichacl(struct xfs_sb *sbp) +{ + return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) && + (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_RICHACL); +} + /* * end of superblock version macros */ diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 8294132..e37bdf87 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -27,6 +27,7 @@ #include "xfs_bmap.h" #include "xfs_bmap_util.h" #include "xfs_acl.h" +#include "xfs_richacl.h" #include "xfs_quota.h" #include "xfs_error.h" #include "xfs_attr.h" @@ -42,6 +43,7 @@ #include <linux/capability.h> #include <linux/xattr.h> #include <linux/posix_acl.h> +#include <linux/richacl.h> #include <linux/security.h> #include <linux/fiemap.h> #include <linux/slab.h> @@ -133,7 +135,8 @@ xfs_generic_create( { struct inode *inode; struct xfs_inode *ip = NULL; - struct posix_acl *default_acl, *acl; + struct posix_acl *default_acl = NULL, *acl = NULL; + struct richacl *richacl = NULL; struct xfs_name name; int error; @@ -149,9 +152,15 @@ xfs_generic_create( rdev = 0; } - error = posix_acl_create(dir, &mode, &default_acl, &acl); - if (error) - return error; + if (XFS_IS_RICHACL(dir)) { + richacl = richacl_create(&mode, dir); + if (IS_ERR(richacl)) + return PTR_ERR(richacl); + } else { + error = posix_acl_create(dir, &mode, &default_acl, &acl); + if (error) + return error; + } if (!tmpfile) { xfs_dentry_to_name(&name, dentry, mode); @@ -180,6 +189,13 @@ xfs_generic_create( goto out_cleanup_inode; } #endif +#ifdef CONFIG_XFS_RICHACL + if (richacl) { + error = xfs_set_richacl(inode, richacl); + if (error) + goto out_cleanup_inode; + } +#endif if (tmpfile) d_tmpfile(dentry, inode); @@ -189,10 +205,9 @@ xfs_generic_create( xfs_finish_inode_setup(ip); out_free_acl: - if (default_acl) - posix_acl_release(default_acl); - if (acl) - posix_acl_release(acl); + posix_acl_release(default_acl); + posix_acl_release(acl); + richacl_put(richacl); return error; out_cleanup_inode: @@ -722,7 +737,10 @@ xfs_setattr_nonsize( * Posix ACL code seems to care about this issue either. */ if ((mask & ATTR_MODE) && !(flags & XFS_ATTR_NOACL)) { - error = posix_acl_chmod(inode, inode->i_mode); + if (XFS_IS_RICHACL(inode)) + error = richacl_chmod(inode, inode->i_mode); + else + error = posix_acl_chmod(inode, inode->i_mode); if (error) return error; } @@ -1104,6 +1122,8 @@ xfs_vn_tmpfile( static const struct inode_operations xfs_inode_operations = { .get_acl = xfs_get_acl, .set_acl = xfs_set_acl, + .get_richacl = xfs_get_richacl, + .set_richacl = xfs_set_richacl, .getattr = xfs_vn_getattr, .setattr = xfs_vn_setattr, .setxattr = generic_setxattr, @@ -1132,6 +1152,8 @@ static const struct inode_operations xfs_dir_inode_operations = { .rename2 = xfs_vn_rename, .get_acl = xfs_get_acl, .set_acl = xfs_set_acl, + .get_richacl = xfs_get_richacl, + .set_richacl = xfs_set_richacl, .getattr = xfs_vn_getattr, .setattr = xfs_vn_setattr, .setxattr = generic_setxattr, @@ -1160,6 +1182,8 @@ static const struct inode_operations xfs_dir_ci_inode_operations = { .rename2 = xfs_vn_rename, .get_acl = xfs_get_acl, .set_acl = xfs_set_acl, + .get_richacl = xfs_get_richacl, + .set_richacl = xfs_set_richacl, .getattr = xfs_vn_getattr, .setattr = xfs_vn_setattr, .setxattr = generic_setxattr, diff --git a/fs/xfs/xfs_richacl.c b/fs/xfs/xfs_richacl.c new file mode 100644 index 0000000..af74e92 --- /dev/null +++ b/fs/xfs/xfs_richacl.c @@ -0,0 +1,97 @@ +/* + * Copyright (C) 2015 Red Hat, Inc. + * Author: Andreas Gruenbacher <agruenba@redhat.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of version 2.1 of the GNU Lesser 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. + * + */ + +#include "xfs.h" +#include "xfs_format.h" +#include "xfs_log_format.h" +#include "xfs_inode.h" +#include "xfs_attr.h" + +#include <linux/richacl_xattr.h> + +struct richacl * +xfs_get_richacl(struct inode *inode) +{ + struct xfs_inode *ip = XFS_I(inode); + struct richacl *acl; + int size = XATTR_SIZE_MAX; + void *value; + int error; + + value = kmem_zalloc_large(size, KM_SLEEP); + if (!value) + return ERR_PTR(-ENOMEM); + + error = xfs_attr_get(ip, XATTR_NAME_RICHACL, value, &size, ATTR_ROOT); + if (error) { + /* + * If the attribute doesn't exist make sure we have a negative + * cache entry, for any other error assume it is transient and + * leave the cache entry as ACL_NOT_CACHED. + */ + acl = (error == -ENOATTR) ? NULL : ERR_PTR(error); + } else + acl = richacl_from_xattr(&init_user_ns, value, size); + + if (!IS_ERR(acl)) + set_cached_richacl(inode, acl); + kfree(value); + + return acl; +} + +int +xfs_set_richacl(struct inode *inode, struct richacl *acl) +{ + struct xfs_inode *ip = XFS_I(inode); + umode_t mode = inode->i_mode; + int error; + + if (acl) { + /* Don't allow acls with unmapped identifiers. */ + if (richacl_has_unmapped_identifiers(acl)) + return -EINVAL; + + if (richacl_equiv_mode(acl, &mode) == 0) { + xfs_set_mode(inode, mode); + acl = NULL; + } + } + if (acl) { + void *value; + int size = richacl_xattr_size(acl); + + value = kmem_zalloc_large(size, KM_SLEEP); + if (!value) + return -ENOMEM; + richacl_to_xattr(&init_user_ns, acl, value, size); + error = xfs_attr_set(ip, XATTR_NAME_RICHACL, value, size, + ATTR_ROOT); + kfree(value); + + if (!error) { + mode &= ~S_IRWXUGO; + mode |= richacl_masks_to_mode(acl); + xfs_set_mode(inode, mode); + } + } else { + error = xfs_attr_remove(ip, XATTR_NAME_RICHACL, ATTR_ROOT); + if (error == -ENOATTR) + error = 0; + } + if (!error) + set_cached_richacl(inode, acl); + + return 0; +} diff --git a/fs/xfs/xfs_richacl.h b/fs/xfs/xfs_richacl.h new file mode 100644 index 0000000..78ad314 --- /dev/null +++ b/fs/xfs/xfs_richacl.h @@ -0,0 +1,34 @@ +/* + * Copyright (C) 2015 Red Hat, Inc. + * Author: Andreas Gruenbacher <agruenba@redhat.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of version 2.1 of the GNU Lesser 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. + * + */ + +#ifndef __FS_XFS_RICHACL_H +#define __FS_XFS_RICHACL_H + +#include <linux/richacl.h> + +#ifdef CONFIG_XFS_RICHACL + +#define XFS_IS_RICHACL(inode) IS_RICHACL(inode) + +extern struct richacl *xfs_get_richacl(struct inode *); +extern int xfs_set_richacl(struct inode *, struct richacl *); + +#else /* CONFIG_XFS_RICHACL */ + +#define XFS_IS_RICHACL(inode) (0) +#define xfs_get_richacl NULL +#define xfs_set_richacl NULL + +#endif /* CONFIG_XFS_RICHACL */ +#endif /* __FS_XFS_RICHACL_H */ diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 904f637..74eeb0e 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1500,7 +1500,19 @@ xfs_fs_fill_super( sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits); sb->s_max_links = XFS_MAXLINK; sb->s_time_gran = 1; - set_posix_acl_flag(sb); + + if (xfs_sb_version_hasrichacl(&mp->m_sb)) { +#ifdef CONFIG_XFS_RICHACL + sb->s_flags |= MS_RICHACL; +#else + error = -EOPNOTSUPP; + goto out_free_sb; +#endif + } else { +#ifdef CONFIG_XFS_POSIX_ACL + sb->s_flags |= MS_POSIXACL; +#endif + } /* version 5 superblocks support inode version counters. */ if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5) diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h index 499058f..072fd57 100644 --- a/fs/xfs/xfs_super.h +++ b/fs/xfs/xfs_super.h @@ -30,10 +30,14 @@ extern void xfs_qm_exit(void); #ifdef CONFIG_XFS_POSIX_ACL # define XFS_ACL_STRING "ACLs, " -# define set_posix_acl_flag(sb) ((sb)->s_flags |= MS_POSIXACL) #else # define XFS_ACL_STRING -# define set_posix_acl_flag(sb) do { } while (0) +#endif + +#ifdef CONFIG_XFS_RICHACL +# define XFS_RICHACL_STRING "Richacls, " +#else +# define XFS_RICHACL_STRING #endif #define XFS_SECURITY_STRING "security attributes, " @@ -52,6 +56,7 @@ extern void xfs_qm_exit(void); #define XFS_VERSION_STRING "SGI XFS" #define XFS_BUILD_OPTIONS XFS_ACL_STRING \ + XFS_RICHACL_STRING \ XFS_SECURITY_STRING \ XFS_REALTIME_STRING \ XFS_DBG_STRING /* DBG must be last */ diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c index c0368151..238b39f 100644 --- a/fs/xfs/xfs_xattr.c +++ b/fs/xfs/xfs_xattr.c @@ -28,6 +28,7 @@ #include "xfs_acl.h" #include <linux/posix_acl_xattr.h> +#include <linux/richacl_xattr.h> #include <linux/xattr.h> @@ -103,6 +104,9 @@ const struct xattr_handler *xfs_xattr_handlers[] = { &posix_acl_access_xattr_handler, &posix_acl_default_xattr_handler, #endif +#ifdef CONFIG_XFS_RICHACL + &richacl_xattr_handler, +#endif NULL };