[RFC PATCH 0/5] ubifs: Support POSIX Access Control Lists (ACLs)

[RFC PATCH 0/5] ubifs: Support POSIX Access Control Lists (ACLs)

[Li Zetao]

Hi,

This patchset is base on [1] and [2], adding implementation of ACLs for
ubifs.

Implement ACLs features based on POSIX to solve some difficulties that
require fine-grained access control. At the same time, it is also to
facilitate cross-file system migration.

In order to simplify the implementation, only v2 version POSIX ACLs are
implemented, eliminating the need for in-memory and on-flash format
conversion. And no need to implement security xattr handler in ubifs.

Some testcases have been tested and passed:
  * generic testcases (modified version) for acl group in xfstest[3], they are generic/026/053/077/099/105/237/307/318/319/375/389/444/449/529/697.
  * tacl_xattr.sh (modified version) in LTP[4].

[1]: https://lore.kernel.org/linux-mtd/1441962597-13543-1-git-send-email-shengyong1@huawei.com/
[2]: https://lore.kernel.org/linux-mtd/1476046382-19185-1-git-send-email-pascal.eberhard@gmail.com/
[3]: https://kernel.googlesource.com/pub/scm/fs/xfs/xfstests-dev/+/refs/heads/master/tests/generic/
[4]: https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/fs/acl/tacl_xattr.sh

Li Zetao (5):
  ubifs: Implement POSIX Access Control Lists (ACLs)
  ubifs: Initialize or update ACLs for inode
  ubifs: Support accessing ACLs through inode_operations
  ubifs: Introduce ACLs mount options
  ubifs: Add ACLs config option

 fs/ubifs/Kconfig  |  14 +++++
 fs/ubifs/Makefile |   1 +
 fs/ubifs/acl.c    | 140 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/ubifs/dir.c    |  18 ++++++
 fs/ubifs/file.c   |   6 ++
 fs/ubifs/super.c  |  40 +++++++++++++
 fs/ubifs/ubifs.h  |  15 +++++
 fs/ubifs/xattr.c  |   1 -
 8 files changed, 234 insertions(+), 1 deletion(-)
 create mode 100644 fs/ubifs/acl.c

-- 
2.34.1

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

[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.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 fs/ubifs/acl.c   | 140 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/ubifs/ubifs.h |  13 +++++
 fs/ubifs/xattr.c |   1 -
 3 files changed, 153 insertions(+), 1 deletion(-)
 create mode 100644 fs/ubifs/acl.c

diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
new file mode 100644
index 000000000000..253568baf097
--- /dev/null
+++ b/fs/ubifs/acl.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * This file is part of UBIFS.
+ *
+ * Copyright (C) 2024 Huawei Tech. Co., Ltd.
+ *
+ * Authors: Li Zetao <lizetao1@huawei.com>
+ */
+
+/* This file implements POSIX Access Control Lists (ACLs) */
+
+#include "ubifs.h"
+
+#include <linux/posix_acl_xattr.h>
+
+struct posix_acl *ubifs_get_inode_acl(struct inode *inode, int type, bool rcu)
+{
+	char *xattr_value = NULL;
+	const char *xattr_name;
+	struct posix_acl *acl;
+	ssize_t size;
+
+	if (rcu)
+		return ERR_PTR(-ECHILD);
+
+	xattr_name = posix_acl_xattr_name(type);
+	if (unlikely(!strcmp(xattr_name, "")))
+		return ERR_PTR(-EINVAL);
+
+	size = ubifs_xattr_get(inode, xattr_name, NULL, 0);
+	if (size > 0) {
+		xattr_value = kzalloc(size, GFP_KERNEL);
+		if (unlikely(!xattr_value))
+			return ERR_PTR(-ENOMEM);
+
+		size = ubifs_xattr_get(inode, xattr_name, xattr_value, size);
+	}
+
+	if (size > 0)
+		acl = posix_acl_from_xattr(&init_user_ns, xattr_value, size);
+	else if (size == -ENODATA || size == 0)
+		acl = NULL;
+	else
+		acl = ERR_PTR(size);
+
+	kfree(xattr_value);
+
+	return acl;
+}
+
+static int __ubifs_set_acl(struct inode *inode, int type, struct posix_acl *acl, int flags)
+{
+	void *xattr_value = NULL;
+	const char *xattr_name;
+	size_t size = 0;
+	int error;
+
+	xattr_name = posix_acl_xattr_name(type);
+	if (unlikely(!strcmp(xattr_name, "")))
+		return -EINVAL;
+
+	if (unlikely(!strcmp(xattr_name, XATTR_NAME_POSIX_ACL_DEFAULT) && !S_ISDIR(inode->i_mode)))
+		return acl ? -EACCES : 0;
+
+	if (acl) {
+		size = posix_acl_xattr_size(acl->a_count);
+		xattr_value = kmalloc(size, GFP_KERNEL);
+		if (unlikely(!xattr_value))
+			return -ENOMEM;
+
+		error = posix_acl_to_xattr(&init_user_ns, acl, xattr_value, size);
+		if (unlikely(error < 0))
+			goto out;
+	}
+
+	error = ubifs_xattr_set(inode, xattr_name, xattr_value, size, flags, false);
+	if (likely(!error))
+		set_cached_acl(inode, type, acl);
+out:
+	kfree(xattr_value);
+	return error;
+}
+
+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 old_mode = inode->i_mode;
+	int error;
+
+	if (type == ACL_TYPE_ACCESS && acl) {
+		error = posix_acl_update_mode(idmap, inode, &inode->i_mode, &acl);
+		if (unlikely(error))
+			return error;
+	}
+
+	error = __ubifs_set_acl(inode, type, acl, 0);
+	if (unlikely(error))
+		inode->i_mode = old_mode;
+
+	return error;
+
+}
+
+/**
+ * ubifs_init_acl - initialize the ACLs for a new inode.
+ * @inode: newly created inode
+ * @dir: parent directory inode
+ *
+ * This function initialize ACLs, including inheriting the
+ * default ACLs of parent directory or modifying the default
+ * ACLs according to the mode parameter in open() / creat()
+ * system calls.
+ */
+int ubifs_init_acl(struct inode *inode, struct inode *dir)
+{
+	struct posix_acl *default_acl;
+	struct posix_acl *acl;
+	int error;
+
+	error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
+	if (unlikely(error))
+		return error;
+
+	if (default_acl) {
+		error = __ubifs_set_acl(inode, ACL_TYPE_DEFAULT, default_acl, XATTR_CREATE);
+		posix_acl_release(default_acl);
+	} else {
+		inode->i_default_acl = NULL;
+	}
+
+	if (acl) {
+		if (likely(!error))
+			error = __ubifs_set_acl(inode, ACL_TYPE_ACCESS, acl, XATTR_CREATE);
+		posix_acl_release(acl);
+	} else {
+		inode->i_acl = NULL;
+	}
+
+	return error;
+}
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 3916dc4f30ca..b0d3b076290d 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -2069,6 +2069,19 @@ static inline int ubifs_init_security(struct inode *dentry,
 }
 #endif
 
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+struct posix_acl *ubifs_get_inode_acl(struct inode *inode, int type, bool rcu);
+int ubifs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, struct posix_acl *acl, int type);
+int ubifs_init_acl(struct inode *inode, struct inode *dir);
+
+#else /* CONFIG_UBIFS_FS_POSIX_ACL */
+#define ubifs_get_inode_acl NULL
+#define ubifs_set_acl NULL
+static inline int ubifs_init_acl(struct inode *inode, struct inode *dir)
+{
+	return 0;
+}
+#endif /* CONFIG_UBIFS_FS_POSIX_ACL */
 
 /* super.c */
 struct inode *ubifs_iget(struct super_block *sb, unsigned long inum);
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 0847db521984..eb1c1f5d10df 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -40,7 +40,6 @@
  * in the VFS inode cache. The xentries are cached in the LNC cache (see
  * tnc.c).
  *
- * ACL support is not implemented.
  */
 
 #include "ubifs.h"
-- 
2.34.1

[RFC PATCH 2/5] ubifs: Initialize or update ACLs for inode

[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.

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;
+
 	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;
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;
 }
 
-- 
2.34.1

[RFC PATCH 3/5] ubifs: Support accessing ACLs through inode_operations

[Li Zetao]

Register the get/set interfaces to the inode operations whilch
allows access to the ACL through the vfs layer.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 fs/ubifs/dir.c  | 2 ++
 fs/ubifs/file.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index dfb6823cc953..59784349ba21 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -1724,6 +1724,8 @@ const struct inode_operations ubifs_dir_inode_operations = {
 	.setattr     = ubifs_setattr,
 	.getattr     = ubifs_getattr,
 	.listxattr   = ubifs_listxattr,
+	.get_inode_acl = ubifs_get_inode_acl,
+	.set_acl     = ubifs_set_acl,
 	.update_time = ubifs_update_time,
 	.tmpfile     = ubifs_tmpfile,
 	.fileattr_get = ubifs_fileattr_get,
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 8f964f8b0f96..80def8734b13 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1665,6 +1665,8 @@ const struct inode_operations ubifs_file_inode_operations = {
 	.setattr     = ubifs_setattr,
 	.getattr     = ubifs_getattr,
 	.listxattr   = ubifs_listxattr,
+	.get_inode_acl = ubifs_get_inode_acl,
+	.set_acl      = ubifs_set_acl,
 	.update_time = ubifs_update_time,
 	.fileattr_get = ubifs_fileattr_get,
 	.fileattr_set = ubifs_fileattr_set,
-- 
2.34.1

[RFC PATCH 4/5] ubifs: Introduce ACLs mount options

[Li Zetao]

Implement the ability to enable or disable the ACLs feature through
mount options. "-o acl" option means enable and "-o noacl" means disable
and it is enable by default.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 fs/ubifs/super.c | 40 ++++++++++++++++++++++++++++++++++++++++
 fs/ubifs/ubifs.h |  2 ++
 2 files changed, 42 insertions(+)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 7f4031a15f4d..ed03bf11e51d 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -457,6 +457,13 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root)
 	seq_printf(s, ",assert=%s", ubifs_assert_action_name(c));
 	seq_printf(s, ",ubi=%d,vol=%d", c->vi.ubi_num, c->vi.vol_id);
 
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+	if (c->mount_opts.acl == 2)
+		seq_puts(s, ",acl");
+	else if (c->mount_opts.acl == 1)
+		seq_puts(s, ",noacl");
+#endif
+
 	return 0;
 }
 
@@ -967,6 +974,8 @@ static int check_volume_empty(struct ubifs_info *c)
  * Opt_assert: set ubifs_assert() action
  * Opt_auth_key: The key name used for authentication
  * Opt_auth_hash_name: The hash type used for authentication
+ * Opt_acl: enable posix acl
+ * Opt_noacl: disable posix acl
  * Opt_err: just end of array marker
  */
 enum {
@@ -981,6 +990,8 @@ enum {
 	Opt_auth_key,
 	Opt_auth_hash_name,
 	Opt_ignore,
+	Opt_acl,
+	Opt_noacl,
 	Opt_err,
 };
 
@@ -997,6 +1008,8 @@ static const match_table_t tokens = {
 	{Opt_ignore, "ubi=%s"},
 	{Opt_ignore, "vol=%s"},
 	{Opt_assert, "assert=%s"},
+	{Opt_acl, "acl"},
+	{Opt_noacl, "noacl"},
 	{Opt_err, NULL},
 };
 
@@ -1137,6 +1150,23 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
 			break;
 		case Opt_ignore:
 			break;
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+		case Opt_acl:
+			c->mount_opts.acl = 2;
+			c->vfs_sb->s_flags |= SB_POSIXACL;
+			break;
+		case Opt_noacl:
+			c->mount_opts.acl = 1;
+			c->vfs_sb->s_flags &= ~SB_POSIXACL;
+			break;
+#else
+		case Opt_acl:
+			ubifs_err(c, "acl options not supported");
+			return -EINVAL;
+		case Opt_noacl:
+			ubifs_err(c, "noacl options not supported");
+			return -EINVAL;
+#endif
 		default:
 		{
 			unsigned long flag;
@@ -2011,12 +2041,17 @@ static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
 	sync_filesystem(sb);
 	dbg_gen("old flags %#lx, new flags %#x", sb->s_flags, *flags);
 
+	c->mount_opts.acl = 0;
 	err = ubifs_parse_options(c, data, 1);
 	if (err) {
 		ubifs_err(c, "invalid or unknown remount parameter");
 		return err;
 	}
 
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+	if (!c->mount_opts.acl)
+		c->vfs_sb->s_flags |= SB_POSIXACL;
+#endif
 	if (c->ro_mount && !(*flags & SB_RDONLY)) {
 		if (c->ro_error) {
 			ubifs_msg(c, "cannot re-mount R/W due to prior errors");
@@ -2197,6 +2232,11 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 	if (err)
 		goto out_close;
 
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+	if (!c->mount_opts.acl)
+		c->vfs_sb->s_flags |= SB_POSIXACL;
+#endif
+
 	/*
 	 * UBIFS provides 'backing_dev_info' in order to disable read-ahead. For
 	 * UBIFS, I/O is not deferred, it is done immediately in read_folio,
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index b0d3b076290d..4a6078cbb2f5 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -956,6 +956,7 @@ struct ubifs_orphan {
  *                  specified in @compr_type)
  * @compr_type: compressor type to override the superblock compressor with
  *              (%UBIFS_COMPR_NONE, etc)
+ * @acl: enable/disable posix acl (%0 default, %1 disable, %2 enable)
  */
 struct ubifs_mount_opts {
 	unsigned int unmount_mode:2;
@@ -963,6 +964,7 @@ struct ubifs_mount_opts {
 	unsigned int chk_data_crc:2;
 	unsigned int override_compr:1;
 	unsigned int compr_type:2;
+	unsigned int acl:2;
 };
 
 /**
-- 
2.34.1

[RFC PATCH 5/5] ubifs: Add ACLs config option

[Li Zetao]

Add CONFIG_UBIFS_FS_POSIX_ACL to select ACL for UBIFS, but it should
be noted that this config option depends on UBIFS_FS_XATTR.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 fs/ubifs/Kconfig  | 14 ++++++++++++++
 fs/ubifs/Makefile |  1 +
 2 files changed, 15 insertions(+)

diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
index 45d3d207fb99..9ac5ddd5ded3 100644
--- a/fs/ubifs/Kconfig
+++ b/fs/ubifs/Kconfig
@@ -98,4 +98,18 @@ config UBIFS_FS_AUTHENTICATION
 	  sha256, these are not selected automatically since there are many
 	  different options.
 
+config UBIFS_FS_POSIX_ACL
+	bool "UBIFS POSIX Access Control Lists"
+	depends on UBIFS_FS_XATTR
+	select FS_POSIX_ACL
+	default y
+	help
+	  Posix Access Control Lists (ACLs) support permissions for users and
+	  groups beyond the owner/group/world scheme.
+
+	  To learn more about Access Control Lists, visit the Posix ACLs for
+	  Linux website <http://acl.bestbits.at/>.
+
+	  If you don't know what Access Control Lists are, say N
+
 endif # UBIFS_FS
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
index 314c80b24a76..1e0733a647d5 100644
--- a/fs/ubifs/Makefile
+++ b/fs/ubifs/Makefile
@@ -9,3 +9,4 @@ ubifs-y += misc.o sysfs.o
 ubifs-$(CONFIG_FS_ENCRYPTION) += crypto.o
 ubifs-$(CONFIG_UBIFS_FS_XATTR) += xattr.o
 ubifs-$(CONFIG_UBIFS_FS_AUTHENTICATION) += auth.o
+ubifs-$(CONFIG_UBIFS_FS_POSIX_ACL) += acl.o
\ No newline at end of file
-- 
2.34.1

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

[Zhihao Cheng]

在 2024/3/20 0:16, 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.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>   fs/ubifs/acl.c   | 140 +++++++++++++++++++++++++++++++++++++++++++++++
>   fs/ubifs/ubifs.h |  13 +++++
>   fs/ubifs/xattr.c |   1 -
>   3 files changed, 153 insertions(+), 1 deletion(-)
>   create mode 100644 fs/ubifs/acl.c
> 
> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
> new file mode 100644
> index 000000000000..253568baf097
> --- /dev/null
> +++ b/fs/ubifs/acl.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * This file is part of UBIFS.
> + *
> + * Copyright (C) 2024 Huawei Tech. Co., Ltd.
> + *
> + * Authors: Li Zetao <lizetao1@huawei.com>
> + */
> +
> +/* This file implements POSIX Access Control Lists (ACLs) */
> +
> +#include "ubifs.h"
> +
> +#include <linux/posix_acl_xattr.h>
> +
> +struct posix_acl *ubifs_get_inode_acl(struct inode *inode, int type, bool rcu)
> +{
> +	char *xattr_value = NULL;
> +	const char *xattr_name;
> +	struct posix_acl *acl;
> +	ssize_t size;
> +
> +	if (rcu)
> +		return ERR_PTR(-ECHILD);
> +
> +	xattr_name = posix_acl_xattr_name(type);
> +	if (unlikely(!strcmp(xattr_name, "")))
> +		return ERR_PTR(-EINVAL);
The acl type has been guaranteed valid from vfs caller, there is no need 
to check converted name by 'strcmp', in theory, we can use it directly 
just like f2fs does. For this case, I suggest to unfold the 
posix_acl_xattr_name and convert it to corresponding name just like 
btrfs does.
> +
> +	size = ubifs_xattr_get(inode, xattr_name, NULL, 0);
> +	if (size > 0) {
> +		xattr_value = kzalloc(size, GFP_KERNEL);
> +		if (unlikely(!xattr_value))
> +			return ERR_PTR(-ENOMEM);
> +
> +		size = ubifs_xattr_get(inode, xattr_name, xattr_value, size);
> +	}
> +
> +	if (size > 0)
> +		acl = posix_acl_from_xattr(&init_user_ns, xattr_value, size);
> +	else if (size == -ENODATA || size == 0)
> +		acl = NULL;
> +	else
> +		acl = ERR_PTR(size);
> +
> +	kfree(xattr_value);
> +
> +	return acl;
> +}
> +
> +static int __ubifs_set_acl(struct inode *inode, int type, struct posix_acl *acl, int flags)
> +{
> +	void *xattr_value = NULL;
> +	const char *xattr_name;
> +	size_t size = 0;
> +	int error;
> +

> +	xattr_name = posix_acl_xattr_name(type);
> +	if (unlikely(!strcmp(xattr_name, "")))
> +		return -EINVAL;
> +
> +	if (unlikely(!strcmp(xattr_name, XATTR_NAME_POSIX_ACL_DEFAULT) && !S_ISDIR(inode->i_mode)))
> +		return acl ? -EACCES : 0;
> +
Similar to previous, replace above 6 lines, refer to __btrfs_set_acl but 
keep the error code same with __ext4_set_acl.
> +	if (acl) {
> +		size = posix_acl_xattr_size(acl->a_count);
> +		xattr_value = kmalloc(size, GFP_KERNEL);
> +		if (unlikely(!xattr_value))
> +			return -ENOMEM;
> +
> +		error = posix_acl_to_xattr(&init_user_ns, acl, xattr_value, size);
> +		if (unlikely(error < 0))
> +			goto out;
> +	}
> +
> +	error = ubifs_xattr_set(inode, xattr_name, xattr_value, size, flags, false);
There are 2 situations here, Updating acl and Removing acl. For the 
later case, funcion vfs_remove_acl will remove corresponding xattr, the 
xattr removing function in ubifs is ubifs_xattr_remove.
> +	if (likely(!error))
I prefer to remove the 'likely', UBIFS limits the max xattr count for 
each file(Goto create_xattr), non zero error returned is a common case 
on a small LEB flash.
> +		set_cached_acl(inode, type, acl);
> +out:
> +	kfree(xattr_value);
> +	return error;
> +}
> +
> +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 old_mode = inode->i_mode;
> +	int error;
> +
> +	if (type == ACL_TYPE_ACCESS && acl) {
> +		error = posix_acl_update_mode(idmap, inode, &inode->i_mode, &acl);
> +		if (unlikely(error))
> +			return error;
> +	}
> +
> +	error = __ubifs_set_acl(inode, type, acl, 0);
> +	if (unlikely(error))
Mentioned in __ubifs_set_acl, error could be returned, just remove 
'unlikely'.
> +		inode->i_mode = old_mode;
> +
> +	return error;
> +
> +}
> +
> +/**
> + * ubifs_init_acl - initialize the ACLs for a new inode.
> + * @inode: newly created inode
> + * @dir: parent directory inode
> + *
> + * This function initialize ACLs, including inheriting the
initialize -> initializes
> + * default ACLs of parent directory or modifying the default
> + * ACLs according to the mode parameter in open() / creat()
> + * system calls.
> + */
> +int ubifs_init_acl(struct inode *inode, struct inode *dir)
> +{
> +	struct posix_acl *default_acl;
> +	struct posix_acl *acl;
> +	int error;
> +
> +	error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> +	if (unlikely(error))
> +		return error;
> +
> +	if (default_acl) {
> +		error = __ubifs_set_acl(inode, ACL_TYPE_DEFAULT, default_acl, XATTR_CREATE);
> +		posix_acl_release(default_acl);
> +	} else {
> +		inode->i_default_acl = NULL;
> +	}
> +
> +	if (acl) {
> +		if (likely(!error))
Remove 'likely'.
> +			error = __ubifs_set_acl(inode, ACL_TYPE_ACCESS, acl, XATTR_CREATE);
> +		posix_acl_release(acl);
> +	} else {
> +		inode->i_acl = NULL;
> +	}
> +
> +	return error;
> +}
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 3916dc4f30ca..b0d3b076290d 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -2069,6 +2069,19 @@ static inline int ubifs_init_security(struct inode *dentry,
>   }
>   #endif
>   
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +struct posix_acl *ubifs_get_inode_acl(struct inode *inode, int type, bool rcu);
> +int ubifs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, struct posix_acl *acl, int type);
> +int ubifs_init_acl(struct inode *inode, struct inode *dir);
> +
> +#else /* CONFIG_UBIFS_FS_POSIX_ACL */
> +#define ubifs_get_inode_acl NULL
> +#define ubifs_set_acl NULL
> +static inline int ubifs_init_acl(struct inode *inode, struct inode *dir)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_UBIFS_FS_POSIX_ACL */
>   
>   /* super.c */
>   struct inode *ubifs_iget(struct super_block *sb, unsigned long inum);
> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
> index 0847db521984..eb1c1f5d10df 100644
> --- a/fs/ubifs/xattr.c
> +++ b/fs/ubifs/xattr.c
> @@ -40,7 +40,6 @@
>    * in the VFS inode cache. The xentries are cached in the LNC cache (see
>    * tnc.c).
>    *
> - * ACL support is not implemented.
>    */
>   
>   #include "ubifs.h"
> 

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

[Zhihao Cheng]

在 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;
>   }
>   
> 

Re: [RFC PATCH 4/5] ubifs: Introduce ACLs mount options

[Zhihao Cheng]

在 2024/3/20 0:16, Li Zetao 写道:
> Implement the ability to enable or disable the ACLs feature through
> mount options. "-o acl" option means enable and "-o noacl" means disable
> and it is enable by default.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>   fs/ubifs/super.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   fs/ubifs/ubifs.h |  2 ++
>   2 files changed, 42 insertions(+)
> 
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 7f4031a15f4d..ed03bf11e51d 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -457,6 +457,13 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root)
>   	seq_printf(s, ",assert=%s", ubifs_assert_action_name(c));
>   	seq_printf(s, ",ubi=%d,vol=%d", c->vi.ubi_num, c->vi.vol_id);
>   
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL

This config is introduced in pacth 5, we cannot use it in patch 4.
> +	if (c->mount_opts.acl == 2)
> +		seq_puts(s, ",acl");
> +	else if (c->mount_opts.acl == 1)
> +		seq_puts(s, ",noacl");
> +#endif
> +
>   	return 0;
>   }
>   
> @@ -967,6 +974,8 @@ static int check_volume_empty(struct ubifs_info *c)
>    * Opt_assert: set ubifs_assert() action
>    * Opt_auth_key: The key name used for authentication
>    * Opt_auth_hash_name: The hash type used for authentication
> + * Opt_acl: enable posix acl
> + * Opt_noacl: disable posix acl
>    * Opt_err: just end of array marker
>    */
>   enum {
> @@ -981,6 +990,8 @@ enum {
>   	Opt_auth_key,
>   	Opt_auth_hash_name,
>   	Opt_ignore,
> +	Opt_acl,
> +	Opt_noacl,
It would be better to update Documentation/filesystems/ubifs.rst to 
describe new mount options
>   	Opt_err,
>   };
>   
> @@ -997,6 +1008,8 @@ static const match_table_t tokens = {
>   	{Opt_ignore, "ubi=%s"},
>   	{Opt_ignore, "vol=%s"},
>   	{Opt_assert, "assert=%s"},
> +	{Opt_acl, "acl"},
> +	{Opt_noacl, "noacl"},
>   	{Opt_err, NULL},
>   };
>   
> @@ -1137,6 +1150,23 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
>   			break;
>   		case Opt_ignore:
>   			break;
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +		case Opt_acl:
> +			c->mount_opts.acl = 2;
> +			c->vfs_sb->s_flags |= SB_POSIXACL;
> +			break;
> +		case Opt_noacl:
> +			c->mount_opts.acl = 1;
> +			c->vfs_sb->s_flags &= ~SB_POSIXACL;
> +			break;
> +#else
> +		case Opt_acl:
> +			ubifs_err(c, "acl options not supported");
> +			return -EINVAL;
> +		case Opt_noacl:
> +			ubifs_err(c, "noacl options not supported");
> +			return -EINVAL;
> +#endif
>   		default:
>   		{
>   			unsigned long flag;
> @@ -2011,12 +2041,17 @@ static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
>   	sync_filesystem(sb);
>   	dbg_gen("old flags %#lx, new flags %#x", sb->s_flags, *flags);
>   
> +	c->mount_opts.acl = 0;
>   	err = ubifs_parse_options(c, data, 1);

1. mount -onoacl /dev/ubi0_0 /mnt # After that, mount will show 'noacl' 
option
2. mount -oremount,xxx /dev/ubi0_0 /mnt
If 'xxx' has nothing to do with acl, c->mount_opts.acl is set as '0'. 
Then superblock flag is assigned with 'SB_POSIXACL' and mount will not 
display 'nocal'. Will it make user confused?
>   	if (err) {
>   		ubifs_err(c, "invalid or unknown remount parameter");
>   		return err;
>   	}
>   
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +	if (!c->mount_opts.acl)
> +		c->vfs_sb->s_flags |= SB_POSIXACL;
> +#endif
>   	if (c->ro_mount && !(*flags & SB_RDONLY)) {
>   		if (c->ro_error) {
>   			ubifs_msg(c, "cannot re-mount R/W due to prior errors");
> @@ -2197,6 +2232,11 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
>   	if (err)
>   		goto out_close;
>   
> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> +	if (!c->mount_opts.acl)
> +		c->vfs_sb->s_flags |= SB_POSIXACL;
> +#endif
> +
>   	/*
>   	 * UBIFS provides 'backing_dev_info' in order to disable read-ahead. For
>   	 * UBIFS, I/O is not deferred, it is done immediately in read_folio,
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index b0d3b076290d..4a6078cbb2f5 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -956,6 +956,7 @@ struct ubifs_orphan {
>    *                  specified in @compr_type)
>    * @compr_type: compressor type to override the superblock compressor with
>    *              (%UBIFS_COMPR_NONE, etc)
> + * @acl: enable/disable posix acl (%0 default, %1 disable, %2 enable)
>    */
>   struct ubifs_mount_opts {
>   	unsigned int unmount_mode:2;
> @@ -963,6 +964,7 @@ struct ubifs_mount_opts {
>   	unsigned int chk_data_crc:2;
>   	unsigned int override_compr:1;
>   	unsigned int compr_type:2;
> +	unsigned int acl:2;
>   };
>   
>   /**
> 

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

[Li Zetao]

Hi,

On 2024/3/21 10:55, Zhihao Cheng wrote:
> 在 2024/3/20 0:16, 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.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>>   fs/ubifs/acl.c   | 140 +++++++++++++++++++++++++++++++++++++++++++++++
>>   fs/ubifs/ubifs.h |  13 +++++
>>   fs/ubifs/xattr.c |   1 -
>>   3 files changed, 153 insertions(+), 1 deletion(-)
>>   create mode 100644 fs/ubifs/acl.c
>>
>> diff --git a/fs/ubifs/acl.c b/fs/ubifs/acl.c
>> new file mode 100644
>> index 000000000000..253568baf097
>> --- /dev/null
>> +++ b/fs/ubifs/acl.c
>> @@ -0,0 +1,140 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * This file is part of UBIFS.
>> + *
>> + * Copyright (C) 2024 Huawei Tech. Co., Ltd.
>> + *
>> + * Authors: Li Zetao <lizetao1@huawei.com>
>> + */
>> +
>> +/* This file implements POSIX Access Control Lists (ACLs) */
>> +
>> +#include "ubifs.h"
>> +
>> +#include <linux/posix_acl_xattr.h>
>> +
>> +struct posix_acl *ubifs_get_inode_acl(struct inode *inode, int type, 
>> bool rcu)
>> +{
>> +    char *xattr_value = NULL;
>> +    const char *xattr_name;
>> +    struct posix_acl *acl;
>> +    ssize_t size;
>> +
>> +    if (rcu)
>> +        return ERR_PTR(-ECHILD);
>> +
>> +    xattr_name = posix_acl_xattr_name(type);
>> +    if (unlikely(!strcmp(xattr_name, "")))
>> +        return ERR_PTR(-EINVAL);
> The acl type has been guaranteed valid from vfs caller, there is no need 
> to check converted name by 'strcmp', in theory, we can use it directly 
> just like f2fs does. For this case, I suggest to unfold the 
> posix_acl_xattr_name and convert it to corresponding name just like 
> btrfs does.
Ok, I will modify it in the next version.
>> +
>> +    size = ubifs_xattr_get(inode, xattr_name, NULL, 0);
>> +    if (size > 0) {
>> +        xattr_value = kzalloc(size, GFP_KERNEL);
>> +        if (unlikely(!xattr_value))
>> +            return ERR_PTR(-ENOMEM);
>> +
>> +        size = ubifs_xattr_get(inode, xattr_name, xattr_value, size);
>> +    }
>> +
>> +    if (size > 0)
>> +        acl = posix_acl_from_xattr(&init_user_ns, xattr_value, size);
>> +    else if (size == -ENODATA || size == 0)
>> +        acl = NULL;
>> +    else
>> +        acl = ERR_PTR(size);
>> +
>> +    kfree(xattr_value);
>> +
>> +    return acl;
>> +}
>> +
>> +static int __ubifs_set_acl(struct inode *inode, int type, struct 
>> posix_acl *acl, int flags)
>> +{
>> +    void *xattr_value = NULL;
>> +    const char *xattr_name;
>> +    size_t size = 0;
>> +    int error;
>> +
> 
>> +    xattr_name = posix_acl_xattr_name(type);
>> +    if (unlikely(!strcmp(xattr_name, "")))
>> +        return -EINVAL;
>> +
>> +    if (unlikely(!strcmp(xattr_name, XATTR_NAME_POSIX_ACL_DEFAULT) && 
>> !S_ISDIR(inode->i_mode)))
>> +        return acl ? -EACCES : 0;
>> +
> Similar to previous, replace above 6 lines, refer to __btrfs_set_acl but 
> keep the error code same with __ext4_set_acl.
Ok.
>> +    if (acl) {
>> +        size = posix_acl_xattr_size(acl->a_count);
>> +        xattr_value = kmalloc(size, GFP_KERNEL);
>> +        if (unlikely(!xattr_value))
>> +            return -ENOMEM;
>> +
>> +        error = posix_acl_to_xattr(&init_user_ns, acl, xattr_value, 
>> size);
>> +        if (unlikely(error < 0))
>> +            goto out;
>> +    }
>> +
>> +    error = ubifs_xattr_set(inode, xattr_name, xattr_value, size, 
>> flags, false);
> There are 2 situations here, Updating acl and Removing acl. For the 
> later case, funcion vfs_remove_acl will remove corresponding xattr, the 
> xattr removing function in ubifs is ubifs_xattr_remove.
Thanks. Indeed, ubifs_xattr_set() is is different from other file 
systems, it will not delete xattr when value is NULL.
>> +    if (likely(!error))
> I prefer to remove the 'likely', UBIFS limits the max xattr count for 
> each file(Goto create_xattr), non zero error returned is a common case 
> on a small LEB flash.
Ok.
>> +        set_cached_acl(inode, type, acl);
>> +out:
>> +    kfree(xattr_value);
>> +    return error;
>> +}
>> +
>> +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 old_mode = inode->i_mode;
>> +    int error;
>> +
>> +    if (type == ACL_TYPE_ACCESS && acl) {
>> +        error = posix_acl_update_mode(idmap, inode, &inode->i_mode, 
>> &acl);
>> +        if (unlikely(error))
>> +            return error;
>> +    }
>> +
>> +    error = __ubifs_set_acl(inode, type, acl, 0);
>> +    if (unlikely(error))
> Mentioned in __ubifs_set_acl, error could be returned, just remove 
> 'unlikely'.
Ok.
>> +        inode->i_mode = old_mode;
>> +
>> +    return error;
>> +
>> +}
>> +
>> +/**
>> + * ubifs_init_acl - initialize the ACLs for a new inode.
>> + * @inode: newly created inode
>> + * @dir: parent directory inode
>> + *
>> + * This function initialize ACLs, including inheriting the
> initialize -> initializes
Ok.
>> + * default ACLs of parent directory or modifying the default
>> + * ACLs according to the mode parameter in open() / creat()
>> + * system calls.
>> + */
>> +int ubifs_init_acl(struct inode *inode, struct inode *dir)
>> +{
>> +    struct posix_acl *default_acl;
>> +    struct posix_acl *acl;
>> +    int error;
>> +
>> +    error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
>> +    if (unlikely(error))
>> +        return error;
>> +
>> +    if (default_acl) {
>> +        error = __ubifs_set_acl(inode, ACL_TYPE_DEFAULT, default_acl, 
>> XATTR_CREATE);
>> +        posix_acl_release(default_acl);
>> +    } else {
>> +        inode->i_default_acl = NULL;
>> +    }
>> +
>> +    if (acl) {
>> +        if (likely(!error))
> Remove 'likely'.
Ok.
>> +            error = __ubifs_set_acl(inode, ACL_TYPE_ACCESS, acl, 
>> XATTR_CREATE);
>> +        posix_acl_release(acl);
>> +    } else {
>> +        inode->i_acl = NULL;
>> +    }
>> +
>> +    return error;
>> +}
>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>> index 3916dc4f30ca..b0d3b076290d 100644
>> --- a/fs/ubifs/ubifs.h
>> +++ b/fs/ubifs/ubifs.h
>> @@ -2069,6 +2069,19 @@ static inline int ubifs_init_security(struct 
>> inode *dentry,
>>   }
>>   #endif
>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>> +struct posix_acl *ubifs_get_inode_acl(struct inode *inode, int type, 
>> bool rcu);
>> +int ubifs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, 
>> struct posix_acl *acl, int type);
>> +int ubifs_init_acl(struct inode *inode, struct inode *dir);
>> +
>> +#else /* CONFIG_UBIFS_FS_POSIX_ACL */
>> +#define ubifs_get_inode_acl NULL
>> +#define ubifs_set_acl NULL
>> +static inline int ubifs_init_acl(struct inode *inode, struct inode *dir)
>> +{
>> +    return 0;
>> +}
>> +#endif /* CONFIG_UBIFS_FS_POSIX_ACL */
>>   /* super.c */
>>   struct inode *ubifs_iget(struct super_block *sb, unsigned long inum);
>> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
>> index 0847db521984..eb1c1f5d10df 100644
>> --- a/fs/ubifs/xattr.c
>> +++ b/fs/ubifs/xattr.c
>> @@ -40,7 +40,6 @@
>>    * in the VFS inode cache. The xentries are cached in the LNC cache 
>> (see
>>    * tnc.c).
>>    *
>> - * ACL support is not implemented.
>>    */
>>   #include "ubifs.h"
>>
> 

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

[Li Zetao]

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;
>>   }
>>
> 

Re: [RFC PATCH 4/5] ubifs: Introduce ACLs mount options

[Li Zetao]

Hi,

On 2024/3/21 14:49, Zhihao Cheng wrote:
> 在 2024/3/20 0:16, Li Zetao 写道:
>> Implement the ability to enable or disable the ACLs feature through
>> mount options. "-o acl" option means enable and "-o noacl" means disable
>> and it is enable by default.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>>   fs/ubifs/super.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   fs/ubifs/ubifs.h |  2 ++
>>   2 files changed, 42 insertions(+)
>>
>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>> index 7f4031a15f4d..ed03bf11e51d 100644
>> --- a/fs/ubifs/super.c
>> +++ b/fs/ubifs/super.c
>> @@ -457,6 +457,13 @@ static int ubifs_show_options(struct seq_file *s, 
>> struct dentry *root)
>>       seq_printf(s, ",assert=%s", ubifs_assert_action_name(c));
>>       seq_printf(s, ",ubi=%d,vol=%d", c->vi.ubi_num, c->vi.vol_id);
>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
> 
> This config is introduced in pacth 5, we cannot use it in patch 4.
>> +    if (c->mount_opts.acl == 2)
>> +        seq_puts(s, ",acl");
>> +    else if (c->mount_opts.acl == 1)
>> +        seq_puts(s, ",noacl");
>> +#endif
>> +
>>       return 0;
>>   }
>> @@ -967,6 +974,8 @@ static int check_volume_empty(struct ubifs_info *c)
>>    * Opt_assert: set ubifs_assert() action
>>    * Opt_auth_key: The key name used for authentication
>>    * Opt_auth_hash_name: The hash type used for authentication
>> + * Opt_acl: enable posix acl
>> + * Opt_noacl: disable posix acl
>>    * Opt_err: just end of array marker
>>    */
>>   enum {
>> @@ -981,6 +990,8 @@ enum {
>>       Opt_auth_key,
>>       Opt_auth_hash_name,
>>       Opt_ignore,
>> +    Opt_acl,
>> +    Opt_noacl,
> It would be better to update Documentation/filesystems/ubifs.rst to 
> describe new mount options
Ok.
>>       Opt_err,
>>   };
>> @@ -997,6 +1008,8 @@ static const match_table_t tokens = {
>>       {Opt_ignore, "ubi=%s"},
>>       {Opt_ignore, "vol=%s"},
>>       {Opt_assert, "assert=%s"},
>> +    {Opt_acl, "acl"},
>> +    {Opt_noacl, "noacl"},
>>       {Opt_err, NULL},
>>   };
>> @@ -1137,6 +1150,23 @@ static int ubifs_parse_options(struct 
>> ubifs_info *c, char *options,
>>               break;
>>           case Opt_ignore:
>>               break;
>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>> +        case Opt_acl:
>> +            c->mount_opts.acl = 2;
>> +            c->vfs_sb->s_flags |= SB_POSIXACL;
>> +            break;
>> +        case Opt_noacl:
>> +            c->mount_opts.acl = 1;
>> +            c->vfs_sb->s_flags &= ~SB_POSIXACL;
>> +            break;
>> +#else
>> +        case Opt_acl:
>> +            ubifs_err(c, "acl options not supported");
>> +            return -EINVAL;
>> +        case Opt_noacl:
>> +            ubifs_err(c, "noacl options not supported");
>> +            return -EINVAL;
>> +#endif
>>           default:
>>           {
>>               unsigned long flag;
>> @@ -2011,12 +2041,17 @@ static int ubifs_remount_fs(struct super_block 
>> *sb, int *flags, char *data)
>>       sync_filesystem(sb);
>>       dbg_gen("old flags %#lx, new flags %#x", sb->s_flags, *flags);
>> +    c->mount_opts.acl = 0;
>>       err = ubifs_parse_options(c, data, 1);
> 
> 1. mount -onoacl /dev/ubi0_0 /mnt # After that, mount will show 'noacl' 
> option
> 2. mount -oremount,xxx /dev/ubi0_0 /mnt
> If 'xxx' has nothing to do with acl, c->mount_opts.acl is set as '0'. 
> Then superblock flag is assigned with 'SB_POSIXACL' and mount will not 
> display 'nocal'. Will it make user confused?
I have tested this use case and it works fine. This is because the mount 
options will be re-parsed during remount, just like the last mount. But 
this is indeed redundant. I adjusted it to make it more reasonable.
>>       if (err) {
>>           ubifs_err(c, "invalid or unknown remount parameter");
>>           return err;
>>       }
>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>> +    if (!c->mount_opts.acl)
>> +        c->vfs_sb->s_flags |= SB_POSIXACL;
>> +#endif
>>       if (c->ro_mount && !(*flags & SB_RDONLY)) {
>>           if (c->ro_error) {
>>               ubifs_msg(c, "cannot re-mount R/W due to prior errors");
>> @@ -2197,6 +2232,11 @@ static int ubifs_fill_super(struct super_block 
>> *sb, void *data, int silent)
>>       if (err)
>>           goto out_close;
>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL
>> +    if (!c->mount_opts.acl)
>> +        c->vfs_sb->s_flags |= SB_POSIXACL;
>> +#endif
>> +
>>       /*
>>        * UBIFS provides 'backing_dev_info' in order to disable 
>> read-ahead. For
>>        * UBIFS, I/O is not deferred, it is done immediately in 
>> read_folio,
>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>> index b0d3b076290d..4a6078cbb2f5 100644
>> --- a/fs/ubifs/ubifs.h
>> +++ b/fs/ubifs/ubifs.h
>> @@ -956,6 +956,7 @@ struct ubifs_orphan {
>>    *                  specified in @compr_type)
>>    * @compr_type: compressor type to override the superblock 
>> compressor with
>>    *              (%UBIFS_COMPR_NONE, etc)
>> + * @acl: enable/disable posix acl (%0 default, %1 disable, %2 enable)
>>    */
>>   struct ubifs_mount_opts {
>>       unsigned int unmount_mode:2;
>> @@ -963,6 +964,7 @@ struct ubifs_mount_opts {
>>       unsigned int chk_data_crc:2;
>>       unsigned int override_compr:1;
>>       unsigned int compr_type:2;
>> +    unsigned int acl:2;
>>   };
>>   /**
>>
> 

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

[Zhihao Cheng]

在 2024/3/22 19:57, Li Zetao 写道:
> 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.

You are right, only bcachefs support acl for symlink.

Re: [RFC PATCH 4/5] ubifs: Introduce ACLs mount options

[Zhihao Cheng]

在 2024/3/22 20:05, Li Zetao 写道:
> Hi,
> 
> On 2024/3/21 14:49, Zhihao Cheng wrote:
>> 在 2024/3/20 0:16, Li Zetao 写道:
>>> Implement the ability to enable or disable the ACLs feature through
>>> mount options. "-o acl" option means enable and "-o noacl" means disable
>>> and it is enable by default.
>>>
>>> Signed-off-by: Li Zetao <lizetao1@huawei.com>

[...]
>>> @@ -2011,12 +2041,17 @@ static int ubifs_remount_fs(struct 
>>> super_block *sb, int *flags, char *data)
>>>       sync_filesystem(sb);
>>>       dbg_gen("old flags %#lx, new flags %#x", sb->s_flags, *flags);
>>> +    c->mount_opts.acl = 0;
>>>       err = ubifs_parse_options(c, data, 1);
>>
>> 1. mount -onoacl /dev/ubi0_0 /mnt # After that, mount will show 
>> 'noacl' option
>> 2. mount -oremount,xxx /dev/ubi0_0 /mnt
>> If 'xxx' has nothing to do with acl, c->mount_opts.acl is set as '0'. 
>> Then superblock flag is assigned with 'SB_POSIXACL' and mount will not 
>> display 'nocal'. Will it make user confused?
> I have tested this use case and it works fine. This is because the mount 
> options will be re-parsed during remount, just like the last mount. But 
> this is indeed redundant. I adjusted it to make it more reasonable.

Yes, parameter 'data' carries old mount information. Removing the 
redundant assignment is fine.