Received-SPF: pass (google.com: domain of linux-kernel+bounces-111389-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <661736fb-bb3c-3519-1f4b-44dff285ea0b@huawei.com>
Date: Fri, 22 Mar 2024 19:57:24 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [RFC PATCH 2/5] ubifs: Initialize or update ACLs for inode
Content-Language: en-US
To: Zhihao Cheng <chengzhihao1@huawei.com>, <richard@nod.at>,
	<kent.overstreet@linux.dev>, <agruenba@redhat.com>
CC: <linux-kernel@vger.kernel.org>, <linux-mtd@lists.infradead.org>
References: <20240319161646.2153867-1-lizetao1@huawei.com>
 <20240319161646.2153867-3-lizetao1@huawei.com>
 <2991c168-723d-48ef-8420-61e22a897239@huawei.com>
From: Li Zetao <lizetao1@huawei.com>
In-Reply-To: <2991c168-723d-48ef-8420-61e22a897239@huawei.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

Hi,

On 2024/3/21 11:47, Zhihao Cheng wrote:
> 在 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.
Thanks for the reviews, but this is inconsistent with my understanding.
I think most file systems in Linux do not support it, because most file 
systems do not register the get/set functions of ACLs for symlink 
operations. And the posix_acl_create() will determine that it is a 
symlink type inode, and then skip the creation process. But except for 
bcachefs, it may be to solve the problem of certain scenarios, so it 
would be nice if anyone could explain it to us.
>>
>> 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
This problem is indeed a bit tricky.
>>       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],
Yes, thanks, I have fixed it in v2 version.
> 
> [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;
>>   }
>>
>