Received-SPF: pass (google.com: domain of linux-kernel+bounces-109633-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Subject: Re: [RFC PATCH 2/5] ubifs: Initialize or update ACLs for inode
To: Li Zetao <lizetao1@huawei.com>, <richard@nod.at>
CC: <linux-kernel@vger.kernel.org>, <linux-mtd@lists.infradead.org>
References: <20240319161646.2153867-1-lizetao1@huawei.com>
 <20240319161646.2153867-3-lizetao1@huawei.com>
From: Zhihao Cheng <chengzhihao1@huawei.com>
Message-ID: <2991c168-723d-48ef-8420-61e22a897239@huawei.com>
Date: Thu, 21 Mar 2024 11:47:56 +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: <20240319161646.2153867-3-lizetao1@huawei.com>
Content-Type: text/plain; charset="gbk"; format=flowed
Content-Transfer-Encoding: 8bit

?? 2024/3/20 0:16, Li Zetao д??:
> There are two scenarios where ACL needs to be updated, the first one
> is when creating the inode, and the second one is in the chmod process.
> When creating directories/files/device node/tmpfile, ACLs needs to be
> initialized, but symlink do not.Why not support symlink? It looks like many filesystems(eg. ext4, f2fs, 
btrfs) support it, except xfs.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>   fs/ubifs/dir.c  | 16 ++++++++++++++++
>   fs/ubifs/file.c |  4 ++++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 551148de66cd..dfb6823cc953 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -316,6 +316,10 @@ static int ubifs_create(struct mnt_idmap *idmap, struct inode *dir,
>   		goto out_fname;
>   	}
>   
> +	err = ubifs_init_acl(inode, dir);
> +	if (err)
> +		goto out_inode;
> +
Attention, a new inconsistent problem point is imported by acl xattr 
creation. See https://bugzilla.kernel.org/show_bug.cgi?id=218309.  @Richard
>   	err = ubifs_init_security(dir, inode, &dentry->d_name);
>   	if (err)
>   		goto out_inode;
> @@ -466,6 +470,10 @@ static int ubifs_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
>   	}
>   	ui = ubifs_inode(inode);
>   
> +	err = ubifs_init_acl(inode, dir);
> +	if (err)
> +		goto out_inode;
> +
>   	err = ubifs_init_security(dir, inode, &dentry->d_name);
>   	if (err)
>   		goto out_inode;
> @@ -1013,6 +1021,10 @@ static int ubifs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>   		goto out_fname;
>   	}
>   
> +	err = ubifs_init_acl(inode, dir);
> +	if (err)
> +		goto out_inode;
> +
>   	err = ubifs_init_security(dir, inode, &dentry->d_name);
>   	if (err)
>   		goto out_inode;
> @@ -1108,6 +1120,10 @@ static int ubifs_mknod(struct mnt_idmap *idmap, struct inode *dir,
>   	ui->data = dev;
>   	ui->data_len = devlen;
>   
> +	err = ubifs_init_acl(inode, dir);
> +	if (err)
> +		goto out_inode;
> +
>   	err = ubifs_init_security(dir, inode, &dentry->d_name);
>   	if (err)
>   		goto out_inode;
The whiteout inode is not set acl for rename(WHITEOUT) operation. It 
looks like many filesystems(eg. ext4, f2fs, btrfs) support it, except 
xfs. In my opinion, whiteout is a char dev, since char/block device is 
supported, why not support whiteout?

If we support whiteout, we should make sure that the whiteout renameing 
operation is atomic[1]. But I cannot come up with an idea how to combine 
whiteout xattr(acl) creation and whiteout file creation into an atomic 
operation, just like problem mentioned in [2],

[1] 
https://lore.kernel.org/linux-mtd/20211227032246.2886878-6-chengzhihao1@huawei.com/
[2] https://bugzilla.kernel.org/show_bug.cgi?id=218309
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 5029eb3390a5..8f964f8b0f96 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -41,6 +41,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)
> @@ -1298,6 +1299,9 @@ int ubifs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>   	else
>   		err = do_setattr(c, inode, attr);
>   
> +	if (!err && (attr->ia_valid & ATTR_MODE))
> +		err = posix_acl_chmod(idmap, dentry, inode->i_mode);
> +
>   	return err;
>   }
>   
>