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

[Zhihao Cheng]

在 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx/
>
>   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;
> +
> +}