Re: [RFC PATCH 1/5] ubifs: Implement POSIX Access Control Lists (ACLs)

[Li Zetao]

Hi,

On 2024/3/21 10:55, Zhihao Cheng wrote:
> 在 2024/3/20 0:16, Li Zetao 写道:
> > Implement the ACLs feature for ubifs based on vfs Posix ACLs,
> > details as follows:
> >    * Initialize acl for newly created inode.
> >    * Provides get/set interface to access ACLs.
> >
> > ACLs feature relies on xattr implementation which using specific key
> > names "system.posix_acl_default" and "system.posix_acl_access". Now Only
> > the v2 version of POSIX ACLs is supported, and ubifs does not need to
> > customize the storage format, which can simplify the implementation.
> >
> > Signed-off-by: Li Zetao <lizetao1@xxxxxxxxxx>
> > ---
> >   fs/ubifs/acl.c   | 140 +++++++++++++++++++++++++++++++++++++++++++++++
> >   fs/ubifs/ubifs.h |  13 +++++
> >   fs/ubifs/xattr.c |   1 -
> >   3 files changed, 153 insertions(+), 1 deletion(-)
> >   create mode 100644 fs/ubifs/acl.c
> >
> > diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
> > new file mode 100644
> > index 000000000000..253568baf097
> > --- /dev/null
> > +++ b/fs/ubifs/acl.c
> > @@ -0,0 +1,140 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * This file is part of UBIFS.
> > + *
> > + * Copyright (C) 2024 Huawei Tech. Co., Ltd.
> > + *
> > + * Authors: Li Zetao <lizetao1@xxxxxxxxxx>
> > + */
> > +
> > +/* This file implements POSIX Access Control Lists (ACLs) */
> > +
> > +#include "ubifs.h"
> > +
> > +#include <linux/posix_acl_xattr.h>
> > +
> > +struct posix_acl *ubifs_get_inode_acl(struct inode *inode, int type, 
> > bool rcu)
> > +{
> > +    char *xattr_value = NULL;
> > +    const char *xattr_name;
> > +    struct posix_acl *acl;
> > +    ssize_t size;
> > +
> > +    if (rcu)
> > +        return ERR_PTR(-ECHILD);
> > +
> > +    xattr_name = posix_acl_xattr_name(type);
> > +    if (unlikely(!strcmp(xattr_name, "")))
> > +        return ERR_PTR(-EINVAL);
> The acl type has been guaranteed valid from vfs caller, there is no need 
> to check converted name by 'strcmp', in theory, we can use it directly 
> just like f2fs does. For this case, I suggest to unfold the 
> posix_acl_xattr_name and convert it to corresponding name just like 
> btrfs does.
Ok, I will modify it in the next version.
> > +
> > +    size = ubifs_xattr_get(inode, xattr_name, NULL, 0);
> > +    if (size > 0) {
> > +        xattr_value = kzalloc(size, GFP_KERNEL);
> > +        if (unlikely(!xattr_value))
> > +            return ERR_PTR(-ENOMEM);
> > +
> > +        size = ubifs_xattr_get(inode, xattr_name, xattr_value, size);
> > +    }
> > +
> > +    if (size > 0)
> > +        acl = posix_acl_from_xattr(&init_user_ns, xattr_value, size);
> > +    else if (size == -ENODATA || size == 0)
> > +        acl = NULL;
> > +    else
> > +        acl = ERR_PTR(size);
> > +
> > +    kfree(xattr_value);
> > +
> > +    return acl;
> > +}
> > +
> > +static int __ubifs_set_acl(struct inode *inode, int type, struct 
> > posix_acl *acl, int flags)
> > +{
> > +    void *xattr_value = NULL;
> > +    const char *xattr_name;
> > +    size_t size = 0;
> > +    int error;
> > +
>
> > +    xattr_name = posix_acl_xattr_name(type);
> > +    if (unlikely(!strcmp(xattr_name, "")))
> > +        return -EINVAL;
> > +
> > +    if (unlikely(!strcmp(xattr_name, XATTR_NAME_POSIX_ACL_DEFAULT) && 
> > !S_ISDIR(inode->i_mode)))
> > +        return acl ? -EACCES : 0;
> > +
> Similar to previous, replace above 6 lines, refer to __btrfs_set_acl but 
> keep the error code same with __ext4_set_acl.
Ok.
> > +    if (acl) {
> > +        size = posix_acl_xattr_size(acl->a_count);
> > +        xattr_value = kmalloc(size, GFP_KERNEL);
> > +        if (unlikely(!xattr_value))
> > +            return -ENOMEM;
> > +
> > +        error = posix_acl_to_xattr(&init_user_ns, acl, xattr_value, 
> > size);
> > +        if (unlikely(error < 0))
> > +            goto out;
> > +    }
> > +
> > +    error = ubifs_xattr_set(inode, xattr_name, xattr_value, size, 
> > flags, false);
> There are 2 situations here, Updating acl and Removing acl. For the 
> later case, funcion vfs_remove_acl will remove corresponding xattr, the 
> xattr removing function in ubifs is ubifs_xattr_remove.
Thanks. Indeed, ubifs_xattr_set() is is different from other file 
systems, it will not delete xattr when value is NULL.
> > +    if (likely(!error))
> I prefer to remove the 'likely', UBIFS limits the max xattr count for 
> each file(Goto create_xattr), non zero error returned is a common case 
> on a small LEB flash.
Ok.
> > +        set_cached_acl(inode, type, acl);
> > +out:
> > +    kfree(xattr_value);
> > +    return error;
> > +}
> > +
> > +int ubifs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, 
> > struct posix_acl *acl, int type)
> > +{
> > +    struct inode *inode = d_inode(dentry);
> > +    umode_t old_mode = inode->i_mode;
> > +    int error;
> > +
> > +    if (type == ACL_TYPE_ACCESS && acl) {
> > +        error = posix_acl_update_mode(idmap, inode, &inode->i_mode, 
> > &acl);
> > +        if (unlikely(error))
> > +            return error;
> > +    }
> > +
> > +    error = __ubifs_set_acl(inode, type, acl, 0);
> > +    if (unlikely(error))
> Mentioned in __ubifs_set_acl, error could be returned, just remove 
> 'unlikely'.
Ok.
> > +        inode->i_mode = old_mode;
> > +
> > +    return error;
> > +
> > +}
> > +
> > +/**
> > + * ubifs_init_acl - initialize the ACLs for a new inode.
> > + * @inode: newly created inode
> > + * @dir: parent directory inode
> > + *
> > + * This function initialize ACLs, including inheriting the
> initialize -> initializes
Ok.
> > + * default ACLs of parent directory or modifying the default
> > + * ACLs according to the mode parameter in open() / creat()
> > + * system calls.
> > + */
> > +int ubifs_init_acl(struct inode *inode, struct inode *dir)
> > +{
> > +    struct posix_acl *default_acl;
> > +    struct posix_acl *acl;
> > +    int error;
> > +
> > +    error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> > +    if (unlikely(error))
> > +        return error;
> > +
> > +    if (default_acl) {
> > +        error = __ubifs_set_acl(inode, ACL_TYPE_DEFAULT, default_acl, 
> > XATTR_CREATE);
> > +        posix_acl_release(default_acl);
> > +    } else {
> > +        inode->i_default_acl = NULL;
> > +    }
> > +
> > +    if (acl) {
> > +        if (likely(!error))
> Remove 'likely'.
Ok.
> > +            error = __ubifs_set_acl(inode, ACL_TYPE_ACCESS, acl, 
> > XATTR_CREATE);
> > +        posix_acl_release(acl);
> > +    } else {
> > +        inode->i_acl = NULL;
> > +    }
> > +
> > +    return error;
> > +}
> > diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> > index 3916dc4f30ca..b0d3b076290d 100644
> > --- a/fs/ubifs/ubifs.h
> > +++ b/fs/ubifs/ubifs.h
> > @@ -2069,6 +2069,19 @@ static inline int ubifs_init_security(struct 
> > inode *dentry,
> >   }
> >   #endif
> > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> > +struct posix_acl *ubifs_get_inode_acl(struct inode *inode, int type, 
> > bool rcu);
> > +int ubifs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, 
> > struct posix_acl *acl, int type);
> > +int ubifs_init_acl(struct inode *inode, struct inode *dir);
> > +
> > +#else /* CONFIG_UBIFS_FS_POSIX_ACL */
> > +#define ubifs_get_inode_acl NULL
> > +#define ubifs_set_acl NULL
> > +static inline int ubifs_init_acl(struct inode *inode, struct inode *dir)
> > +{
> > +    return 0;
> > +}
> > +#endif /* CONFIG_UBIFS_FS_POSIX_ACL */
> >   /* 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 0847db521984..eb1c1f5d10df 100644
> > --- a/fs/ubifs/xattr.c
> > +++ b/fs/ubifs/xattr.c
> > @@ -40,7 +40,6 @@
> >    * in the VFS inode cache. The xentries are cached in the LNC cache 
> > (see
> >    * tnc.c).
> >    *
> > - * ACL support is not implemented.
> >    */
> >   #include "ubifs.h"