Re: [PATCH v13 20/51] ext4: Add richacl support

Andreas Dilger
2015-11-04T02:13:23Z

[-- Attachment #1.1: Type: text/plain, Size: 12423 bytes --]

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>
>
> Support the richacl permission model in ext4. The richacls are stored
> in "system.richacl" xattrs. Richacls need to be enabled by tune2fs or
> at file system create time.

Patch looks reasonable. One minor cleanup below that could be fixed when
the patch series is refreshed, and you can add:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
> fs/ext4/Kconfig | 11 +++++
> fs/ext4/Makefile | 1 +
> fs/ext4/file.c | 3 ++
> fs/ext4/ialloc.c | 11 ++++-
> fs/ext4/inode.c | 12 ++++-
> fs/ext4/namei.c | 5 ++
> fs/ext4/richacl.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/ext4/richacl.h | 40 ++++++++++++++++
> fs/ext4/xattr.c | 7 +++
> 9 files changed, 228 insertions(+), 3 deletions(-)
> create mode 100644 fs/ext4/richacl.c
> create mode 100644 fs/ext4/richacl.h
>
> diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig
> index b46e9fc..65c5230 100644
> --- a/fs/ext4/Kconfig
> +++ b/fs/ext4/Kconfig
> @@ -22,6 +22,17 @@ config EXT3_FS_POSIX_ACL
> This config option is here only for backward compatibility. ext3
> filesystem is now handled by the ext4 driver.
>
> +config EXT4_FS_RICHACL
> + bool "Ext4 Rich Access Control Lists (EXPERIMENTAL)"
> + depends on EXT4_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 EXT3_FS_SECURITY
> bool "Ext3 Security Labels"
> depends on EXT3_FS
> diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
> index 75285ea..ea0d539 100644
> --- a/fs/ext4/Makefile
> +++ b/fs/ext4/Makefile
> @@ -14,3 +14,4 @@ ext4-$(CONFIG_EXT4_FS_POSIX_ACL) += acl.o
> ext4-$(CONFIG_EXT4_FS_SECURITY) += xattr_security.o
> ext4-$(CONFIG_EXT4_FS_ENCRYPTION) += crypto_policy.o crypto.o \
> crypto_key.o crypto_fname.o
> +ext4-$(CONFIG_EXT4_FS_RICHACL) += richacl.o
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 113837e..a03b4a5 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -30,6 +30,7 @@
> #include "ext4_jbd2.h"
> #include "xattr.h"
> #include "acl.h"
> +#include "richacl.h"
>
> /*
> * Called when an inode is released. Note that this is different
> @@ -719,6 +720,8 @@ const struct inode_operations ext4_file_inode_operations = {
> .removexattr = generic_removexattr,
> .get_acl = ext4_get_acl,
> .set_acl = ext4_set_acl,
> + .get_richacl = ext4_get_richacl,
> + .set_richacl = ext4_set_richacl,
> .fiemap = ext4_fiemap,
> };
>
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index 619bfc1..9657b3a 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -27,6 +27,7 @@
> #include "ext4_jbd2.h"
> #include "xattr.h"
> #include "acl.h"
> +#include "richacl.h"
>
> #include <trace/events/ext4.h>
>
> @@ -697,6 +698,14 @@ out:
> return ret;
> }
>
> +static inline int
> +ext4_new_acl(handle_t *handle, struct inode *inode, struct inode *dir)
> +{
> + if (IS_RICHACL(dir))
> + return ext4_init_richacl(handle, inode, dir);
> + return ext4_init_acl(handle, inode, dir);
> +}
> +
> /*
> * There are two policies for allocating an inode. If the new inode is
> * a directory, then a forward search is made for a block group with both
> @@ -1052,7 +1061,7 @@ got:
> if (err)
> goto fail_drop;
>
> - err = ext4_init_acl(handle, inode, dir);
> + err = ext4_new_acl(handle, inode, dir);
> if (err)
> goto fail_free_drop;
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 612fbcf..647f3c3 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -42,6 +42,7 @@
> #include "xattr.h"
> #include "acl.h"
> #include "truncate.h"
> +#include "richacl.h"
>
> #include <trace/events/ext4.h>
>
> @@ -4638,6 +4639,14 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)
> }
> }
>
> +static inline int
> +ext4_acl_chmod(struct inode *inode, umode_t mode)
> +{
> + if (IS_RICHACL(inode))
> + return richacl_chmod(inode, inode->i_mode);
> + return posix_acl_chmod(inode, inode->i_mode);
> +}
> +
> /*
> * ext4_setattr()
> *
> @@ -4806,8 +4815,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
> ext4_orphan_del(NULL, inode);
>
> if (!rc && (ia_valid & ATTR_MODE))
> - rc = posix_acl_chmod(inode, inode->i_mode);
> -
> + rc = ext4_acl_chmod(inode, inode->i_mode);
> err_out:
> ext4_std_error(inode->i_sb, error);
> if (!error)
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 9f61e76..9b6e8b9 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -38,6 +38,7 @@
>
> #include "xattr.h"
> #include "acl.h"
> +#include "richacl.h"
>
> #include <trace/events/ext4.h>
> /*
> @@ -3854,6 +3855,8 @@ const struct inode_operations ext4_dir_inode_operations = {
> .removexattr = generic_removexattr,
> .get_acl = ext4_get_acl,
> .set_acl = ext4_set_acl,
> + .get_richacl = ext4_get_richacl,
> + .set_richacl = ext4_set_richacl,
> .fiemap = ext4_fiemap,
> };
>
> @@ -3865,4 +3868,6 @@ const struct inode_operations ext4_special_inode_operations = {
> .removexattr = generic_removexattr,
> .get_acl = ext4_get_acl,
> .set_acl = ext4_set_acl,
> + .get_richacl = ext4_get_richacl,
> + .set_richacl = ext4_set_richacl,
> };
> diff --git a/fs/ext4/richacl.c b/fs/ext4/richacl.c
> new file mode 100644
> index 0000000..906d048
> --- /dev/null
> +++ b/fs/ext4/richacl.c
> @@ -0,0 +1,141 @@
> +/*
> + * Copyright IBM Corporation, 2010
> + * Copyright (C) 2015 Red Hat, Inc.
> + * Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>,
> + * 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 <linux/kernel.h>
> +#include <linux/fs.h>
> +#include <linux/richacl_xattr.h>
> +
> +#include "ext4.h"
> +#include "ext4_jbd2.h"
> +#include "xattr.h"
> +#include "acl.h"
> +#include "richacl.h"
> +
> +struct richacl *
> +ext4_get_richacl(struct inode *inode)
> +{
> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
> + void *value = NULL;
> + struct richacl *acl = NULL;
> + int retval;
> +
> + retval = ext4_xattr_get(inode, name_index, "", NULL, 0);
> + if (retval > 0) {
> + value = kmalloc(retval, GFP_NOFS);
> + if (!value)
> + return ERR_PTR(-ENOMEM);
> + retval = ext4_xattr_get(inode, name_index, "", value, retval);
> + }
> + if (retval > 0) {
> + acl = richacl_from_xattr(&init_user_ns, value, retval);
> + if (acl == ERR_PTR(-EINVAL))
> + acl = ERR_PTR(-EIO);
> + } else if (retval != -ENODATA && retval != -ENOSYS)
> + acl = ERR_PTR(retval);

(style) Typically, the braces on if/else blocks are kept matching.

> + kfree(value);
> +
> + if (!IS_ERR(acl))
> + set_cached_richacl(inode, acl);
> +
> + return acl;
> +}
> +
> +static int
> +__ext4_remove_richacl(handle_t *handle, struct inode *inode)
> +{
> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
> + int retval;
> +
> + retval = ext4_xattr_set_handle(handle, inode, name_index, "",
> + NULL, 0, 0);
> + if (!retval)
> + set_cached_richacl(inode, NULL);
> + return retval;
> +}
> +
> +static int
> +__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl *acl)
> +{
> + const int name_index = EXT4_XATTR_INDEX_RICHACL;
> + umode_t mode = inode->i_mode;
> + int retval, size;
> + void *value;
> +
> + if (richacl_equiv_mode(acl, &mode) == 0) {
> + inode->i_ctime = ext4_current_time(inode);
> + inode->i_mode = mode;
> + ext4_mark_inode_dirty(handle, inode);
> + return __ext4_remove_richacl(handle, inode);
> + }
> +
> + mode &= ~S_IRWXUGO;
> + mode |= richacl_masks_to_mode(acl);
> +
> + size = richacl_xattr_size(acl);
> + value = kmalloc(size, GFP_NOFS);
> + if (!value)
> + return -ENOMEM;
> + richacl_to_xattr(&init_user_ns, acl, value, size);
> + inode->i_mode = mode;
> + retval = ext4_xattr_set_handle(handle, inode, name_index, "",
> + value, size, 0);
> + kfree(value);
> + if (retval)
> + return retval;
> +
> + set_cached_richacl(inode, acl);
> +
> + return 0;
> +}
> +
> +int
> +ext4_set_richacl(struct inode *inode, struct richacl *acl)
> +{
> + handle_t *handle;
> + int retval, retries = 0;
> +
> +retry:
> + handle = ext4_journal_start(inode, EXT4_HT_XATTR,
> + ext4_jbd2_credits_xattr(inode));
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> +
> + if (acl)
> + retval = __ext4_set_richacl(handle, inode, acl);
> + else
> + retval = __ext4_remove_richacl(handle, inode);
> +
> + ext4_journal_stop(handle);
> + if (retval == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
> + goto retry;
> + return retval;
> +}
> +
> +int
> +ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
> +{
> + struct richacl *acl = richacl_create(&inode->i_mode, dir);
> + int error;
> +
> + error = PTR_ERR(acl);
> + if (IS_ERR(acl))
> + return error;
> + if (acl) {
> + error = __ext4_set_richacl(handle, inode, acl);
> + richacl_put(acl);
> + }
> + return error;
> +}
> diff --git a/fs/ext4/richacl.h b/fs/ext4/richacl.h
> new file mode 100644
> index 0000000..6fe9a92
> --- /dev/null
> +++ b/fs/ext4/richacl.h
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright IBM Corporation, 2010
> + * Copyright (C) 2015 Red Hat, Inc.
> + * Author Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.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_EXT4_RICHACL_H
> +#define __FS_EXT4_RICHACL_H
> +
> +#include <linux/richacl.h>
> +
> +#ifdef CONFIG_EXT4_FS_RICHACL
> +
> +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 */
> +
> +#define ext4_get_richacl NULL
> +#define ext4_set_richacl NULL
> +
> +static inline int
> +ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
> +{
> + return 0;
> +}
> +
> +#endif /* CONFIG_EXT4_FS_RICHACL */
> +#endif /* __FS_EXT4_RICHACL_H */
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 16e28c0..4d79adb 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -55,6 +55,7 @@
> #include <linux/slab.h>
> #include <linux/mbcache.h>
> #include <linux/quotaops.h>
> +#include <linux/richacl_xattr.h>
> #include "ext4_jbd2.h"
> #include "ext4.h"
> #include "xattr.h"
> @@ -99,6 +100,9 @@ static const struct xattr_handler *ext4_xattr_handler_map[] = {
> #ifdef CONFIG_EXT4_FS_SECURITY
> [EXT4_XATTR_INDEX_SECURITY] = &ext4_xattr_security_handler,
> #endif
> +#ifdef CONFIG_EXT4_FS_RICHACL
> + [EXT4_XATTR_INDEX_RICHACL] = &richacl_xattr_handler,
> +#endif
> };
>
> const struct xattr_handler *ext4_xattr_handlers[] = {
> @@ -111,6 +115,9 @@ const struct xattr_handler *ext4_xattr_handlers[] = {
> #ifdef CONFIG_EXT4_FS_SECURITY
> &ext4_xattr_security_handler,
> #endif
> +#ifdef CONFIG_EXT4_FS_RICHACL
> + &richacl_xattr_handler,
> +#endif
> NULL
> };
>
> --
> 2.5.0
>

Cheers, Andreas

[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]