[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <daa0f858-ea85-1bf3-906c-4ef1a4998ccf@huawei.com>
Date: Sat, 23 Mar 2024 10:42:08 +0800
From: Zhihao Cheng <chengzhihao1@...wei.com>
To: Li Zetao <lizetao1@...wei.com>, <richard@....at>, <corbet@....net>,
<kent.overstreet@...ux.dev>, <agruenba@...hat.com>
CC: <linux-mtd@...ts.infradead.org>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 2/5] ubifs: Implement POSIX Access Control Lists
(ACLs)
在 2024/3/22 23:48, 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.
>
> It should be noted that Linux supports updating the file mode through
> ACLs. However the acl may not exist, so ubifs_xattr_remove() returns
> -ENODATA. Such a scenario needs to be specially handled. At the same
> time, it needs to ensure that the updated inode is written to flash.
>
> Signed-off-by: Li Zetao <lizetao1@...wei.com>
> ---
> v1 -> v2:
> * Get xattr_name by direct expansion instead of posix_acl_xattr_name().
> * Modify ubifs_xattr_remove to an external function to remove the xattr of ACL.
> * Remove redundant likely() and unlikely().
> * Fix updating file mode via ACL and support writing to flash.
>
> v1: https://lore.kernel.org/all/20240319161646.2153867-2-lizetao1@huawei.com/
>
> fs/ubifs/Makefile | 1 +
> fs/ubifs/acl.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++
> fs/ubifs/ubifs.h | 14 ++++
> fs/ubifs/xattr.c | 3 +-
> 4 files changed, 208 insertions(+), 2 deletions(-)
> create mode 100644 fs/ubifs/acl.c
>
[...]
> +static int ubifs_update_mode(struct inode *inode, umode_t mode)
> +{
> + struct ubifs_inode *ui = ubifs_inode(inode);
> + struct ubifs_info *c = inode->i_sb->s_fs_info;
> + struct ubifs_budget_req req = { .dirtied_ino = 1,
> + .dirtied_ino_d = ALIGN(ui->data_len, 8) };
> + int release;
> + int err;
> +
> + err = ubifs_budget_space(c, &req);
> + if (err)
> + return err;
> +
> + mutex_lock(&ui->ui_mutex);
> + release = ui->dirty;
> + inode->i_mode = mode;
> + inode_set_ctime_current(inode);
> + mark_inode_dirty_sync(inode);
> + mutex_unlock(&ui->ui_mutex);
> +
> + if (release)
> + ubifs_release_budget(c, &req);
> + if (IS_SYNC(inode))
> + err = inode->i_sb->s_op->write_inode(inode, NULL);
> +
> + return err;
> +}
> +
> +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 mode = inode->i_mode;
> + bool update_mode = false;
> + int error;
> +
> + if (type == ACL_TYPE_ACCESS && acl) {
> + error = posix_acl_update_mode(idmap, inode, &mode, &acl);
> + if (unlikely(error))
> + return error;
> +
> + if (inode->i_mode != mode)
> + update_mode = true;
> + }
> +
> + error = __ubifs_set_acl(inode, type, acl, 0);
> + if (!error && update_mode)
> + error = ubifs_update_mode(inode, mode);
Updating inode mode to disk is a right thing. However, this makes
ubifs_set_acl is not atomic, which is manifested in two points:
1. If ubifs_budget_space fails by ENOSPC, __ubifs_set_acl has stored
xattrs into disk, but inode mode is not updated, which makes inode->mode
be inconsistent with acl. This problem can be easily solved by moving
ubifs_budget_space before the __ubifs_set_acl.
2. If ubifs_write_inode fails or a powercut happens between
__ubifs_set_acl and ubifs_write_inode, inode->mode becomes inconsistent
with acl. PS: Ext4 makes set_acl atomic by 'handle'.
> +
> + return error;
> +
> +}
Powered by blists - more mailing lists