Received-SPF: pass (google.com: domain of linux-kernel+bounces-112190-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Subject: Re: [RFC PATCH v2 2/5] ubifs: Implement POSIX Access Control Lists
 (ACLs)
To: Li Zetao <lizetao1@huawei.com>, <richard@nod.at>, <corbet@lwn.net>,
	<kent.overstreet@linux.dev>, <agruenba@redhat.com>
CC: <linux-mtd@lists.infradead.org>, <linux-doc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
References: <20240322154812.215369-1-lizetao1@huawei.com>
 <20240322154812.215369-3-lizetao1@huawei.com>
From: Zhihao Cheng <chengzhihao1@huawei.com>
Message-ID: <daa0f858-ea85-1bf3-906c-4ef1a4998ccf@huawei.com>
Date: Sat, 23 Mar 2024 10:42:08 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240322154812.215369-3-lizetao1@huawei.com>
Content-Type: text/plain; charset="gbk"; format=flowed
Content-Transfer-Encoding: 8bit

?? 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@huawei.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;
> +
> +}