linux-kernel - Re: [RFC PATCH 2/5] ubifs: Initialize or update ACLs for inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2991c168-723d-48ef-8420-61e22a897239@huawei.com>
Date: Thu, 21 Mar 2024 11:47:56 +0800
From: Zhihao Cheng <chengzhihao1@...wei.com>
To: Li Zetao <lizetao1@...wei.com>, <richard@....at>
CC: <linux-kernel@...r.kernel.org>, <linux-mtd@...ts.infradead.org>
Subject: Re: [RFC PATCH 2/5] ubifs: Initialize or update ACLs for inode

在 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@...wei.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;
>   }
>   
>