[PATCH v1 0/5] Landlock support for UML

[PATCH v1 0/5] Landlock support for UML

[Mickaël Salaün]

Hi,

Commit cb2c7d1a1776 ("landlock: Support filesystem access-control")
introduced a new ARCH_EPHEMERAL_INODES configuration, only enabled for
User-Mode Linux.  The reason was that UML's hostfs managed inodes in an
ephemeral way: from the kernel point of view, the same inode struct
could be created several times while being used by user space because
the kernel didn't hold references to inodes.  Because Landlock (and
probably other subsystems) ties properties (i.e. access rights) to inode
objects, it wasn't possible to create rules that match inodes and then
allow specific accesses.

This patch series fixes the way UML manages inodes according to the
underlying filesystem.  They are now properly handles as for other
filesystems, which enables to support Landlock (and probably other
features).

Backporting these patches requires some selftest harness patches
backports too.

Regards,

Mickaël Salaün (5):
  hostfs: Fix ephemeral inodes
  selftests/landlock: Don't create useless file layouts
  selftests/landlock: Add supports_filesystem() helper
  selftests/landlock: Make mounts configurable
  selftests/landlock: Add tests for pseudo filesystems

 arch/Kconfig                               |   7 -
 arch/um/Kconfig                            |   1 -
 fs/hostfs/hostfs.h                         |   1 +
 fs/hostfs/hostfs_kern.c                    | 213 ++++++------
 fs/hostfs/hostfs_user.c                    |   1 +
 security/landlock/Kconfig                  |   2 +-
 tools/testing/selftests/landlock/config    |   8 +-
 tools/testing/selftests/landlock/fs_test.c | 381 +++++++++++++++++++--
 8 files changed, 472 insertions(+), 142 deletions(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
-- 
2.39.2

[PATCH v1 1/5] hostfs: Fix ephemeral inodes

[Mickaël Salaün]

hostfs creates a new inode for each opened or created file, which created
useless inode allocations and forbade identifying a host file with a kernel
inode.

Fix this uncommon filesystem behavior by tying kernel inodes to host
file's inode and device IDs.  Even if the host filesystem inodes may be
recycled, this cannot happen while a file referencing it is open, which
is the case with hostfs.  It should be noted that hostfs inode IDs may
not be unique for the same hostfs superblock because multiple host's
(backed) superblocks may be used.

Delete inodes when dropping them to force backed host's file descriptors
closing.

This enables to entirely remove ARCH_EPHEMERAL_INODES, and then makes
Landlock fully supported by UML.  This is very useful for testing
(ongoing and backported) changes.

These changes also factor out and simplify some helpers thanks to the
new hostfs_inode_update() and the hostfs_iget() revamp: read_name(),
hostfs_create(), hostfs_lookup(), hostfs_mknod(), and
hostfs_fill_sb_common().

A following commit with new Landlock tests check this new hostfs inode
consistency.

Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Richard Weinberger <richard@nod.at>
Cc: <stable@vger.kernel.org> # 5.15.x: ce72750f04d6: hostfs: Fix writeback of dirty pages
Cc: <stable@vger.kernel.org> # 5.15+
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20230309165455.175131-2-mic@digikod.net
---
 arch/Kconfig              |   7 --
 arch/um/Kconfig           |   1 -
 fs/hostfs/hostfs.h        |   1 +
 fs/hostfs/hostfs_kern.c   | 213 +++++++++++++++++++-------------------
 fs/hostfs/hostfs_user.c   |   1 +
 security/landlock/Kconfig |   2 +-
 6 files changed, 109 insertions(+), 116 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index e3511afbb7f2..d5f0841ac3c1 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1156,13 +1156,6 @@ config COMPAT_32BIT_TIME
 config ARCH_NO_PREEMPT
 	bool
 
-config ARCH_EPHEMERAL_INODES
-	def_bool n
-	help
-	  An arch should select this symbol if it doesn't keep track of inode
-	  instances on its own, but instead relies on something else (e.g. the
-	  host kernel for an UML kernel).
-
 config ARCH_SUPPORTS_RT
 	bool
 
diff --git a/arch/um/Kconfig b/arch/um/Kconfig
index 541a9b18e343..4057d5267c6a 100644
--- a/arch/um/Kconfig
+++ b/arch/um/Kconfig
@@ -5,7 +5,6 @@ menu "UML-specific options"
 config UML
 	bool
 	default y
-	select ARCH_EPHEMERAL_INODES
 	select ARCH_HAS_FORTIFY_SOURCE
 	select ARCH_HAS_GCOV_PROFILE_ALL
 	select ARCH_HAS_KCOV
diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
index 69cb796f6270..0239e3af3945 100644
--- a/fs/hostfs/hostfs.h
+++ b/fs/hostfs/hostfs.h
@@ -65,6 +65,7 @@ struct hostfs_stat {
 	unsigned long long blocks;
 	unsigned int maj;
 	unsigned int min;
+	dev_t dev;
 };
 
 extern int stat_file(const char *path, struct hostfs_stat *p, int fd);
diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 28b4f15c19eb..19496f732016 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -26,6 +26,7 @@ struct hostfs_inode_info {
 	fmode_t mode;
 	struct inode vfs_inode;
 	struct mutex open_mutex;
+	dev_t dev;
 };
 
 static inline struct hostfs_inode_info *HOSTFS_I(struct inode *inode)
@@ -182,14 +183,6 @@ static char *follow_link(char *link)
 	return ERR_PTR(n);
 }
 
-static struct inode *hostfs_iget(struct super_block *sb)
-{
-	struct inode *inode = new_inode(sb);
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
-	return inode;
-}
-
 static int hostfs_statfs(struct dentry *dentry, struct kstatfs *sf)
 {
 	/*
@@ -228,6 +221,7 @@ static struct inode *hostfs_alloc_inode(struct super_block *sb)
 		return NULL;
 	hi->fd = -1;
 	hi->mode = 0;
+	hi->dev = 0;
 	inode_init_once(&hi->vfs_inode);
 	mutex_init(&hi->open_mutex);
 	return &hi->vfs_inode;
@@ -240,6 +234,7 @@ static void hostfs_evict_inode(struct inode *inode)
 	if (HOSTFS_I(inode)->fd != -1) {
 		close_file(&HOSTFS_I(inode)->fd);
 		HOSTFS_I(inode)->fd = -1;
+		HOSTFS_I(inode)->dev = 0;
 	}
 }
 
@@ -265,6 +260,7 @@ static int hostfs_show_options(struct seq_file *seq, struct dentry *root)
 static const struct super_operations hostfs_sbops = {
 	.alloc_inode	= hostfs_alloc_inode,
 	.free_inode	= hostfs_free_inode,
+	.drop_inode	= generic_delete_inode,
 	.evict_inode	= hostfs_evict_inode,
 	.statfs		= hostfs_statfs,
 	.show_options	= hostfs_show_options,
@@ -512,18 +508,31 @@ static const struct address_space_operations hostfs_aops = {
 	.write_end	= hostfs_write_end,
 };
 
-static int read_name(struct inode *ino, char *name)
+static int hostfs_inode_update(struct inode *ino, const struct hostfs_stat *st)
+{
+	set_nlink(ino, st->nlink);
+	i_uid_write(ino, st->uid);
+	i_gid_write(ino, st->gid);
+	ino->i_atime =
+		(struct timespec64){ st->atime.tv_sec, st->atime.tv_nsec };
+	ino->i_mtime =
+		(struct timespec64){ st->mtime.tv_sec, st->mtime.tv_nsec };
+	ino->i_ctime =
+		(struct timespec64){ st->ctime.tv_sec, st->ctime.tv_nsec };
+	ino->i_size = st->size;
+	ino->i_blocks = st->blocks;
+	return 0;
+}
+
+static int hostfs_inode_set(struct inode *ino, void *data)
 {
+	struct hostfs_stat *st = data;
 	dev_t rdev;
-	struct hostfs_stat st;
-	int err = stat_file(name, &st, -1);
-	if (err)
-		return err;
 
 	/* Reencode maj and min with the kernel encoding.*/
-	rdev = MKDEV(st.maj, st.min);
+	rdev = MKDEV(st->maj, st->min);
 
-	switch (st.mode & S_IFMT) {
+	switch (st->mode & S_IFMT) {
 	case S_IFLNK:
 		ino->i_op = &hostfs_link_iops;
 		break;
@@ -535,7 +544,7 @@ static int read_name(struct inode *ino, char *name)
 	case S_IFBLK:
 	case S_IFIFO:
 	case S_IFSOCK:
-		init_special_inode(ino, st.mode & S_IFMT, rdev);
+		init_special_inode(ino, st->mode & S_IFMT, rdev);
 		ino->i_op = &hostfs_iops;
 		break;
 	case S_IFREG:
@@ -547,17 +556,42 @@ static int read_name(struct inode *ino, char *name)
 		return -EIO;
 	}
 
-	ino->i_ino = st.ino;
-	ino->i_mode = st.mode;
-	set_nlink(ino, st.nlink);
-	i_uid_write(ino, st.uid);
-	i_gid_write(ino, st.gid);
-	ino->i_atime = (struct timespec64){ st.atime.tv_sec, st.atime.tv_nsec };
-	ino->i_mtime = (struct timespec64){ st.mtime.tv_sec, st.mtime.tv_nsec };
-	ino->i_ctime = (struct timespec64){ st.ctime.tv_sec, st.ctime.tv_nsec };
-	ino->i_size = st.size;
-	ino->i_blocks = st.blocks;
-	return 0;
+	HOSTFS_I(ino)->dev = st->dev;
+	ino->i_ino = st->ino;
+	ino->i_mode = st->mode;
+	return hostfs_inode_update(ino, st);
+}
+
+static int hostfs_inode_test(struct inode *inode, void *data)
+{
+	const struct hostfs_stat *st = data;
+
+	return inode->i_ino == st->ino && HOSTFS_I(inode)->dev == st->dev;
+}
+
+static struct inode *hostfs_iget(struct super_block *sb, char *name)
+{
+	struct inode *inode;
+	struct hostfs_stat st;
+	int err = stat_file(name, &st, -1);
+
+	if (err)
+		return ERR_PTR(err);
+
+	inode = iget5_locked(sb, st.ino, hostfs_inode_test, hostfs_inode_set,
+			     &st);
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+
+	if (inode->i_state & I_NEW) {
+		unlock_new_inode(inode);
+	} else {
+		spin_lock(&inode->i_lock);
+		hostfs_inode_update(inode, &st);
+		spin_unlock(&inode->i_lock);
+	}
+
+	return inode;
 }
 
 static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
@@ -565,62 +599,48 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
 {
 	struct inode *inode;
 	char *name;
-	int error, fd;
-
-	inode = hostfs_iget(dir->i_sb);
-	if (IS_ERR(inode)) {
-		error = PTR_ERR(inode);
-		goto out;
-	}
+	int fd;
 
-	error = -ENOMEM;
 	name = dentry_name(dentry);
 	if (name == NULL)
-		goto out_put;
+		return -ENOMEM;
 
 	fd = file_create(name, mode & 0777);
-	if (fd < 0)
-		error = fd;
-	else
-		error = read_name(inode, name);
+	if (fd < 0) {
+		__putname(name);
+		return fd;
+	}
 
+	inode = hostfs_iget(dir->i_sb, name);
 	__putname(name);
-	if (error)
-		goto out_put;
+	if (IS_ERR(inode))
+		return PTR_ERR(inode);
 
 	HOSTFS_I(inode)->fd = fd;
 	HOSTFS_I(inode)->mode = FMODE_READ | FMODE_WRITE;
 	d_instantiate(dentry, inode);
 	return 0;
-
- out_put:
-	iput(inode);
- out:
-	return error;
 }
 
 static struct dentry *hostfs_lookup(struct inode *ino, struct dentry *dentry,
 				    unsigned int flags)
 {
-	struct inode *inode;
+	struct inode *inode = NULL;
 	char *name;
-	int err;
-
-	inode = hostfs_iget(ino->i_sb);
-	if (IS_ERR(inode))
-		goto out;
 
-	err = -ENOMEM;
 	name = dentry_name(dentry);
-	if (name) {
-		err = read_name(inode, name);
-		__putname(name);
-	}
-	if (err) {
-		iput(inode);
-		inode = (err == -ENOENT) ? NULL : ERR_PTR(err);
+	if (name == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	inode = hostfs_iget(ino->i_sb, name);
+	__putname(name);
+	if (IS_ERR(inode)) {
+		if (PTR_ERR(inode) == -ENOENT)
+			inode = NULL;
+		else
+			return ERR_CAST(inode);
 	}
- out:
+
 	return d_splice_alias(inode, dentry);
 }
 
@@ -704,35 +724,23 @@ static int hostfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
 	char *name;
 	int err;
 
-	inode = hostfs_iget(dir->i_sb);
-	if (IS_ERR(inode)) {
-		err = PTR_ERR(inode);
-		goto out;
-	}
-
-	err = -ENOMEM;
 	name = dentry_name(dentry);
 	if (name == NULL)
-		goto out_put;
+		return -ENOMEM;
 
 	err = do_mknod(name, mode, MAJOR(dev), MINOR(dev));
-	if (err)
-		goto out_free;
+	if (err) {
+		__putname(name);
+		return err;
+	}
 
-	err = read_name(inode, name);
+	inode = hostfs_iget(dir->i_sb, name);
 	__putname(name);
-	if (err)
-		goto out_put;
+	if (IS_ERR(inode))
+		return PTR_ERR(inode);
 
 	d_instantiate(dentry, inode);
 	return 0;
-
- out_free:
-	__putname(name);
- out_put:
-	iput(inode);
- out:
-	return err;
 }
 
 static int hostfs_rename2(struct mnt_idmap *idmap,
@@ -929,49 +937,40 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 	err = super_setup_bdi(sb);
 	if (err)
-		goto out;
+		return err;
 
 	/* NULL is printed as '(null)' by printf(): avoid that. */
 	if (req_root == NULL)
 		req_root = "";
 
-	err = -ENOMEM;
 	sb->s_fs_info = host_root_path =
 		kasprintf(GFP_KERNEL, "%s/%s", root_ino, req_root);
 	if (host_root_path == NULL)
-		goto out;
-
-	root_inode = new_inode(sb);
-	if (!root_inode)
-		goto out;
+		return -ENOMEM;
 
-	err = read_name(root_inode, host_root_path);
-	if (err)
-		goto out_put;
+	root_inode = hostfs_iget(sb, host_root_path);
+	if (IS_ERR(root_inode))
+		return PTR_ERR(root_inode);
 
 	if (S_ISLNK(root_inode->i_mode)) {
-		char *name = follow_link(host_root_path);
-		if (IS_ERR(name)) {
-			err = PTR_ERR(name);
-			goto out_put;
-		}
-		err = read_name(root_inode, name);
+		char *name;
+
+		iput(root_inode);
+		name = follow_link(host_root_path);
+		if (IS_ERR(name))
+			return PTR_ERR(name);
+
+		root_inode = hostfs_iget(sb, name);
 		kfree(name);
-		if (err)
-			goto out_put;
+		if (IS_ERR(root_inode))
+			return PTR_ERR(root_inode);
 	}
 
-	err = -ENOMEM;
 	sb->s_root = d_make_root(root_inode);
 	if (sb->s_root == NULL)
-		goto out;
+		return -ENOMEM;
 
 	return 0;
-
-out_put:
-	iput(root_inode);
-out:
-	return err;
 }
 
 static struct dentry *hostfs_read_sb(struct file_system_type *type,
diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
index 5ecc4706172b..840619e39a1a 100644
--- a/fs/hostfs/hostfs_user.c
+++ b/fs/hostfs/hostfs_user.c
@@ -36,6 +36,7 @@ static void stat64_to_hostfs(const struct stat64 *buf, struct hostfs_stat *p)
 	p->blocks = buf->st_blocks;
 	p->maj = os_major(buf->st_rdev);
 	p->min = os_minor(buf->st_rdev);
+	p->dev = buf->st_dev;
 }
 
 int stat_file(const char *path, struct hostfs_stat *p, int fd)
diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
index 8e33c4e8ffb8..c1e862a38410 100644
--- a/security/landlock/Kconfig
+++ b/security/landlock/Kconfig
@@ -2,7 +2,7 @@
 
 config SECURITY_LANDLOCK
 	bool "Landlock support"
-	depends on SECURITY && !ARCH_EPHEMERAL_INODES
+	depends on SECURITY
 	select SECURITY_PATH
 	help
 	  Landlock is a sandboxing mechanism that enables processes to restrict
-- 
2.39.2

[PATCH v1 2/5] selftests/landlock: Don't create useless file layouts

[Mickaël Salaün]

Add and use a layout0 test fixture to not populate the tmpfs filesystem
if it is not required for tests: unknown_access_rights, proc_nsfs,
unpriv and max_layers.

This doesn't change these tests but it speeds up their setup and makes
them less prone to error.  This prepare the ground for a next commit.

Cc: <stable@vger.kernel.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20230309165455.175131-3-mic@digikod.net
---
 tools/testing/selftests/landlock/fs_test.c | 26 +++++++++++++++++-----
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index b6c4be3faf7a..0438651f61d2 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -231,6 +231,20 @@ static void cleanup_layout(struct __test_metadata *const _metadata)
 	EXPECT_EQ(0, remove_path(TMP_DIR));
 }
 
+/* clang-format off */
+FIXTURE(layout0) {};
+/* clang-format on */
+
+FIXTURE_SETUP(layout0)
+{
+	prepare_layout(_metadata);
+}
+
+FIXTURE_TEARDOWN(layout0)
+{
+	cleanup_layout(_metadata);
+}
+
 static void create_layout1(struct __test_metadata *const _metadata)
 {
 	create_file(_metadata, file1_s1d1);
@@ -510,7 +524,7 @@ TEST_F_FORK(layout1, file_and_dir_access_rights)
 	ASSERT_EQ(0, close(ruleset_fd));
 }
 
-TEST_F_FORK(layout1, unknown_access_rights)
+TEST_F_FORK(layout0, unknown_access_rights)
 {
 	__u64 access_mask;
 
@@ -608,7 +622,7 @@ static void enforce_ruleset(struct __test_metadata *const _metadata,
 	}
 }
 
-TEST_F_FORK(layout1, proc_nsfs)
+TEST_F_FORK(layout0, proc_nsfs)
 {
 	const struct rule rules[] = {
 		{
@@ -657,11 +671,11 @@ TEST_F_FORK(layout1, proc_nsfs)
 	ASSERT_EQ(0, close(path_beneath.parent_fd));
 }
 
-TEST_F_FORK(layout1, unpriv)
+TEST_F_FORK(layout0, unpriv)
 {
 	const struct rule rules[] = {
 		{
-			.path = dir_s1d2,
+			.path = TMP_DIR,
 			.access = ACCESS_RO,
 		},
 		{},
@@ -1301,12 +1315,12 @@ TEST_F_FORK(layout1, inherit_superset)
 	ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
 }
 
-TEST_F_FORK(layout1, max_layers)
+TEST_F_FORK(layout0, max_layers)
 {
 	int i, err;
 	const struct rule rules[] = {
 		{
-			.path = dir_s1d2,
+			.path = TMP_DIR,
 			.access = ACCESS_RO,
 		},
 		{},
-- 
2.39.2

[PATCH v1 3/5] selftests/landlock: Add supports_filesystem() helper

[Mickaël Salaün]

Replace supports_overlayfs() with supports_filesystem() to be able to
check several filesystems.  This will be useful in a following commit.

Only check for overlay filesystem once in the setup step, and then rely
on self->skip_test.

Cc: Guenter Roeck <groeck@chromium.org>
Cc: Jeff Xu <jeffxu@google.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20230309165455.175131-4-mic@digikod.net
---
 tools/testing/selftests/landlock/fs_test.c | 36 ++++++++++++++--------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 0438651f61d2..c1e655fc06bb 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -107,8 +107,10 @@ static bool fgrep(FILE *const inf, const char *const str)
 	return false;
 }
 
-static bool supports_overlayfs(void)
+static bool supports_filesystem(const char *const filesystem)
 {
+	char str[32];
+	int len;
 	bool res;
 	FILE *const inf = fopen("/proc/filesystems", "r");
 
@@ -119,7 +121,12 @@ static bool supports_overlayfs(void)
 	if (!inf)
 		return true;
 
-	res = fgrep(inf, "nodev\toverlay\n");
+	len = snprintf(str, sizeof(str), "nodev\t%s\n", filesystem);
+	if (len >= sizeof(str))
+		/* Ignores too-long filesystem names. */
+		return true;
+
+	res = fgrep(inf, str);
 	fclose(inf);
 	return res;
 }
@@ -4044,14 +4051,17 @@ static const char (*merge_sub_files[])[] = {
  *         └── work
  */
 
-/* clang-format off */
-FIXTURE(layout2_overlay) {};
-/* clang-format on */
+FIXTURE(layout2_overlay)
+{
+	bool skip_test;
+};
 
 FIXTURE_SETUP(layout2_overlay)
 {
-	if (!supports_overlayfs())
-		SKIP(return, "overlayfs is not supported");
+	if (!supports_filesystem("overlay")) {
+		self->skip_test = true;
+		SKIP(return, "overlayfs is not supported (setup)");
+	}
 
 	prepare_layout(_metadata);
 
@@ -4089,8 +4099,8 @@ FIXTURE_SETUP(layout2_overlay)
 
 FIXTURE_TEARDOWN(layout2_overlay)
 {
-	if (!supports_overlayfs())
-		SKIP(return, "overlayfs is not supported");
+	if (self->skip_test)
+		SKIP(return, "overlayfs is not supported (teardown)");
 
 	EXPECT_EQ(0, remove_path(lower_do1_fl3));
 	EXPECT_EQ(0, remove_path(lower_dl1_fl2));
@@ -4123,8 +4133,8 @@ FIXTURE_TEARDOWN(layout2_overlay)
 
 TEST_F_FORK(layout2_overlay, no_restriction)
 {
-	if (!supports_overlayfs())
-		SKIP(return, "overlayfs is not supported");
+	if (self->skip_test)
+		SKIP(return, "overlayfs is not supported (test)");
 
 	ASSERT_EQ(0, test_open(lower_fl1, O_RDONLY));
 	ASSERT_EQ(0, test_open(lower_dl1, O_RDONLY));
@@ -4289,8 +4299,8 @@ TEST_F_FORK(layout2_overlay, same_content_different_file)
 	size_t i;
 	const char *path_entry;
 
-	if (!supports_overlayfs())
-		SKIP(return, "overlayfs is not supported");
+	if (self->skip_test)
+		SKIP(return, "overlayfs is not supported (test)");
 
 	/* Sets rules on base directories (i.e. outside overlay scope). */
 	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer1_base);
-- 
2.39.2

[PATCH v1 4/5] selftests/landlock: Make mounts configurable

[Mickaël Salaün]

Add a new struct mnt_opt to define a mount point with the mount_opt()
helper.  This doesn't change tests but prepare for the next commit.

Cc: <stable@vger.kernel.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20230309165455.175131-5-mic@digikod.net
---
 tools/testing/selftests/landlock/fs_test.c | 34 ++++++++++++++++++----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index c1e655fc06bb..de29c8c9e194 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -213,7 +213,26 @@ static int remove_path(const char *const path)
 	return err;
 }
 
-static void prepare_layout(struct __test_metadata *const _metadata)
+struct mnt_opt {
+	const char *const source;
+	const char *const type;
+	const unsigned long flags;
+	const char *const data;
+};
+
+const struct mnt_opt mnt_tmp = {
+	.type = "tmpfs",
+	.data = "size=4m,mode=700",
+};
+
+static int mount_opt(const struct mnt_opt *const mnt, const char *const target)
+{
+	return mount(mnt->source ?: mnt->type, target, mnt->type, mnt->flags,
+		     mnt->data);
+}
+
+static void prepare_layout_opt(struct __test_metadata *const _metadata,
+			       const struct mnt_opt *const mnt)
 {
 	disable_caps(_metadata);
 	umask(0077);
@@ -225,11 +244,16 @@ static void prepare_layout(struct __test_metadata *const _metadata)
 	 */
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	ASSERT_EQ(0, unshare(CLONE_NEWNS));
-	ASSERT_EQ(0, mount("tmp", TMP_DIR, "tmpfs", 0, "size=4m,mode=700"));
+	ASSERT_EQ(0, mount_opt(mnt, TMP_DIR));
 	ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC, NULL));
 	clear_cap(_metadata, CAP_SYS_ADMIN);
 }
 
+static void prepare_layout(struct __test_metadata *const _metadata)
+{
+	prepare_layout_opt(_metadata, &mnt_tmp);
+}
+
 static void cleanup_layout(struct __test_metadata *const _metadata)
 {
 	set_cap(_metadata, CAP_SYS_ADMIN);
@@ -269,7 +293,7 @@ static void create_layout1(struct __test_metadata *const _metadata)
 	create_file(_metadata, file1_s3d1);
 	create_directory(_metadata, dir_s3d2);
 	set_cap(_metadata, CAP_SYS_ADMIN);
-	ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
+	ASSERT_EQ(0, mount_opt(&mnt_tmp, dir_s3d2));
 	clear_cap(_metadata, CAP_SYS_ADMIN);
 
 	ASSERT_EQ(0, mkdir(dir_s3d3, 0700));
@@ -4068,7 +4092,7 @@ FIXTURE_SETUP(layout2_overlay)
 	create_directory(_metadata, LOWER_BASE);
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	/* Creates tmpfs mount points to get deterministic overlayfs. */
-	ASSERT_EQ(0, mount("tmp", LOWER_BASE, "tmpfs", 0, "size=4m,mode=700"));
+	ASSERT_EQ(0, mount_opt(&mnt_tmp, LOWER_BASE));
 	clear_cap(_metadata, CAP_SYS_ADMIN);
 	create_file(_metadata, lower_fl1);
 	create_file(_metadata, lower_dl1_fl2);
@@ -4078,7 +4102,7 @@ FIXTURE_SETUP(layout2_overlay)
 
 	create_directory(_metadata, UPPER_BASE);
 	set_cap(_metadata, CAP_SYS_ADMIN);
-	ASSERT_EQ(0, mount("tmp", UPPER_BASE, "tmpfs", 0, "size=4m,mode=700"));
+	ASSERT_EQ(0, mount_opt(&mnt_tmp, UPPER_BASE));
 	clear_cap(_metadata, CAP_SYS_ADMIN);
 	create_file(_metadata, upper_fu1);
 	create_file(_metadata, upper_du1_fu2);
-- 
2.39.2

[PATCH v1 5/5] selftests/landlock: Add tests for pseudo filesystems

[Mickaël Salaün]

Add generic and read-only tests for 7 pseudo filesystems to make sure
they have a consistent inode management, which is required for
Landlock's file hierarchy identification:
- ramfs
- tmpfs
- cgroup
- cgroup2
- proc
- sysfs
- hostfs

Update related kernel configuration to support these new filesystems,
and sort all entries.  If these filesystems are not supported by the
kernel running tests, the related tests are skipped.

Expanding variants, this adds 35 new tests for layout3_fs:
- tag_inode_dir_parent
- tag_inode_dir_mnt
- tag_inode_dir_child
- tag_inode_dir_file
- release_inodes

The hostfs filesystem, only available for an User-Mode Linux kernel, is
special because we cannot explicitly mount it.  The layout3_fs.hostfs
variant tests are skipped if the current test directory is not backed by
this filesystem.

The layout3_fs.hostfs.tag_inode_dir_child and
layout3_fs.hostfs.tag_inode_file tests pass thanks to a previous
commit fixing hostfs inode management.  Without this fix, the
deny-by-default policy would apply and all access requests would be
denied.

Cc: <stable@vger.kernel.org> # 5.15.x: 63e6b2a42342: selftests/harness: Run TEARDOWN for ASSERT failures
Cc: <stable@vger.kernel.org> # 5.15.x: 79ee8aa31d51: selftests/harness: Pass variant to teardown
Cc: <stable@vger.kernel.org> # 5.15+
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20230309165455.175131-6-mic@digikod.net
---
 tools/testing/selftests/landlock/config    |   8 +-
 tools/testing/selftests/landlock/fs_test.c | 285 +++++++++++++++++++++
 2 files changed, 291 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
index 0f0a65287bac..5501108d00b7 100644
--- a/tools/testing/selftests/landlock/config
+++ b/tools/testing/selftests/landlock/config
@@ -1,7 +1,11 @@
+CONFIG_CGROUPS=y
+CONFIG_CGROUP_SCHED=y
 CONFIG_OVERLAY_FS=y
+CONFIG_PROC_FS=y
+CONFIG_SECURITY=y
 CONFIG_SECURITY_LANDLOCK=y
 CONFIG_SECURITY_PATH=y
-CONFIG_SECURITY=y
 CONFIG_SHMEM=y
-CONFIG_TMPFS_XATTR=y
+CONFIG_SYSFS=y
 CONFIG_TMPFS=y
+CONFIG_TMPFS_XATTR=y
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index de29c8c9e194..f6a80ad738e1 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -10,6 +10,7 @@
 #define _GNU_SOURCE
 #include <fcntl.h>
 #include <linux/landlock.h>
+#include <linux/magic.h>
 #include <sched.h>
 #include <stdio.h>
 #include <string.h>
@@ -19,6 +20,7 @@
 #include <sys/sendfile.h>
 #include <sys/stat.h>
 #include <sys/sysmacros.h>
+#include <sys/vfs.h>
 #include <unistd.h>
 
 #include "common.h"
@@ -121,6 +123,10 @@ static bool supports_filesystem(const char *const filesystem)
 	if (!inf)
 		return true;
 
+	/* filesystem can be null for bind mounts. */
+	if (!filesystem)
+		return true;
+
 	len = snprintf(str, sizeof(str), "nodev\t%s\n", filesystem);
 	if (len >= sizeof(str))
 		/* Ignores too-long filesystem names. */
@@ -131,6 +137,19 @@ static bool supports_filesystem(const char *const filesystem)
 	return res;
 }
 
+static bool cwd_matches_fs(unsigned int fs_magic)
+{
+	struct statfs statfs_buf;
+
+	if (!fs_magic)
+		return true;
+
+	if (statfs(".", &statfs_buf))
+		return true;
+
+	return statfs_buf.f_type == fs_magic;
+}
+
 static void mkdir_parents(struct __test_metadata *const _metadata,
 			  const char *const path)
 {
@@ -307,11 +326,13 @@ static void remove_layout1(struct __test_metadata *const _metadata)
 	EXPECT_EQ(0, remove_path(file1_s1d3));
 	EXPECT_EQ(0, remove_path(file1_s1d2));
 	EXPECT_EQ(0, remove_path(file1_s1d1));
+	EXPECT_EQ(0, remove_path(dir_s1d3));
 
 	EXPECT_EQ(0, remove_path(file2_s2d3));
 	EXPECT_EQ(0, remove_path(file1_s2d3));
 	EXPECT_EQ(0, remove_path(file1_s2d2));
 	EXPECT_EQ(0, remove_path(file1_s2d1));
+	EXPECT_EQ(0, remove_path(dir_s2d2));
 
 	EXPECT_EQ(0, remove_path(file1_s3d1));
 	EXPECT_EQ(0, remove_path(dir_s3d3));
@@ -4471,4 +4492,268 @@ TEST_F_FORK(layout2_overlay, same_content_different_file)
 	}
 }
 
+FIXTURE(layout3_fs)
+{
+	bool has_created_dir;
+	bool has_created_file;
+	char *dir_path;
+	bool skip_test;
+};
+
+FIXTURE_VARIANT(layout3_fs)
+{
+	const struct mnt_opt mnt;
+	const char *const file_path;
+	unsigned int cwd_fs_magic;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout3_fs, tmpfs) {
+	/* clang-format on */
+	.mnt = mnt_tmp,
+	.file_path = file1_s1d1,
+};
+
+FIXTURE_VARIANT_ADD(layout3_fs, ramfs) {
+	.mnt = {
+		.type = "ramfs",
+		.data = "mode=700",
+	},
+	.file_path = TMP_DIR "/dir/file",
+};
+
+FIXTURE_VARIANT_ADD(layout3_fs, cgroup) {
+	.mnt = {
+		.type = "cgroup",
+	},
+	.file_path = TMP_DIR "/test/tasks",
+};
+
+FIXTURE_VARIANT_ADD(layout3_fs, cgroup2) {
+	.mnt = {
+		.type = "cgroup2",
+	},
+	.file_path = TMP_DIR "/test/cgroup.procs",
+};
+
+FIXTURE_VARIANT_ADD(layout3_fs, proc) {
+	.mnt = {
+		.type = "proc",
+	},
+	.file_path = TMP_DIR "/self/status",
+};
+
+FIXTURE_VARIANT_ADD(layout3_fs, sysfs) {
+	.mnt = {
+		.type = "sysfs",
+	},
+	.file_path = TMP_DIR "/kernel/notes",
+};
+
+FIXTURE_VARIANT_ADD(layout3_fs, hostfs) {
+	.mnt = {
+		.source = TMP_DIR,
+		.flags = MS_BIND,
+	},
+	.file_path = TMP_DIR "/dir/file",
+	.cwd_fs_magic = HOSTFS_SUPER_MAGIC,
+};
+
+FIXTURE_SETUP(layout3_fs)
+{
+	struct stat statbuf;
+	const char *slash;
+	size_t dir_len;
+
+	if (!supports_filesystem(variant->mnt.type) ||
+	    !cwd_matches_fs(variant->cwd_fs_magic)) {
+		self->skip_test = true;
+		SKIP(return, "this filesystem is not supported (setup)");
+	}
+
+	slash = strrchr(variant->file_path, '/');
+	ASSERT_NE(slash, NULL);
+	dir_len = (size_t)slash - (size_t)variant->file_path;
+	ASSERT_LT(0, dir_len);
+	self->dir_path = malloc(dir_len + 1);
+	self->dir_path[dir_len] = '\0';
+	strncpy(self->dir_path, variant->file_path, dir_len);
+
+	prepare_layout_opt(_metadata, &variant->mnt);
+
+	/* Creates directory when required. */
+	if (stat(self->dir_path, &statbuf)) {
+		set_cap(_metadata, CAP_DAC_OVERRIDE);
+		EXPECT_EQ(0, mkdir(self->dir_path, 0700))
+		{
+			TH_LOG("Failed to create directory \"%s\": %d %s",
+			       self->dir_path, errno, strerror(errno));
+			free(self->dir_path);
+			self->dir_path = NULL;
+		}
+		self->has_created_dir = true;
+		clear_cap(_metadata, CAP_DAC_OVERRIDE);
+	}
+
+	/* Creates file when required. */
+	if (stat(variant->file_path, &statbuf)) {
+		int fd;
+
+		set_cap(_metadata, CAP_DAC_OVERRIDE);
+		fd = creat(variant->file_path, 0600);
+		EXPECT_LE(0, fd)
+		{
+			TH_LOG("Failed to create file \"%s\": %d %s",
+			       variant->file_path, errno, strerror(errno));
+		}
+		EXPECT_EQ(0, close(fd));
+		self->has_created_file = true;
+		clear_cap(_metadata, CAP_DAC_OVERRIDE);
+	}
+}
+
+FIXTURE_TEARDOWN(layout3_fs)
+{
+	if (self->skip_test)
+		SKIP(return, "this filesystem is not supported (teardown)");
+
+	if (self->has_created_file) {
+		set_cap(_metadata, CAP_DAC_OVERRIDE);
+		/*
+		 * Don't check for error because the file might already
+		 * have been removed (cf. release_inode test).
+		 */
+		unlink(variant->file_path);
+		clear_cap(_metadata, CAP_DAC_OVERRIDE);
+	}
+
+	if (self->has_created_dir) {
+		set_cap(_metadata, CAP_DAC_OVERRIDE);
+		/*
+		 * Don't check for error because the directory might already
+		 * have been removed (cf. release_inode test).
+		 */
+		rmdir(self->dir_path);
+		clear_cap(_metadata, CAP_DAC_OVERRIDE);
+	}
+	free(self->dir_path);
+	self->dir_path = NULL;
+
+	cleanup_layout(_metadata);
+}
+
+static void layer3_fs_tag_inode(struct __test_metadata *const _metadata,
+				FIXTURE_DATA(layout3_fs) * self,
+				const FIXTURE_VARIANT(layout3_fs) * variant,
+				const char *const rule_path)
+{
+	const struct rule layer1_allow_read_file[] = {
+		{
+			.path = rule_path,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{},
+	};
+	const struct landlock_ruleset_attr layer2_deny_everything_attr = {
+		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
+	};
+	const char *const dev_null_path = "/dev/null";
+	int ruleset_fd;
+
+	if (self->skip_test)
+		SKIP(return, "this filesystem is not supported (test)");
+
+	/* Checks without Landlock. */
+	EXPECT_EQ(0, test_open(dev_null_path, O_RDONLY | O_CLOEXEC));
+	EXPECT_EQ(0, test_open(variant->file_path, O_RDONLY | O_CLOEXEC));
+
+	ruleset_fd = create_ruleset(_metadata, LANDLOCK_ACCESS_FS_READ_FILE,
+				    layer1_allow_read_file);
+	EXPECT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	EXPECT_EQ(EACCES, test_open(dev_null_path, O_RDONLY | O_CLOEXEC));
+	EXPECT_EQ(0, test_open(variant->file_path, O_RDONLY | O_CLOEXEC));
+
+	/* Forbids directory reading. */
+	ruleset_fd =
+		landlock_create_ruleset(&layer2_deny_everything_attr,
+					sizeof(layer2_deny_everything_attr), 0);
+	EXPECT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	/* Checks with Landlock and forbidden access. */
+	EXPECT_EQ(EACCES, test_open(dev_null_path, O_RDONLY | O_CLOEXEC));
+	EXPECT_EQ(EACCES, test_open(variant->file_path, O_RDONLY | O_CLOEXEC));
+}
+
+/* Matrix of tests to check file hierarchy evaluation. */
+
+TEST_F_FORK(layout3_fs, tag_inode_dir_parent)
+{
+	/* The current directory must not be the root for this test. */
+	layer3_fs_tag_inode(_metadata, self, variant, ".");
+}
+
+TEST_F_FORK(layout3_fs, tag_inode_dir_mnt)
+{
+	layer3_fs_tag_inode(_metadata, self, variant, TMP_DIR);
+}
+
+TEST_F_FORK(layout3_fs, tag_inode_dir_child)
+{
+	layer3_fs_tag_inode(_metadata, self, variant, self->dir_path);
+}
+
+TEST_F_FORK(layout3_fs, tag_inode_file)
+{
+	layer3_fs_tag_inode(_metadata, self, variant, variant->file_path);
+}
+
+/* Light version of layout1.release_inodes */
+TEST_F_FORK(layout3_fs, release_inodes)
+{
+	const struct rule layer1[] = {
+		{
+			.path = TMP_DIR,
+			.access = LANDLOCK_ACCESS_FS_READ_DIR,
+		},
+		{},
+	};
+	int ruleset_fd;
+
+	if (self->skip_test)
+		SKIP(return, "this filesystem is not supported (test)");
+
+	/* Clean up for the teardown to not fail. */
+	if (self->has_created_file)
+		EXPECT_EQ(0, remove_path(variant->file_path));
+
+	if (self->has_created_dir)
+		/* Don't check for error because of cgroup specificities. */
+		remove_path(self->dir_path);
+
+	ruleset_fd =
+		create_ruleset(_metadata, LANDLOCK_ACCESS_FS_READ_DIR, layer1);
+	ASSERT_LE(0, ruleset_fd);
+
+	/* Unmount the filesystem while it is being used by a ruleset. */
+	set_cap(_metadata, CAP_SYS_ADMIN);
+	ASSERT_EQ(0, umount(TMP_DIR));
+	clear_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* Replaces with a new mount point to simplify FIXTURE_TEARDOWN. */
+	set_cap(_metadata, CAP_SYS_ADMIN);
+	ASSERT_EQ(0, mount("tmp", TMP_DIR, "tmpfs", 0, "size=4m,mode=700"));
+	clear_cap(_metadata, CAP_SYS_ADMIN);
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* Checks that access to the new mount point is denied. */
+	ASSERT_EQ(EACCES, test_open(TMP_DIR, O_RDONLY));
+}
+
 TEST_HARNESS_MAIN
-- 
2.39.2

Re: [PATCH v1 0/5] Landlock support for UML

[Mickaël Salaün]

Richard, Anton, Johannes, what do you think about these UML changes?


On 09/03/2023 17:54, Mickaël Salaün wrote:
> Hi,
> 
> Commit cb2c7d1a1776 ("landlock: Support filesystem access-control")
> introduced a new ARCH_EPHEMERAL_INODES configuration, only enabled for
> User-Mode Linux.  The reason was that UML's hostfs managed inodes in an
> ephemeral way: from the kernel point of view, the same inode struct
> could be created several times while being used by user space because
> the kernel didn't hold references to inodes.  Because Landlock (and
> probably other subsystems) ties properties (i.e. access rights) to inode
> objects, it wasn't possible to create rules that match inodes and then
> allow specific accesses.
> 
> This patch series fixes the way UML manages inodes according to the
> underlying filesystem.  They are now properly handles as for other
> filesystems, which enables to support Landlock (and probably other
> features).
> 
> Backporting these patches requires some selftest harness patches
> backports too.
> 
> Regards,
> 
> Mickaël Salaün (5):
>    hostfs: Fix ephemeral inodes
>    selftests/landlock: Don't create useless file layouts
>    selftests/landlock: Add supports_filesystem() helper
>    selftests/landlock: Make mounts configurable
>    selftests/landlock: Add tests for pseudo filesystems
> 
>   arch/Kconfig                               |   7 -
>   arch/um/Kconfig                            |   1 -
>   fs/hostfs/hostfs.h                         |   1 +
>   fs/hostfs/hostfs_kern.c                    | 213 ++++++------
>   fs/hostfs/hostfs_user.c                    |   1 +
>   security/landlock/Kconfig                  |   2 +-
>   tools/testing/selftests/landlock/config    |   8 +-
>   tools/testing/selftests/landlock/fs_test.c | 381 +++++++++++++++++++--
>   8 files changed, 472 insertions(+), 142 deletions(-)
> 
> 
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Re: [PATCH v1 0/5] Landlock support for UML

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Mickaël Salaün" <mic@digikod.net>
> Richard, Anton, Johannes, what do you think about these UML changes?

I like them but didn't had a chance for a deeper look so far. :-S

Thanks,
//richard

Re: [PATCH v1 0/5] Landlock support for UML

[Mickaël Salaün]

On 21/03/2023 22:38, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Mickaël Salaün" <mic@digikod.net>
>> Richard, Anton, Johannes, what do you think about these UML changes?
> 
> I like them but didn't had a chance for a deeper look so far. :-S

Good! Do you think it could make it for v6.4?  Should we push it in 
-next for testing?

Thanks,
  Mickaël

Re: [PATCH v1 0/5] Landlock support for UML

[Mickaël Salaün]

Hi Richard, any news?

On 04/04/2023 15:52, Mickaël Salaün wrote:
> 
> On 21/03/2023 22:38, Richard Weinberger wrote:
>> ----- Ursprüngliche Mail -----
>>> Von: "Mickaël Salaün" <mic@digikod.net>
>>> Richard, Anton, Johannes, what do you think about these UML changes?
>>
>> I like them but didn't had a chance for a deeper look so far. :-S
> 
> Good! Do you think it could make it for v6.4?  Should we push it in
> -next for testing?
> 
> Thanks,
>    Mickaël

Re: [PATCH v1 1/5] hostfs: Fix ephemeral inodes

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Mickaël Salaün" <mic@digikod.net>
> hostfs creates a new inode for each opened or created file, which created
> useless inode allocations and forbade identifying a host file with a kernel
> inode.
> 
> Fix this uncommon filesystem behavior by tying kernel inodes to host
> file's inode and device IDs.  Even if the host filesystem inodes may be
> recycled, this cannot happen while a file referencing it is open, which
> is the case with hostfs.  It should be noted that hostfs inode IDs may
> not be unique for the same hostfs superblock because multiple host's
> (backed) superblocks may be used.
> 
> Delete inodes when dropping them to force backed host's file descriptors
> closing.
> 
> This enables to entirely remove ARCH_EPHEMERAL_INODES, and then makes
> Landlock fully supported by UML.  This is very useful for testing
> (ongoing and backported) changes.

Removing ARCH_EPHEMERAL_INODES should be a patch on its own, IMHO.

> These changes also factor out and simplify some helpers thanks to the
> new hostfs_inode_update() and the hostfs_iget() revamp: read_name(),
> hostfs_create(), hostfs_lookup(), hostfs_mknod(), and
> hostfs_fill_sb_common().
> 
> A following commit with new Landlock tests check this new hostfs inode
> consistency.
> 
> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: <stable@vger.kernel.org> # 5.15.x: ce72750f04d6: hostfs: Fix writeback of
> dirty pages
> Cc: <stable@vger.kernel.org> # 5.15+

I'm not sure whether this patch qualifies as stable material.
While I fully agree that the current behavoir is odd, nothing user visible
is really broken so far.

> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20230309165455.175131-2-mic@digikod.net

Other than that, patch looks good to me.

Thanks,
//richard

Re: [PATCH v1 1/5] hostfs: Fix ephemeral inodes

[Mickaël Salaün]

On 21/05/2023 23:13, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Mickaël Salaün" <mic@digikod.net>
>> hostfs creates a new inode for each opened or created file, which created
>> useless inode allocations and forbade identifying a host file with a kernel
>> inode.
>>
>> Fix this uncommon filesystem behavior by tying kernel inodes to host
>> file's inode and device IDs.  Even if the host filesystem inodes may be
>> recycled, this cannot happen while a file referencing it is open, which
>> is the case with hostfs.  It should be noted that hostfs inode IDs may
>> not be unique for the same hostfs superblock because multiple host's
>> (backed) superblocks may be used.
>>
>> Delete inodes when dropping them to force backed host's file descriptors
>> closing.
>>
>> This enables to entirely remove ARCH_EPHEMERAL_INODES, and then makes
>> Landlock fully supported by UML.  This is very useful for testing
>> (ongoing and backported) changes.
> 
> Removing ARCH_EPHEMERAL_INODES should be a patch on its own, IMHO.

OK, I'll do that in the next series.

> 
>> These changes also factor out and simplify some helpers thanks to the
>> new hostfs_inode_update() and the hostfs_iget() revamp: read_name(),
>> hostfs_create(), hostfs_lookup(), hostfs_mknod(), and
>> hostfs_fill_sb_common().
>>
>> A following commit with new Landlock tests check this new hostfs inode
>> consistency.
>>
>> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> Cc: Johannes Berg <johannes@sipsolutions.net>
>> Cc: Richard Weinberger <richard@nod.at>
>> Cc: <stable@vger.kernel.org> # 5.15.x: ce72750f04d6: hostfs: Fix writeback of
>> dirty pages
>> Cc: <stable@vger.kernel.org> # 5.15+
> 
> I'm not sure whether this patch qualifies as stable material.
> While I fully agree that the current behavoir is odd, nothing user visible
> is really broken so far.
I added the ARCH_EPHEMERAL_INODES knob to avoid unexpected behavior. 
Thanks to that there is no regression for Landlock, but it's unfortunate 
that we could not use UML to test old kernel versions. According to this 
odd behavior, I guess some user space may not work with hostfs because 
of this issue, hence this Cc. I can remove it if you think it is not the 
case.


> 
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Link: https://lore.kernel.org/r/20230309165455.175131-2-mic@digikod.net
> 
> Other than that, patch looks good to me.

Good, I'll send a new series with your suggestions.

> 
> Thanks,
> //richard

Re: [PATCH v1 1/5] hostfs: Fix ephemeral inodes

[Mickaël Salaün]

On 26/05/2023 18:40, Mickaël Salaün wrote:
> 
> On 21/05/2023 23:13, Richard Weinberger wrote:
>> ----- Ursprüngliche Mail -----
>>> Von: "Mickaël Salaün" <mic@digikod.net>
>>> hostfs creates a new inode for each opened or created file, which created
>>> useless inode allocations and forbade identifying a host file with a kernel
>>> inode.
>>>
>>> Fix this uncommon filesystem behavior by tying kernel inodes to host
>>> file's inode and device IDs.  Even if the host filesystem inodes may be
>>> recycled, this cannot happen while a file referencing it is open, which
>>> is the case with hostfs.  It should be noted that hostfs inode IDs may
>>> not be unique for the same hostfs superblock because multiple host's
>>> (backed) superblocks may be used.
>>>
>>> Delete inodes when dropping them to force backed host's file descriptors
>>> closing.
>>>
>>> This enables to entirely remove ARCH_EPHEMERAL_INODES, and then makes
>>> Landlock fully supported by UML.  This is very useful for testing
>>> (ongoing and backported) changes.
>>
>> Removing ARCH_EPHEMERAL_INODES should be a patch on its own, IMHO.
> 
> OK, I'll do that in the next series.

Well, I added ARCH_EPHEMERAL_INODES for Landlock specifically because of 
this hostfs inconsistency, and it is not used by anything else in the 
kernel: https://git.kernel.org/torvalds/c/cb2c7d1a1776
I then think it makes sense to remove this Kconfig option with the 
hostfs change. Moreover, this protects against erroneously backporting 
the ARCH_EPHEMERAL_INODES change, which would silently introduce a bug 
for Landlock.


> 
>>
>>> These changes also factor out and simplify some helpers thanks to the
>>> new hostfs_inode_update() and the hostfs_iget() revamp: read_name(),
>>> hostfs_create(), hostfs_lookup(), hostfs_mknod(), and
>>> hostfs_fill_sb_common().
>>>
>>> A following commit with new Landlock tests check this new hostfs inode
>>> consistency.
>>>
>>> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>> Cc: Johannes Berg <johannes@sipsolutions.net>
>>> Cc: Richard Weinberger <richard@nod.at>
>>> Cc: <stable@vger.kernel.org> # 5.15.x: ce72750f04d6: hostfs: Fix writeback of
>>> dirty pages
>>> Cc: <stable@vger.kernel.org> # 5.15+
>>
>> I'm not sure whether this patch qualifies as stable material.
>> While I fully agree that the current behavoir is odd, nothing user visible
>> is really broken so far.
> I added the ARCH_EPHEMERAL_INODES knob to avoid unexpected behavior.
> Thanks to that there is no regression for Landlock, but it's unfortunate
> that we could not use UML to test old kernel versions. According to this
> odd behavior, I guess some user space may not work with hostfs because
> of this issue, hence this Cc. I can remove it if you think it is not the
> case.
> 
> 
>>
>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> Link: https://lore.kernel.org/r/20230309165455.175131-2-mic@digikod.net
>>
>> Other than that, patch looks good to me.
> 
> Good, I'll send a new series with your suggestions.

Can I add your Signed-off-by to this patch (without touching 
ARCH_EPHEMERAL_INODES changes, but removing the Cc stable)?

Are you OK for me to push this patch (with the whole series) in the 
Landlock and next tree?

I'll send a new series splitting the Landlock tests to make a patch 
dedicated to Landlock with hostfs tests (not backported), and with 
another patch containing backportable and independent new Landlock FS tests.

Re: [PATCH v1 1/5] hostfs: Fix ephemeral inodes

[Richard Weinberger]

----- Ursprüngliche Mail -----
>> Good, I'll send a new series with your suggestions.
> 
> Can I add your Signed-off-by to this patch (without touching
> ARCH_EPHEMERAL_INODES changes, but removing the Cc stable)?
> 
> Are you OK for me to push this patch (with the whole series) in the
> Landlock and next tree?

Yes. Feel free to add:
Acked-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH v1 1/5] hostfs: Fix ephemeral inodes

[Roberto Sassu]

On Thu, 2023-03-09 at 17:54 +0100, Mickaël Salaün wrote:
> hostfs creates a new inode for each opened or created file, which created
> useless inode allocations and forbade identifying a host file with a kernel
> inode.
> 
> Fix this uncommon filesystem behavior by tying kernel inodes to host
> file's inode and device IDs.  Even if the host filesystem inodes may be
> recycled, this cannot happen while a file referencing it is open, which
> is the case with hostfs.  It should be noted that hostfs inode IDs may
> not be unique for the same hostfs superblock because multiple host's
> (backed) superblocks may be used.

I hoped that this patch solved an issue when testing the
inode_setsecurity and inode_getsecurity combination. Unfortunately, it
does not work, since after inode_setsecurity the inode is dropped. At
the time inode_getsecurity is called, the security blob is lost.

Roberto

> Delete inodes when dropping them to force backed host's file descriptors
> closing.
> 
> This enables to entirely remove ARCH_EPHEMERAL_INODES, and then makes
> Landlock fully supported by UML.  This is very useful for testing
> (ongoing and backported) changes.
> 
> These changes also factor out and simplify some helpers thanks to the
> new hostfs_inode_update() and the hostfs_iget() revamp: read_name(),
> hostfs_create(), hostfs_lookup(), hostfs_mknod(), and
> hostfs_fill_sb_common().
> 
> A following commit with new Landlock tests check this new hostfs inode
> consistency.
> 
> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: <stable@vger.kernel.org> # 5.15.x: ce72750f04d6: hostfs: Fix writeback of dirty pages
> Cc: <stable@vger.kernel.org> # 5.15+
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20230309165455.175131-2-mic@digikod.net
> ---
>  arch/Kconfig              |   7 --
>  arch/um/Kconfig           |   1 -
>  fs/hostfs/hostfs.h        |   1 +
>  fs/hostfs/hostfs_kern.c   | 213 +++++++++++++++++++-------------------
>  fs/hostfs/hostfs_user.c   |   1 +
>  security/landlock/Kconfig |   2 +-
>  6 files changed, 109 insertions(+), 116 deletions(-)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index e3511afbb7f2..d5f0841ac3c1 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -1156,13 +1156,6 @@ config COMPAT_32BIT_TIME
>  config ARCH_NO_PREEMPT
>  	bool
>  
> -config ARCH_EPHEMERAL_INODES
> -	def_bool n
> -	help
> -	  An arch should select this symbol if it doesn't keep track of inode
> -	  instances on its own, but instead relies on something else (e.g. the
> -	  host kernel for an UML kernel).
> -
>  config ARCH_SUPPORTS_RT
>  	bool
>  
> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> index 541a9b18e343..4057d5267c6a 100644
> --- a/arch/um/Kconfig
> +++ b/arch/um/Kconfig
> @@ -5,7 +5,6 @@ menu "UML-specific options"
>  config UML
>  	bool
>  	default y
> -	select ARCH_EPHEMERAL_INODES
>  	select ARCH_HAS_FORTIFY_SOURCE
>  	select ARCH_HAS_GCOV_PROFILE_ALL
>  	select ARCH_HAS_KCOV
> diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
> index 69cb796f6270..0239e3af3945 100644
> --- a/fs/hostfs/hostfs.h
> +++ b/fs/hostfs/hostfs.h
> @@ -65,6 +65,7 @@ struct hostfs_stat {
>  	unsigned long long blocks;
>  	unsigned int maj;
>  	unsigned int min;
> +	dev_t dev;
>  };
>  
>  extern int stat_file(const char *path, struct hostfs_stat *p, int fd);
> diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
> index 28b4f15c19eb..19496f732016 100644
> --- a/fs/hostfs/hostfs_kern.c
> +++ b/fs/hostfs/hostfs_kern.c
> @@ -26,6 +26,7 @@ struct hostfs_inode_info {
>  	fmode_t mode;
>  	struct inode vfs_inode;
>  	struct mutex open_mutex;
> +	dev_t dev;
>  };
>  
>  static inline struct hostfs_inode_info *HOSTFS_I(struct inode *inode)
> @@ -182,14 +183,6 @@ static char *follow_link(char *link)
>  	return ERR_PTR(n);
>  }
>  
> -static struct inode *hostfs_iget(struct super_block *sb)
> -{
> -	struct inode *inode = new_inode(sb);
> -	if (!inode)
> -		return ERR_PTR(-ENOMEM);
> -	return inode;
> -}
> -
>  static int hostfs_statfs(struct dentry *dentry, struct kstatfs *sf)
>  {
>  	/*
> @@ -228,6 +221,7 @@ static struct inode *hostfs_alloc_inode(struct super_block *sb)
>  		return NULL;
>  	hi->fd = -1;
>  	hi->mode = 0;
> +	hi->dev = 0;
>  	inode_init_once(&hi->vfs_inode);
>  	mutex_init(&hi->open_mutex);
>  	return &hi->vfs_inode;
> @@ -240,6 +234,7 @@ static void hostfs_evict_inode(struct inode *inode)
>  	if (HOSTFS_I(inode)->fd != -1) {
>  		close_file(&HOSTFS_I(inode)->fd);
>  		HOSTFS_I(inode)->fd = -1;
> +		HOSTFS_I(inode)->dev = 0;
>  	}
>  }
>  
> @@ -265,6 +260,7 @@ static int hostfs_show_options(struct seq_file *seq, struct dentry *root)
>  static const struct super_operations hostfs_sbops = {
>  	.alloc_inode	= hostfs_alloc_inode,
>  	.free_inode	= hostfs_free_inode,
> +	.drop_inode	= generic_delete_inode,
>  	.evict_inode	= hostfs_evict_inode,
>  	.statfs		= hostfs_statfs,
>  	.show_options	= hostfs_show_options,
> @@ -512,18 +508,31 @@ static const struct address_space_operations hostfs_aops = {
>  	.write_end	= hostfs_write_end,
>  };
>  
> -static int read_name(struct inode *ino, char *name)
> +static int hostfs_inode_update(struct inode *ino, const struct hostfs_stat *st)
> +{
> +	set_nlink(ino, st->nlink);
> +	i_uid_write(ino, st->uid);
> +	i_gid_write(ino, st->gid);
> +	ino->i_atime =
> +		(struct timespec64){ st->atime.tv_sec, st->atime.tv_nsec };
> +	ino->i_mtime =
> +		(struct timespec64){ st->mtime.tv_sec, st->mtime.tv_nsec };
> +	ino->i_ctime =
> +		(struct timespec64){ st->ctime.tv_sec, st->ctime.tv_nsec };
> +	ino->i_size = st->size;
> +	ino->i_blocks = st->blocks;
> +	return 0;
> +}
> +
> +static int hostfs_inode_set(struct inode *ino, void *data)
>  {
> +	struct hostfs_stat *st = data;
>  	dev_t rdev;
> -	struct hostfs_stat st;
> -	int err = stat_file(name, &st, -1);
> -	if (err)
> -		return err;
>  
>  	/* Reencode maj and min with the kernel encoding.*/
> -	rdev = MKDEV(st.maj, st.min);
> +	rdev = MKDEV(st->maj, st->min);
>  
> -	switch (st.mode & S_IFMT) {
> +	switch (st->mode & S_IFMT) {
>  	case S_IFLNK:
>  		ino->i_op = &hostfs_link_iops;
>  		break;
> @@ -535,7 +544,7 @@ static int read_name(struct inode *ino, char *name)
>  	case S_IFBLK:
>  	case S_IFIFO:
>  	case S_IFSOCK:
> -		init_special_inode(ino, st.mode & S_IFMT, rdev);
> +		init_special_inode(ino, st->mode & S_IFMT, rdev);
>  		ino->i_op = &hostfs_iops;
>  		break;
>  	case S_IFREG:
> @@ -547,17 +556,42 @@ static int read_name(struct inode *ino, char *name)
>  		return -EIO;
>  	}
>  
> -	ino->i_ino = st.ino;
> -	ino->i_mode = st.mode;
> -	set_nlink(ino, st.nlink);
> -	i_uid_write(ino, st.uid);
> -	i_gid_write(ino, st.gid);
> -	ino->i_atime = (struct timespec64){ st.atime.tv_sec, st.atime.tv_nsec };
> -	ino->i_mtime = (struct timespec64){ st.mtime.tv_sec, st.mtime.tv_nsec };
> -	ino->i_ctime = (struct timespec64){ st.ctime.tv_sec, st.ctime.tv_nsec };
> -	ino->i_size = st.size;
> -	ino->i_blocks = st.blocks;
> -	return 0;
> +	HOSTFS_I(ino)->dev = st->dev;
> +	ino->i_ino = st->ino;
> +	ino->i_mode = st->mode;
> +	return hostfs_inode_update(ino, st);
> +}
> +
> +static int hostfs_inode_test(struct inode *inode, void *data)
> +{
> +	const struct hostfs_stat *st = data;
> +
> +	return inode->i_ino == st->ino && HOSTFS_I(inode)->dev == st->dev;
> +}
> +
> +static struct inode *hostfs_iget(struct super_block *sb, char *name)
> +{
> +	struct inode *inode;
> +	struct hostfs_stat st;
> +	int err = stat_file(name, &st, -1);
> +
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	inode = iget5_locked(sb, st.ino, hostfs_inode_test, hostfs_inode_set,
> +			     &st);
> +	if (!inode)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (inode->i_state & I_NEW) {
> +		unlock_new_inode(inode);
> +	} else {
> +		spin_lock(&inode->i_lock);
> +		hostfs_inode_update(inode, &st);
> +		spin_unlock(&inode->i_lock);
> +	}
> +
> +	return inode;
>  }
>  
>  static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
> @@ -565,62 +599,48 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
>  {
>  	struct inode *inode;
>  	char *name;
> -	int error, fd;
> -
> -	inode = hostfs_iget(dir->i_sb);
> -	if (IS_ERR(inode)) {
> -		error = PTR_ERR(inode);
> -		goto out;
> -	}
> +	int fd;
>  
> -	error = -ENOMEM;
>  	name = dentry_name(dentry);
>  	if (name == NULL)
> -		goto out_put;
> +		return -ENOMEM;
>  
>  	fd = file_create(name, mode & 0777);
> -	if (fd < 0)
> -		error = fd;
> -	else
> -		error = read_name(inode, name);
> +	if (fd < 0) {
> +		__putname(name);
> +		return fd;
> +	}
>  
> +	inode = hostfs_iget(dir->i_sb, name);
>  	__putname(name);
> -	if (error)
> -		goto out_put;
> +	if (IS_ERR(inode))
> +		return PTR_ERR(inode);
>  
>  	HOSTFS_I(inode)->fd = fd;
>  	HOSTFS_I(inode)->mode = FMODE_READ | FMODE_WRITE;
>  	d_instantiate(dentry, inode);
>  	return 0;
> -
> - out_put:
> -	iput(inode);
> - out:
> -	return error;
>  }
>  
>  static struct dentry *hostfs_lookup(struct inode *ino, struct dentry *dentry,
>  				    unsigned int flags)
>  {
> -	struct inode *inode;
> +	struct inode *inode = NULL;
>  	char *name;
> -	int err;
> -
> -	inode = hostfs_iget(ino->i_sb);
> -	if (IS_ERR(inode))
> -		goto out;
>  
> -	err = -ENOMEM;
>  	name = dentry_name(dentry);
> -	if (name) {
> -		err = read_name(inode, name);
> -		__putname(name);
> -	}
> -	if (err) {
> -		iput(inode);
> -		inode = (err == -ENOENT) ? NULL : ERR_PTR(err);
> +	if (name == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	inode = hostfs_iget(ino->i_sb, name);
> +	__putname(name);
> +	if (IS_ERR(inode)) {
> +		if (PTR_ERR(inode) == -ENOENT)
> +			inode = NULL;
> +		else
> +			return ERR_CAST(inode);
>  	}
> - out:
> +
>  	return d_splice_alias(inode, dentry);
>  }
>  
> @@ -704,35 +724,23 @@ static int hostfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
>  	char *name;
>  	int err;
>  
> -	inode = hostfs_iget(dir->i_sb);
> -	if (IS_ERR(inode)) {
> -		err = PTR_ERR(inode);
> -		goto out;
> -	}
> -
> -	err = -ENOMEM;
>  	name = dentry_name(dentry);
>  	if (name == NULL)
> -		goto out_put;
> +		return -ENOMEM;
>  
>  	err = do_mknod(name, mode, MAJOR(dev), MINOR(dev));
> -	if (err)
> -		goto out_free;
> +	if (err) {
> +		__putname(name);
> +		return err;
> +	}
>  
> -	err = read_name(inode, name);
> +	inode = hostfs_iget(dir->i_sb, name);
>  	__putname(name);
> -	if (err)
> -		goto out_put;
> +	if (IS_ERR(inode))
> +		return PTR_ERR(inode);
>  
>  	d_instantiate(dentry, inode);
>  	return 0;
> -
> - out_free:
> -	__putname(name);
> - out_put:
> -	iput(inode);
> - out:
> -	return err;
>  }
>  
>  static int hostfs_rename2(struct mnt_idmap *idmap,
> @@ -929,49 +937,40 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
>  	sb->s_maxbytes = MAX_LFS_FILESIZE;
>  	err = super_setup_bdi(sb);
>  	if (err)
> -		goto out;
> +		return err;
>  
>  	/* NULL is printed as '(null)' by printf(): avoid that. */
>  	if (req_root == NULL)
>  		req_root = "";
>  
> -	err = -ENOMEM;
>  	sb->s_fs_info = host_root_path =
>  		kasprintf(GFP_KERNEL, "%s/%s", root_ino, req_root);
>  	if (host_root_path == NULL)
> -		goto out;
> -
> -	root_inode = new_inode(sb);
> -	if (!root_inode)
> -		goto out;
> +		return -ENOMEM;
>  
> -	err = read_name(root_inode, host_root_path);
> -	if (err)
> -		goto out_put;
> +	root_inode = hostfs_iget(sb, host_root_path);
> +	if (IS_ERR(root_inode))
> +		return PTR_ERR(root_inode);
>  
>  	if (S_ISLNK(root_inode->i_mode)) {
> -		char *name = follow_link(host_root_path);
> -		if (IS_ERR(name)) {
> -			err = PTR_ERR(name);
> -			goto out_put;
> -		}
> -		err = read_name(root_inode, name);
> +		char *name;
> +
> +		iput(root_inode);
> +		name = follow_link(host_root_path);
> +		if (IS_ERR(name))
> +			return PTR_ERR(name);
> +
> +		root_inode = hostfs_iget(sb, name);
>  		kfree(name);
> -		if (err)
> -			goto out_put;
> +		if (IS_ERR(root_inode))
> +			return PTR_ERR(root_inode);
>  	}
>  
> -	err = -ENOMEM;
>  	sb->s_root = d_make_root(root_inode);
>  	if (sb->s_root == NULL)
> -		goto out;
> +		return -ENOMEM;
>  
>  	return 0;
> -
> -out_put:
> -	iput(root_inode);
> -out:
> -	return err;
>  }
>  
>  static struct dentry *hostfs_read_sb(struct file_system_type *type,
> diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
> index 5ecc4706172b..840619e39a1a 100644
> --- a/fs/hostfs/hostfs_user.c
> +++ b/fs/hostfs/hostfs_user.c
> @@ -36,6 +36,7 @@ static void stat64_to_hostfs(const struct stat64 *buf, struct hostfs_stat *p)
>  	p->blocks = buf->st_blocks;
>  	p->maj = os_major(buf->st_rdev);
>  	p->min = os_minor(buf->st_rdev);
> +	p->dev = buf->st_dev;
>  }
>  
>  int stat_file(const char *path, struct hostfs_stat *p, int fd)
> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
> index 8e33c4e8ffb8..c1e862a38410 100644
> --- a/security/landlock/Kconfig
> +++ b/security/landlock/Kconfig
> @@ -2,7 +2,7 @@
>  
>  config SECURITY_LANDLOCK
>  	bool "Landlock support"
> -	depends on SECURITY && !ARCH_EPHEMERAL_INODES
> +	depends on SECURITY
>  	select SECURITY_PATH
>  	help
>  	  Landlock is a sandboxing mechanism that enables processes to restrict

Re: [PATCH v1 1/5] hostfs: Fix ephemeral inodes

[Mickaël Salaün]

On 06/06/2023 15:12, Roberto Sassu wrote:
> On Thu, 2023-03-09 at 17:54 +0100, Mickaël Salaün wrote:
>> hostfs creates a new inode for each opened or created file, which created
>> useless inode allocations and forbade identifying a host file with a kernel
>> inode.
>>
>> Fix this uncommon filesystem behavior by tying kernel inodes to host
>> file's inode and device IDs.  Even if the host filesystem inodes may be
>> recycled, this cannot happen while a file referencing it is open, which
>> is the case with hostfs.  It should be noted that hostfs inode IDs may
>> not be unique for the same hostfs superblock because multiple host's
>> (backed) superblocks may be used.
> 
> I hoped that this patch solved an issue when testing the
> inode_setsecurity and inode_getsecurity combination. Unfortunately, it
> does not work, since after inode_setsecurity the inode is dropped. At
> the time inode_getsecurity is called, the security blob is lost.

Indeed, this is because inode[sg]etsecurity() rely on inode's xattr, 
which are not handled by hostfs. Why not simply use overlayfs with an 
upper tmpfs that supports xattr? This patch might be needed to such 
scenario though.


> 
> Roberto
> 
>> Delete inodes when dropping them to force backed host's file descriptors
>> closing.
>>
>> This enables to entirely remove ARCH_EPHEMERAL_INODES, and then makes
>> Landlock fully supported by UML.  This is very useful for testing
>> (ongoing and backported) changes.
>>
>> These changes also factor out and simplify some helpers thanks to the
>> new hostfs_inode_update() and the hostfs_iget() revamp: read_name(),
>> hostfs_create(), hostfs_lookup(), hostfs_mknod(), and
>> hostfs_fill_sb_common().
>>
>> A following commit with new Landlock tests check this new hostfs inode
>> consistency.
>>
>> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> Cc: Johannes Berg <johannes@sipsolutions.net>
>> Cc: Richard Weinberger <richard@nod.at>
>> Cc: <stable@vger.kernel.org> # 5.15.x: ce72750f04d6: hostfs: Fix writeback of dirty pages
>> Cc: <stable@vger.kernel.org> # 5.15+
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Link: https://lore.kernel.org/r/20230309165455.175131-2-mic@digikod.net
>> ---
>>   arch/Kconfig              |   7 --
>>   arch/um/Kconfig           |   1 -
>>   fs/hostfs/hostfs.h        |   1 +
>>   fs/hostfs/hostfs_kern.c   | 213 +++++++++++++++++++-------------------
>>   fs/hostfs/hostfs_user.c   |   1 +
>>   security/landlock/Kconfig |   2 +-
>>   6 files changed, 109 insertions(+), 116 deletions(-)
>>
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index e3511afbb7f2..d5f0841ac3c1 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -1156,13 +1156,6 @@ config COMPAT_32BIT_TIME
>>   config ARCH_NO_PREEMPT
>>   	bool
>>   
>> -config ARCH_EPHEMERAL_INODES
>> -	def_bool n
>> -	help
>> -	  An arch should select this symbol if it doesn't keep track of inode
>> -	  instances on its own, but instead relies on something else (e.g. the
>> -	  host kernel for an UML kernel).
>> -
>>   config ARCH_SUPPORTS_RT
>>   	bool
>>   
>> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
>> index 541a9b18e343..4057d5267c6a 100644
>> --- a/arch/um/Kconfig
>> +++ b/arch/um/Kconfig
>> @@ -5,7 +5,6 @@ menu "UML-specific options"
>>   config UML
>>   	bool
>>   	default y
>> -	select ARCH_EPHEMERAL_INODES
>>   	select ARCH_HAS_FORTIFY_SOURCE
>>   	select ARCH_HAS_GCOV_PROFILE_ALL
>>   	select ARCH_HAS_KCOV
>> diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
>> index 69cb796f6270..0239e3af3945 100644
>> --- a/fs/hostfs/hostfs.h
>> +++ b/fs/hostfs/hostfs.h
>> @@ -65,6 +65,7 @@ struct hostfs_stat {
>>   	unsigned long long blocks;
>>   	unsigned int maj;
>>   	unsigned int min;
>> +	dev_t dev;
>>   };
>>   
>>   extern int stat_file(const char *path, struct hostfs_stat *p, int fd);
>> diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
>> index 28b4f15c19eb..19496f732016 100644
>> --- a/fs/hostfs/hostfs_kern.c
>> +++ b/fs/hostfs/hostfs_kern.c
>> @@ -26,6 +26,7 @@ struct hostfs_inode_info {
>>   	fmode_t mode;
>>   	struct inode vfs_inode;
>>   	struct mutex open_mutex;
>> +	dev_t dev;
>>   };
>>   
>>   static inline struct hostfs_inode_info *HOSTFS_I(struct inode *inode)
>> @@ -182,14 +183,6 @@ static char *follow_link(char *link)
>>   	return ERR_PTR(n);
>>   }
>>   
>> -static struct inode *hostfs_iget(struct super_block *sb)
>> -{
>> -	struct inode *inode = new_inode(sb);
>> -	if (!inode)
>> -		return ERR_PTR(-ENOMEM);
>> -	return inode;
>> -}
>> -
>>   static int hostfs_statfs(struct dentry *dentry, struct kstatfs *sf)
>>   {
>>   	/*
>> @@ -228,6 +221,7 @@ static struct inode *hostfs_alloc_inode(struct super_block *sb)
>>   		return NULL;
>>   	hi->fd = -1;
>>   	hi->mode = 0;
>> +	hi->dev = 0;
>>   	inode_init_once(&hi->vfs_inode);
>>   	mutex_init(&hi->open_mutex);
>>   	return &hi->vfs_inode;
>> @@ -240,6 +234,7 @@ static void hostfs_evict_inode(struct inode *inode)
>>   	if (HOSTFS_I(inode)->fd != -1) {
>>   		close_file(&HOSTFS_I(inode)->fd);
>>   		HOSTFS_I(inode)->fd = -1;
>> +		HOSTFS_I(inode)->dev = 0;
>>   	}
>>   }
>>   
>> @@ -265,6 +260,7 @@ static int hostfs_show_options(struct seq_file *seq, struct dentry *root)
>>   static const struct super_operations hostfs_sbops = {
>>   	.alloc_inode	= hostfs_alloc_inode,
>>   	.free_inode	= hostfs_free_inode,
>> +	.drop_inode	= generic_delete_inode,
>>   	.evict_inode	= hostfs_evict_inode,
>>   	.statfs		= hostfs_statfs,
>>   	.show_options	= hostfs_show_options,
>> @@ -512,18 +508,31 @@ static const struct address_space_operations hostfs_aops = {
>>   	.write_end	= hostfs_write_end,
>>   };
>>   
>> -static int read_name(struct inode *ino, char *name)
>> +static int hostfs_inode_update(struct inode *ino, const struct hostfs_stat *st)
>> +{
>> +	set_nlink(ino, st->nlink);
>> +	i_uid_write(ino, st->uid);
>> +	i_gid_write(ino, st->gid);
>> +	ino->i_atime =
>> +		(struct timespec64){ st->atime.tv_sec, st->atime.tv_nsec };
>> +	ino->i_mtime =
>> +		(struct timespec64){ st->mtime.tv_sec, st->mtime.tv_nsec };
>> +	ino->i_ctime =
>> +		(struct timespec64){ st->ctime.tv_sec, st->ctime.tv_nsec };
>> +	ino->i_size = st->size;
>> +	ino->i_blocks = st->blocks;
>> +	return 0;
>> +}
>> +
>> +static int hostfs_inode_set(struct inode *ino, void *data)
>>   {
>> +	struct hostfs_stat *st = data;
>>   	dev_t rdev;
>> -	struct hostfs_stat st;
>> -	int err = stat_file(name, &st, -1);
>> -	if (err)
>> -		return err;
>>   
>>   	/* Reencode maj and min with the kernel encoding.*/
>> -	rdev = MKDEV(st.maj, st.min);
>> +	rdev = MKDEV(st->maj, st->min);
>>   
>> -	switch (st.mode & S_IFMT) {
>> +	switch (st->mode & S_IFMT) {
>>   	case S_IFLNK:
>>   		ino->i_op = &hostfs_link_iops;
>>   		break;
>> @@ -535,7 +544,7 @@ static int read_name(struct inode *ino, char *name)
>>   	case S_IFBLK:
>>   	case S_IFIFO:
>>   	case S_IFSOCK:
>> -		init_special_inode(ino, st.mode & S_IFMT, rdev);
>> +		init_special_inode(ino, st->mode & S_IFMT, rdev);
>>   		ino->i_op = &hostfs_iops;
>>   		break;
>>   	case S_IFREG:
>> @@ -547,17 +556,42 @@ static int read_name(struct inode *ino, char *name)
>>   		return -EIO;
>>   	}
>>   
>> -	ino->i_ino = st.ino;
>> -	ino->i_mode = st.mode;
>> -	set_nlink(ino, st.nlink);
>> -	i_uid_write(ino, st.uid);
>> -	i_gid_write(ino, st.gid);
>> -	ino->i_atime = (struct timespec64){ st.atime.tv_sec, st.atime.tv_nsec };
>> -	ino->i_mtime = (struct timespec64){ st.mtime.tv_sec, st.mtime.tv_nsec };
>> -	ino->i_ctime = (struct timespec64){ st.ctime.tv_sec, st.ctime.tv_nsec };
>> -	ino->i_size = st.size;
>> -	ino->i_blocks = st.blocks;
>> -	return 0;
>> +	HOSTFS_I(ino)->dev = st->dev;
>> +	ino->i_ino = st->ino;
>> +	ino->i_mode = st->mode;
>> +	return hostfs_inode_update(ino, st);
>> +}
>> +
>> +static int hostfs_inode_test(struct inode *inode, void *data)
>> +{
>> +	const struct hostfs_stat *st = data;
>> +
>> +	return inode->i_ino == st->ino && HOSTFS_I(inode)->dev == st->dev;
>> +}
>> +
>> +static struct inode *hostfs_iget(struct super_block *sb, char *name)
>> +{
>> +	struct inode *inode;
>> +	struct hostfs_stat st;
>> +	int err = stat_file(name, &st, -1);
>> +
>> +	if (err)
>> +		return ERR_PTR(err);
>> +
>> +	inode = iget5_locked(sb, st.ino, hostfs_inode_test, hostfs_inode_set,
>> +			     &st);
>> +	if (!inode)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	if (inode->i_state & I_NEW) {
>> +		unlock_new_inode(inode);
>> +	} else {
>> +		spin_lock(&inode->i_lock);
>> +		hostfs_inode_update(inode, &st);
>> +		spin_unlock(&inode->i_lock);
>> +	}
>> +
>> +	return inode;
>>   }
>>   
>>   static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
>> @@ -565,62 +599,48 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
>>   {
>>   	struct inode *inode;
>>   	char *name;
>> -	int error, fd;
>> -
>> -	inode = hostfs_iget(dir->i_sb);
>> -	if (IS_ERR(inode)) {
>> -		error = PTR_ERR(inode);
>> -		goto out;
>> -	}
>> +	int fd;
>>   
>> -	error = -ENOMEM;
>>   	name = dentry_name(dentry);
>>   	if (name == NULL)
>> -		goto out_put;
>> +		return -ENOMEM;
>>   
>>   	fd = file_create(name, mode & 0777);
>> -	if (fd < 0)
>> -		error = fd;
>> -	else
>> -		error = read_name(inode, name);
>> +	if (fd < 0) {
>> +		__putname(name);
>> +		return fd;
>> +	}
>>   
>> +	inode = hostfs_iget(dir->i_sb, name);
>>   	__putname(name);
>> -	if (error)
>> -		goto out_put;
>> +	if (IS_ERR(inode))
>> +		return PTR_ERR(inode);
>>   
>>   	HOSTFS_I(inode)->fd = fd;
>>   	HOSTFS_I(inode)->mode = FMODE_READ | FMODE_WRITE;
>>   	d_instantiate(dentry, inode);
>>   	return 0;
>> -
>> - out_put:
>> -	iput(inode);
>> - out:
>> -	return error;
>>   }
>>   
>>   static struct dentry *hostfs_lookup(struct inode *ino, struct dentry *dentry,
>>   				    unsigned int flags)
>>   {
>> -	struct inode *inode;
>> +	struct inode *inode = NULL;
>>   	char *name;
>> -	int err;
>> -
>> -	inode = hostfs_iget(ino->i_sb);
>> -	if (IS_ERR(inode))
>> -		goto out;
>>   
>> -	err = -ENOMEM;
>>   	name = dentry_name(dentry);
>> -	if (name) {
>> -		err = read_name(inode, name);
>> -		__putname(name);
>> -	}
>> -	if (err) {
>> -		iput(inode);
>> -		inode = (err == -ENOENT) ? NULL : ERR_PTR(err);
>> +	if (name == NULL)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	inode = hostfs_iget(ino->i_sb, name);
>> +	__putname(name);
>> +	if (IS_ERR(inode)) {
>> +		if (PTR_ERR(inode) == -ENOENT)
>> +			inode = NULL;
>> +		else
>> +			return ERR_CAST(inode);
>>   	}
>> - out:
>> +
>>   	return d_splice_alias(inode, dentry);
>>   }
>>   
>> @@ -704,35 +724,23 @@ static int hostfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
>>   	char *name;
>>   	int err;
>>   
>> -	inode = hostfs_iget(dir->i_sb);
>> -	if (IS_ERR(inode)) {
>> -		err = PTR_ERR(inode);
>> -		goto out;
>> -	}
>> -
>> -	err = -ENOMEM;
>>   	name = dentry_name(dentry);
>>   	if (name == NULL)
>> -		goto out_put;
>> +		return -ENOMEM;
>>   
>>   	err = do_mknod(name, mode, MAJOR(dev), MINOR(dev));
>> -	if (err)
>> -		goto out_free;
>> +	if (err) {
>> +		__putname(name);
>> +		return err;
>> +	}
>>   
>> -	err = read_name(inode, name);
>> +	inode = hostfs_iget(dir->i_sb, name);
>>   	__putname(name);
>> -	if (err)
>> -		goto out_put;
>> +	if (IS_ERR(inode))
>> +		return PTR_ERR(inode);
>>   
>>   	d_instantiate(dentry, inode);
>>   	return 0;
>> -
>> - out_free:
>> -	__putname(name);
>> - out_put:
>> -	iput(inode);
>> - out:
>> -	return err;
>>   }
>>   
>>   static int hostfs_rename2(struct mnt_idmap *idmap,
>> @@ -929,49 +937,40 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
>>   	sb->s_maxbytes = MAX_LFS_FILESIZE;
>>   	err = super_setup_bdi(sb);
>>   	if (err)
>> -		goto out;
>> +		return err;
>>   
>>   	/* NULL is printed as '(null)' by printf(): avoid that. */
>>   	if (req_root == NULL)
>>   		req_root = "";
>>   
>> -	err = -ENOMEM;
>>   	sb->s_fs_info = host_root_path =
>>   		kasprintf(GFP_KERNEL, "%s/%s", root_ino, req_root);
>>   	if (host_root_path == NULL)
>> -		goto out;
>> -
>> -	root_inode = new_inode(sb);
>> -	if (!root_inode)
>> -		goto out;
>> +		return -ENOMEM;
>>   
>> -	err = read_name(root_inode, host_root_path);
>> -	if (err)
>> -		goto out_put;
>> +	root_inode = hostfs_iget(sb, host_root_path);
>> +	if (IS_ERR(root_inode))
>> +		return PTR_ERR(root_inode);
>>   
>>   	if (S_ISLNK(root_inode->i_mode)) {
>> -		char *name = follow_link(host_root_path);
>> -		if (IS_ERR(name)) {
>> -			err = PTR_ERR(name);
>> -			goto out_put;
>> -		}
>> -		err = read_name(root_inode, name);
>> +		char *name;
>> +
>> +		iput(root_inode);
>> +		name = follow_link(host_root_path);
>> +		if (IS_ERR(name))
>> +			return PTR_ERR(name);
>> +
>> +		root_inode = hostfs_iget(sb, name);
>>   		kfree(name);
>> -		if (err)
>> -			goto out_put;
>> +		if (IS_ERR(root_inode))
>> +			return PTR_ERR(root_inode);
>>   	}
>>   
>> -	err = -ENOMEM;
>>   	sb->s_root = d_make_root(root_inode);
>>   	if (sb->s_root == NULL)
>> -		goto out;
>> +		return -ENOMEM;
>>   
>>   	return 0;
>> -
>> -out_put:
>> -	iput(root_inode);
>> -out:
>> -	return err;
>>   }
>>   
>>   static struct dentry *hostfs_read_sb(struct file_system_type *type,
>> diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
>> index 5ecc4706172b..840619e39a1a 100644
>> --- a/fs/hostfs/hostfs_user.c
>> +++ b/fs/hostfs/hostfs_user.c
>> @@ -36,6 +36,7 @@ static void stat64_to_hostfs(const struct stat64 *buf, struct hostfs_stat *p)
>>   	p->blocks = buf->st_blocks;
>>   	p->maj = os_major(buf->st_rdev);
>>   	p->min = os_minor(buf->st_rdev);
>> +	p->dev = buf->st_dev;
>>   }
>>   
>>   int stat_file(const char *path, struct hostfs_stat *p, int fd)
>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>> index 8e33c4e8ffb8..c1e862a38410 100644
>> --- a/security/landlock/Kconfig
>> +++ b/security/landlock/Kconfig
>> @@ -2,7 +2,7 @@
>>   
>>   config SECURITY_LANDLOCK
>>   	bool "Landlock support"
>> -	depends on SECURITY && !ARCH_EPHEMERAL_INODES
>> +	depends on SECURITY
>>   	select SECURITY_PATH
>>   	help
>>   	  Landlock is a sandboxing mechanism that enables processes to restrict
>