[PATCH] ubifs: implement POSIX Access Control Lists (ACLs)
Pascal Eberhard
pascal.eberhard at gmail.com
Wed Oct 19 17:04:36 PDT 2016
Hi Andreas,
Thanks for your comments.
On 2016-10-19 16:34, Andreas Gruenbacher wrote:
> Pascal,
>
> On Tue, Oct 18, 2016 at 11:32 PM, Richard Weinberger <richard at nod.at>
> wrote:
>> Pascal,
>>
>> On 09.10.2016 22:53, Pascal Eberhard wrote:
>>> This patch implements POSIX Access Control Lists (ACLs) in UBIFS.
>>> It uses posix_acl_(access|default)_xattr_handler ACL generic handlers
>>> as xattr handlers are now used in UBIFS.
>>
>> Cool! :)
>> CC'ing Andreas, maybe he also has comments.
>
> please run this through xfstests as well. Note the following fixes /
> improvements I've made in the course of reviewing this; they've
> obviously not been merged, yet:
>
> https://github.com/andreas-gruenbacher/xfsprogs-dev/commits/io
> https://github.com/andreas-gruenbacher/xfstests/commits/tmpfile
Ok, nice :), I will run them.
>>> Signed-off-by: Pascal Eberhard <pascal.eberhard at gmail.com>
>>> ---
>>> fs/ubifs/Kconfig | 13 +++++
>>> fs/ubifs/Makefile | 1 +
>>> fs/ubifs/acl.c | 143
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> fs/ubifs/dir.c | 22 +++++++++
>>> fs/ubifs/file.c | 9 ++++
>>> fs/ubifs/super.c | 15 ++++++
>>> fs/ubifs/ubifs.h | 18 +++++++
>>> fs/ubifs/xattr.c | 11 +++--
>>> 8 files changed, 229 insertions(+), 3 deletions(-)
>>> create mode 100644 fs/ubifs/acl.c
>>>
>>> diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
>>> index 7ff7712..39119e5 100644
>>> --- a/fs/ubifs/Kconfig
>>> +++ b/fs/ubifs/Kconfig
>>> @@ -50,3 +50,16 @@ config UBIFS_ATIME_SUPPORT
>>> strictatime is the "heavy", relatime is "lighter", etc.
>>>
>>> If unsure, say 'N'
>>> +
>>> +config UBIFS_FS_POSIX_ACL
>>> + bool "UBIFS POSIX Access Control Lists"
>>> + depends on UBIFS_FS
>>> + select FS_POSIX_ACL
>>> + help
>>> + POSIX Access Control Lists (ACLs) support permissions for
>>> users and
>>> + groups beyond the owner/group/world scheme.
>>> +
>>> + To learn more about Access Control Lists, visit the POSIX
>>> ACLs for
>>> + Linux website <http://acl.bestbits.at/>.
>>> +
>>> + If you don't know what Access Control Lists are, say N
>>> diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
>>> index c54a243..af7a83f 100644
>>> --- a/fs/ubifs/Makefile
>>> +++ b/fs/ubifs/Makefile
>>> @@ -5,3 +5,4 @@ ubifs-y += tnc.o master.o scan.o replay.o log.o
>>> commit.o gc.o orphan.o
>>> ubifs-y += budget.o find.o tnc_commit.o compress.o lpt.o lprops.o
>>> ubifs-y += recovery.o ioctl.o lpt_commit.o tnc_misc.o xattr.o
>>> debug.o
>>> ubifs-y += misc.o
>>> +ubifs-$(CONFIG_UBIFS_FS_POSIX_ACL) += acl.o
>>> \ No newline at end of file
>>> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
>>> new file mode 100644
>>> index 0000000..a4d4734
>>> --- /dev/null
>>> +++ b/fs/ubifs/acl.c
>>> @@ -0,0 +1,143 @@
>>> +/*
>>> + * This file is part of UBIFS.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> modify it
>>> + * under the terms of the GNU General Public License version 2 as
>>> published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will 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.
>>> + *
>>> + */
>>> +
>>> +#include <linux/fs.h>
>>
>> ubifs.h includes this already.
>>
>>> +#include <linux/xattr.h>
>>
>> Ditto.
>>
>>> +#include <linux/posix_acl_xattr.h>
>>> +
>>> +#include "ubifs.h"
>>> +
>>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type)
>>> +{
>>> + int size;
>>> + const char *name;
>>> + char *value = NULL;
>>> + struct posix_acl *acl;
>>> +
>>> + switch (type) {
>>> + case ACL_TYPE_ACCESS:
>>> + name = XATTR_NAME_POSIX_ACL_ACCESS;
>>> + break;
>>> + case ACL_TYPE_DEFAULT:
>>> + name = XATTR_NAME_POSIX_ACL_DEFAULT;
>>> + break;
>>> + default:
>>> + return ERR_PTR(-EINVAL);
>>> + }
>>> +
>>> + size = __ubifs_getxattr(inode, name, NULL, 0);
>>> + if (size > 0) {
>>> + value = kzalloc(size, GFP_KERNEL);
>>> + if (!value)
>>> + return ERR_PTR(-ENOMEM);
>>> + size = __ubifs_getxattr(inode, name, value, size);
>>> + }
>>> + if (size > 0)
>>> + acl = posix_acl_from_xattr(&init_user_ns, value, size);
>>> + else if (size == -ERANGE || size == -ENODATA || size == 0)
>>> + acl = NULL;
>>> + else
>>> + acl = ERR_PTR(size);
>>> +
>>> + kfree(value);
>>> + if (!IS_ERR(acl))
>>> + set_cached_acl(inode, type, acl);
>
> The get_acl inode operation should no longer call set_cached_acl; the
> cached ACL is updated automatically. (It's correct to use
> set_cached_acl in the set_acl inode operation though.)
ok
>>> +
>>> + return acl;
>>> +}
>>> +
>>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int
>>> type)
>>> +{
>>> + int ret;
>>> + int size = 0;
>>> + const char *name;
>>> + char *value = NULL;
>>> + int flags = 0;
>>> +
>>> + switch (type) {
>>> + case ACL_TYPE_ACCESS:
>>> + name = XATTR_NAME_POSIX_ACL_ACCESS;
>>> + if (acl) {
>>> + ret = posix_acl_equiv_mode(acl,
>>> &inode->i_mode);
>>> + if (ret < 0)
>>> + return ret;
>>> + if (ret == 0)
>>> + acl = NULL;
>>> + }
>>> + ret = 0;
>>> + break;
>>> + case ACL_TYPE_DEFAULT:
>>> + if (!S_ISDIR(inode->i_mode))
>>> + return acl ? -EINVAL : 0;
>>> + name = XATTR_NAME_POSIX_ACL_DEFAULT;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (acl) {
>>> + size = posix_acl_xattr_size(acl->a_count);
>>> + value = kmalloc(size, GFP_KERNEL);
>>> + if (!value)
>>> + return -ENOMEM;
>>> +
>>> + ret = posix_acl_to_xattr(&init_user_ns, acl, value,
>>> size);
>>> + if (ret < 0)
>>> + goto out;
>>> + }
>>> +
>>> + if (size == 0)
>>> + flags = XATTR_REPLACE;
>>> + ret = __ubifs_setxattr(inode, name, value, size, flags);
>>> +
>>> +out:
>>> + kfree(value);
>>> + if (!ret)
>>> + set_cached_acl(inode, type, acl);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/*
>>> + * Initialize the ACLs of a new inode.
>>
>> Please use the Javadoc style we use for all UBIFS functions.
>> (applies to other functions too in this patch)-
>>
>>> + */
>>> +int ubifs_init_acl(struct inode *dir, struct inode *inode)
>>> +{
>>> + struct posix_acl *default_acl, *acl;
>>> + int ret = 0;
>>> +
>>> + ret = posix_acl_create(dir, &inode->i_mode, &default_acl,
>>> &acl);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (default_acl) {
>>> + inode_lock(inode);
>
> Why the inode locking here?
I see no reason why we should keep it but I will double check.
>>> + ret = ubifs_set_acl(inode, default_acl,
>>> ACL_TYPE_DEFAULT);
>>> + inode_unlock(inode);
>>> + posix_acl_release(default_acl);
>>> + }
>>> +
>>> + if (acl) {
>>> + if (!ret) {
>>> + inode_lock(inode);
>
> Same here.
Same here :)
>>> + ret = ubifs_set_acl(inode, acl,
>>> ACL_TYPE_ACCESS);
>>> + inode_unlock(inode);
>>> + }
>>> + posix_acl_release(acl);
>>> + }
>>> +
>>> + if (!default_acl && !acl)
>>> + cache_no_acl(inode);
>
> This call to cache_no_acl doesn't make sense; please check. It may be
> wrong in btrfs as well; copying Josef for that.
Both i_acl and i_default_acl are set to ACL_NOT_CACHED when entering
ubifs_init_acl().
As proposed, the function will be updated so cache_no_acl() will be
called unconditionally before setting the actual ACLs (if any).
>>> + return ret;
>>> +}
>>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>>> index ccd9128..a2e5609 100644
>>> --- a/fs/ubifs/dir.c
>>> +++ b/fs/ubifs/dir.c
>>> @@ -270,6 +270,10 @@ static int ubifs_create(struct inode *dir,
>>> struct dentry *dentry, umode_t mode,
>>> goto out_budg;
>>> }
>>>
>>> + err = ubifs_init_acl(dir, inode);
>>> + if (err)
>>> + goto out_inode;
>>> +
>
> The pattern ubifs_budget_space / ubifs_new_inode / ubifs_init_acl /
> ubifs_init_security repeats a number of times in the code, so it
> should probably be put in a function.
I will look into it.
>>> err = ubifs_init_security(dir, inode, &dentry->d_name);
>>> if (err)
>>> goto out_inode;
>>> @@ -341,6 +345,10 @@ static int do_tmpfile(struct inode *dir, struct
>>> dentry *dentry,
>>> ubifs_assert(inode->i_op ==
>>> &ubifs_file_inode_operations);
>>> }
>>>
>>> + err = ubifs_init_acl(dir, inode);
>>> + if (err)
>>> + goto out_inode;
>>
>> Why do we need ACLs for O_TMPFILE?
>>
>>> +
>>> err = ubifs_init_security(dir, inode, &dentry->d_name);
>>> if (err)
>>> goto out_inode;
>>> @@ -821,6 +829,10 @@ static int ubifs_mkdir(struct inode *dir, struct
>>> dentry *dentry, umode_t mode)
>>> goto out_budg;
>>> }
>>>
>>> + err = ubifs_init_acl(dir, inode);
>>> + if (err)
>>> + goto out_inode;
>>> +
>>> err = ubifs_init_security(dir, inode, &dentry->d_name);
>>> if (err)
>>> goto out_inode;
>>> @@ -903,6 +915,10 @@ static int ubifs_mknod(struct inode *dir, struct
>>> dentry *dentry,
>>> ui->data = dev;
>>> ui->data_len = devlen;
>>>
>>> + err = ubifs_init_acl(dir, inode);
>>> + if (err)
>>> + goto out_inode;
>>> +
>>> err = ubifs_init_security(dir, inode, &dentry->d_name);
>>> if (err)
>>> goto out_inode;
>>> @@ -985,6 +1001,10 @@ static int ubifs_symlink(struct inode *dir,
>>> struct dentry *dentry,
>>> ui->data_len = len;
>>> inode->i_size = ubifs_inode(inode)->ui_size = len;
>>>
>>> + err = ubifs_init_acl(dir, inode);
>>> + if (err)
>>> + goto out_inode;
>>
>> Do we really need ACLs for symlinks too? I thought permissions for
>> symlinks are irrelevant.
>>
>>> err = ubifs_init_security(dir, inode, &dentry->d_name);
>>> if (err)
>>> goto out_inode;
>>> @@ -1395,6 +1415,8 @@ const struct inode_operations
>>> ubifs_dir_inode_operations = {
>>> .update_time = ubifs_update_time,
>>> #endif
>>> .tmpfile = ubifs_tmpfile,
>>> + .get_acl = ubifs_get_acl,
>>> + .set_acl = ubifs_set_acl,
>>> };
>>>
>>> const struct file_operations ubifs_dir_operations = {
>>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>>> index 4ed270b..17f6098 100644
>>> --- a/fs/ubifs/file.c
>>> +++ b/fs/ubifs/file.c
>>> @@ -53,6 +53,7 @@
>>> #include <linux/mount.h>
>>> #include <linux/slab.h>
>>> #include <linux/migrate.h>
>>> +#include <linux/posix_acl.h>
>>>
>>> static int read_block(struct inode *inode, void *addr, unsigned int
>>> block,
>>> struct ubifs_data_node *dn)
>>> @@ -1251,6 +1252,10 @@ static int do_setattr(struct ubifs_info *c,
>>> struct inode *inode,
>>> ubifs_release_budget(c, &req);
>>> if (IS_SYNC(inode))
>>> err = inode->i_sb->s_op->write_inode(inode, NULL);
>>> +
>>> + if (!err && (attr->ia_valid & ATTR_MODE))
>>> + err = posix_acl_chmod(inode, inode->i_mode);
>>> +
>>> return err;
>>> }
>>>
>>> @@ -1628,6 +1633,8 @@ const struct inode_operations
>>> ubifs_file_inode_operations = {
>>> #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>> .update_time = ubifs_update_time,
>>> #endif
>>> + .get_acl = ubifs_get_acl,
>>> + .set_acl = ubifs_set_acl,
>>> };
>>>
>>> const struct inode_operations ubifs_symlink_inode_operations = {
>>> @@ -1642,6 +1649,8 @@ const struct inode_operations
>>> ubifs_symlink_inode_operations = {
>>> #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>> .update_time = ubifs_update_time,
>>> #endif
>>> + .get_acl = ubifs_get_acl,
>>> + .set_acl = ubifs_set_acl,
>>> };
>>>
>>> const struct file_operations ubifs_file_operations = {
>>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>>> index 4ec0510..8239dfd 100644
>>> --- a/fs/ubifs/super.c
>>> +++ b/fs/ubifs/super.c
>>> @@ -444,6 +444,9 @@ static int ubifs_show_options(struct seq_file *s,
>>> struct dentry *root)
>>> ubifs_compr_name(c->mount_opts.compr_type));
>>> }
>>>
>>> + if (c->vfs_sb->s_flags & MS_POSIXACL)
>>> + seq_puts(s, ",acl");
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -929,6 +932,8 @@ enum {
>>> Opt_chk_data_crc,
>>> Opt_no_chk_data_crc,
>>> Opt_override_compr,
>>> + Opt_acl,
>>> + Opt_noacl,
>>> Opt_err,
>>> };
>>>
>>> @@ -940,6 +945,8 @@ static const match_table_t tokens = {
>>> {Opt_chk_data_crc, "chk_data_crc"},
>>> {Opt_no_chk_data_crc, "no_chk_data_crc"},
>>> {Opt_override_compr, "compr=%s"},
>>> + {Opt_acl, "acl"},
>>> + {Opt_noacl, "noacl"},
>>> {Opt_err, NULL},
>>> };
>>>
>>> @@ -1040,6 +1047,14 @@ static int ubifs_parse_options(struct
>>> ubifs_info *c, char *options,
>>> c->default_compr = c->mount_opts.compr_type;
>>> break;
>>> }
>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>> + case Opt_acl:
>>> + c->vfs_sb->s_flags |= MS_POSIXACL;
>>> + break;
>>> + case Opt_noacl:
>>> + c->vfs_sb->s_flags &= ~MS_POSIXACL;
>>> + break;
>>> +#endif
>>> default:
>>> {
>>> unsigned long flag;
>>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>>> index 096035e..7935449 100644
>>> --- a/fs/ubifs/ubifs.h
>>> +++ b/fs/ubifs/ubifs.h
>>> @@ -1743,6 +1743,24 @@ extern const struct xattr_handler
>>> *ubifs_xattr_handlers[];
>>> ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t
>>> size);
>>> int ubifs_init_security(struct inode *dentry, struct inode *inode,
>>> const struct qstr *qstr);
>>> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>>> + void *buf, size_t size);
>>> +int __ubifs_setxattr(struct inode *host, const char *name,
>>> + const void *value, size_t size, int flags);
>>> +
>>> +/* acl.c */
>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>> +int ubifs_init_acl(struct inode *dir, struct inode *inode);
>>> +int ubifs_set_acl(struct inode *inode, struct posix_acl *acl, int
>>> type);
>>> +struct posix_acl *ubifs_get_acl(struct inode *inode, int type);
>>> +#else
>>> +static inline int ubifs_init_acl(struct inode *inode, struct inode
>>> *dir)
>>> +{
>>> + return 0;
>>> +}
>>> +#define ubifs_get_acl NULL
>>> +#define ubifs_set_acl NULL
>>> +#endif
>>>
>>> /* super.c */
>>> struct inode *ubifs_iget(struct super_block *sb, unsigned long
>>> inum);
>>> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
>>> index 6c2f4d4..0a596a1 100644
>>> --- a/fs/ubifs/xattr.c
>>> +++ b/fs/ubifs/xattr.c
>>> @@ -52,13 +52,14 @@
>>> * in the VFS inode cache. The xentries are cached in the LNC cache
>>> (see
>>> * tnc.c).
>>> *
>>> - * ACL support is not implemented.
>>> + * ACL support is now implemented.
>>
>> I think we can just drop that line now. :-)
>>
>>> */
>>>
>>> #include "ubifs.h"
>>> #include <linux/fs.h>
>>> #include <linux/slab.h>
>>> #include <linux/xattr.h>
>>> +#include <linux/posix_acl_xattr.h>
>>
>> While we are here, these includes should also be cleaned up.
>>
>>> /*
>>> * Limit the number of extended attributes per inode so that the
>>> total size
>>> @@ -268,7 +269,7 @@ static struct inode *iget_xattr(struct ubifs_info
>>> *c, ino_t inum)
>>> return ERR_PTR(-EINVAL);
>>> }
>>>
>>> -static int __ubifs_setxattr(struct inode *host, const char *name,
>>> +int __ubifs_setxattr(struct inode *host, const char *name,
>>> const void *value, size_t size, int flags)
>>> {
>>> struct inode *inode;
>>> @@ -328,7 +329,7 @@ out_free:
>>> return err;
>>> }
>>>
>>> -static ssize_t __ubifs_getxattr(struct inode *host, const char
>>> *name,
>>> +ssize_t __ubifs_getxattr(struct inode *host, const char *name,
>>> void *buf, size_t size)
>>> {
>>> struct inode *inode;
>>> @@ -619,5 +620,9 @@ const struct xattr_handler
>>> *ubifs_xattr_handlers[] = {
>>> &ubifs_user_xattr_handler,
>>> &ubifs_trusted_xattr_handler,
>>> &ubifs_security_xattr_handler,
>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>>> + &posix_acl_access_xattr_handler,
>>> + &posix_acl_default_xattr_handler,
>>> +#endif
>>> NULL
>>> };
>>>
>>
>> Overall the patch does not look bad and I'm sure we can add it for
>> v4.10 soon.
>> I'm a bit confused since most stuff is not really UBIFS specific.
>> Andreas, is just "normal" for ACL support? I thought most code is
>> generic and already in VFS.
>
> There are several filesystems like ubifs that use the xattr
> representation as the on-disk format, so ubifs_get_acl and
> ubifs_set_acl could be abstracted and shared across those filesystems.
I'd like that, unfortunately ubifs_[gs]et_acl() calls
__ubifs_[gs]etxattr() which
are internal ubifs functions. I am not sure yet how this could be
abstracted.
I will look into it too.
> The rest looks good.
>
> Thanks,
> Andreas
More information about the linux-mtd
mailing list