lkml.org 
[lkml]   [2015]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v13 21/51] ext4: Add richacl feature flag
    From
    Date

    > On Nov 3, 2015, at 8:16 AM, Andreas Gruenbacher <agruenba@redhat.com> wrote:
    >
    > From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
    >
    > This feature flag selects richacl instead of posix acl support on the
    > file system. In addition, the "acl" mount option is needed for enabling
    > either of the two kinds of acls.

    This patch confuses me. I thought the whole point of INCOMPAT_RICHACL
    was that the filesystem should never, ever be mounted without ACL support
    because the ACLs will get confused without it. In that case, it doesn't
    make sense to have a mount option that _has_ to be specified to mount the
    filesystem, and returns an error when trying to disable it.

    It makes more sense to just enable "acl" by default if INCOMPAT_RICHACL
    is set in the superblock and not need the mount option at all.

    Having a mount option made more sense when enabling this support was optional
    for the filesystem, and it could be mounted without it enabled, but you
    wanted INCOMPAT_RICHACL to prevent that so it needs to be fixed.

    Cheers, Andreas

    > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
    > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
    > ---
    > fs/ext4/ext4.h | 6 ++++--
    > fs/ext4/super.c | 49 ++++++++++++++++++++++++++++++++++++++++---------
    > 2 files changed, 44 insertions(+), 11 deletions(-)
    >
    > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
    > index fd1f28b..b97a3b1 100644
    > --- a/fs/ext4/ext4.h
    > +++ b/fs/ext4/ext4.h
    > @@ -991,7 +991,7 @@ struct ext4_inode_info {
    > #define EXT4_MOUNT_UPDATE_JOURNAL 0x01000 /* Update the journal format */
    > #define EXT4_MOUNT_NO_UID32 0x02000 /* Disable 32-bit UIDs */
    > #define EXT4_MOUNT_XATTR_USER 0x04000 /* Extended user attributes */
    > -#define EXT4_MOUNT_POSIX_ACL 0x08000 /* POSIX Access Control Lists */
    > +#define EXT4_MOUNT_ACL 0x08000 /* Access Control Lists */
    > #define EXT4_MOUNT_NO_AUTO_DA_ALLOC 0x10000 /* No auto delalloc mapping */
    > #define EXT4_MOUNT_BARRIER 0x20000 /* Use block barriers */
    > #define EXT4_MOUNT_QUOTA 0x80000 /* Some quota option set */
    > @@ -1582,6 +1582,7 @@ static inline int ext4_encrypted_inode(struct inode *inode)
    > #define EXT4_FEATURE_INCOMPAT_LARGEDIR 0x4000 /* >2GB or 3-lvl htree */
    > #define EXT4_FEATURE_INCOMPAT_INLINE_DATA 0x8000 /* data in inode */
    > #define EXT4_FEATURE_INCOMPAT_ENCRYPT 0x10000
    > +#define EXT4_FEATURE_INCOMPAT_RICHACL 0x20000
    >
    > #define EXT2_FEATURE_COMPAT_SUPP EXT4_FEATURE_COMPAT_EXT_ATTR
    > #define EXT2_FEATURE_INCOMPAT_SUPP (EXT4_FEATURE_INCOMPAT_FILETYPE| \
    > @@ -1607,7 +1608,8 @@ static inline int ext4_encrypted_inode(struct inode *inode)
    > EXT4_FEATURE_INCOMPAT_FLEX_BG| \
    > EXT4_FEATURE_INCOMPAT_MMP | \
    > EXT4_FEATURE_INCOMPAT_INLINE_DATA | \
    > - EXT4_FEATURE_INCOMPAT_ENCRYPT)
    > + EXT4_FEATURE_INCOMPAT_ENCRYPT | \
    > + EXT4_FEATURE_INCOMPAT_RICHACL)
    > #define EXT4_FEATURE_RO_COMPAT_SUPP (EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER| \
    > EXT4_FEATURE_RO_COMPAT_LARGE_FILE| \
    > EXT4_FEATURE_RO_COMPAT_GDT_CSUM| \
    > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
    > index a63c7b0..7457ea8 100644
    > --- a/fs/ext4/super.c
    > +++ b/fs/ext4/super.c
    > @@ -1270,6 +1270,28 @@ static ext4_fsblk_t get_sb_block(void **data)
    > return sb_block;
    > }
    >
    > +static int enable_acl(struct super_block *sb)
    > +{
    > + sb->s_flags &= ~(MS_POSIXACL | MS_RICHACL);
    > + if (test_opt(sb, ACL)) {
    > + if (EXT4_HAS_INCOMPAT_FEATURE(sb,
    > + EXT4_FEATURE_INCOMPAT_RICHACL)) {
    > +#ifdef CONFIG_EXT4_FS_RICHACL
    > + sb->s_flags |= MS_RICHACL;
    > +#else
    > + return -EOPNOTSUPP;
    > +#endif
    > + } else {
    > +#ifdef CONFIG_EXT4_FS_POSIX_ACL
    > + sb->s_flags |= MS_POSIXACL;
    > +#else
    > + return -EOPNOTSUPP;
    > +#endif
    > + }
    > + }
    > + return 0;
    > +}
    > +
    > #define DEFAULT_JOURNAL_IOPRIO (IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 3))
    > static char deprecated_msg[] = "Mount option \"%s\" will be removed by %s\n"
    > "Contact linux-ext4@vger.kernel.org if you think we should keep it.\n";
    > @@ -1416,9 +1438,9 @@ static const struct mount_opts {
    > MOPT_NO_EXT2 | MOPT_DATAJ},
    > {Opt_user_xattr, EXT4_MOUNT_XATTR_USER, MOPT_SET},
    > {Opt_nouser_xattr, EXT4_MOUNT_XATTR_USER, MOPT_CLEAR},
    > -#ifdef CONFIG_EXT4_FS_POSIX_ACL
    > - {Opt_acl, EXT4_MOUNT_POSIX_ACL, MOPT_SET},
    > - {Opt_noacl, EXT4_MOUNT_POSIX_ACL, MOPT_CLEAR},
    > +#if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
    > + {Opt_acl, EXT4_MOUNT_ACL, MOPT_SET},
    > + {Opt_noacl, EXT4_MOUNT_ACL, MOPT_CLEAR},
    > #else
    > {Opt_acl, 0, MOPT_NOSUPPORT},
    > {Opt_noacl, 0, MOPT_NOSUPPORT},
    > @@ -1466,6 +1488,13 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
    > #endif
    > switch (token) {
    > case Opt_noacl:
    > +#ifdef CONFIG_EXT4_FS_RICHACL
    > + if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RICHACL)) {
    > + ext4_msg(sb, KERN_ERR, "Mount option \"%s\" incompatible "
    > + "with richacl feature", opt);
    > + return -1;
    > + }
    > +#endif
    > case Opt_nouser_xattr:
    > ext4_msg(sb, KERN_WARNING, deprecated_msg, opt, "3.5");
    > break;
    > @@ -3576,8 +3605,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
    > set_opt(sb, NO_UID32);
    > /* xattr user namespace & acls are now defaulted on */
    > set_opt(sb, XATTR_USER);
    > -#ifdef CONFIG_EXT4_FS_POSIX_ACL
    > - set_opt(sb, POSIX_ACL);
    > +#if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
    > + set_opt(sb, ACL);
    > #endif
    > /* don't forget to enable journal_csum when metadata_csum is enabled. */
    > if (ext4_has_metadata_csum(sb))
    > @@ -3660,8 +3689,9 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
    > sb->s_iflags |= SB_I_CGROUPWB;
    > }
    >
    > - sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
    > - (test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
    > + err = enable_acl(sb);
    > + if (err)
    > + goto failed_mount;
    >
    > if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
    > (EXT4_HAS_COMPAT_FEATURE(sb, ~0U) ||
    > @@ -4981,8 +5011,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
    > if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED)
    > ext4_abort(sb, "Abort forced by user");
    >
    > - sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
    > - (test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
    > + err = enable_acl(sb);
    > + if (err)
    > + goto restore_opts;
    >
    > es = sbi->s_es;
    >
    > --
    > 2.5.0
    >


    Cheers, Andreas





    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2015-11-04 03:41    [W:2.797 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site