[PATCH 0/4] fs: allow listmount() with reversed ordering

[PATCH 0/4] fs: allow listmount() with reversed ordering

[Christian Brauner]

util-linux is about to implement listmount() and statmount() support.
Karel requested the ability to scan the mount table in backwards order
because that's what libmount currently does in order to get the latest
mount first. We currently don't support this in listmount(). Add a new
LISTMOUNT_RESERVE flag to allow listing mounts in reverse order. For
example, listing all child mounts of /sys without LISTMOUNT_REVERSE
gives:

    /sys/kernel/security @ mnt_id: 4294968369
    /sys/fs/cgroup @ mnt_id: 4294968370
    /sys/firmware/efi/efivars @ mnt_id: 4294968371
    /sys/fs/bpf @ mnt_id: 4294968372
    /sys/kernel/tracing @ mnt_id: 4294968373
    /sys/kernel/debug @ mnt_id: 4294968374
    /sys/fs/fuse/connections @ mnt_id: 4294968375
    /sys/kernel/config @ mnt_id: 4294968376

whereas with LISTMOUNT_RESERVE it gives:

    /sys/kernel/config @ mnt_id: 4294968376
    /sys/fs/fuse/connections @ mnt_id: 4294968375
    /sys/kernel/debug @ mnt_id: 4294968374
    /sys/kernel/tracing @ mnt_id: 4294968373
    /sys/fs/bpf @ mnt_id: 4294968372
    /sys/firmware/efi/efivars @ mnt_id: 4294968371
    /sys/fs/cgroup @ mnt_id: 4294968370
    /sys/kernel/security @ mnt_id: 4294968369

A few smaller cleanups included in this series.

---
---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240607-vfs-listmount-reverse-0f5d4d8248ee

[PATCH 1/4] fs: use semaphore gard in listmount()

[Christian Brauner]

Instead of open-coding the locking use a guard.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 5a51315c6678..72c6e884728b 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5106,7 +5106,8 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, u64 __user *,
 	mnt_parent_id = kreq.mnt_id;
 	last_mnt_id = kreq.param;
 
-	down_read(&namespace_sem);
+	guard(rwsem_read)(&namespace_sem);
+
 	get_fs_root(current->fs, &root);
 	if (mnt_parent_id == LSMT_ROOT) {
 		orig = root;
@@ -5125,7 +5126,6 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, u64 __user *,
 	ret = do_listmount(first, &orig, mnt_parent_id, mnt_ids, nr_mnt_ids, &root);
 err:
 	path_put(&root);
-	up_read(&namespace_sem);
 	return ret;
 }
 

-- 
2.43.0

[PATCH 2/4] path: add cleanup helper

[Christian Brauner]

Add a simple cleanup helper so we can cleanup struct path easily.
No need for any extra machinery. Avoid DEFINE_FREE() as it causes a
local copy of struct path to be used. Just rely on path_put() directly
called from a cleanup helper.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/path.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/path.h b/include/linux/path.h
index 475225a03d0d..ca073e70decd 100644
--- a/include/linux/path.h
+++ b/include/linux/path.h
@@ -24,4 +24,13 @@ static inline void path_put_init(struct path *path)
 	*path = (struct path) { };
 }
 
+/*
+ * Cleanup macro for use with __free(path_put). Avoids dereference and
+ * copying @path unlike DEFINE_FREE(). path_put() will handle the empty
+ * path correctly just ensure @path is initialized:
+ *
+ * struct path path __free(path_put) = {};
+ */
+#define __free_path_put path_put
+
 #endif  /* _LINUX_PATH_H */

-- 
2.43.0

[PATCH 3/4] fs: simplify error handling

[Christian Brauner]

Rely on cleanup helper and simplify error handling

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 72c6e884728b..507f310dbf33 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5083,10 +5083,11 @@ static ssize_t do_listmount(struct mount *first, struct path *orig,
 SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, u64 __user *,
 		mnt_ids, size_t, nr_mnt_ids, unsigned int, flags)
 {
+	struct path root __free(path_put) = {};
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
 	struct mnt_id_req kreq;
 	struct mount *first;
-	struct path root, orig;
+	struct path orig;
 	u64 mnt_parent_id, last_mnt_id;
 	const size_t maxcount = (size_t)-1 >> 3;
 	ssize_t ret;
@@ -5112,10 +5113,9 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, u64 __user *,
 	if (mnt_parent_id == LSMT_ROOT) {
 		orig = root;
 	} else {
-		ret = -ENOENT;
 		orig.mnt = lookup_mnt_in_ns(mnt_parent_id, ns);
 		if (!orig.mnt)
-			goto err;
+			return -ENOENT;
 		orig.dentry = orig.mnt->mnt_root;
 	}
 	if (!last_mnt_id)
@@ -5123,10 +5123,7 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, u64 __user *,
 	else
 		first = mnt_find_id_at(ns, last_mnt_id + 1);
 
-	ret = do_listmount(first, &orig, mnt_parent_id, mnt_ids, nr_mnt_ids, &root);
-err:
-	path_put(&root);
-	return ret;
+	return do_listmount(first, &orig, mnt_parent_id, mnt_ids, nr_mnt_ids, &root);
 }
 
 

-- 
2.43.0

[PATCH 4/4] listmount: allow listing in reverse order

[Christian Brauner]

util-linux is about to implement listmount() and statmount() support.
Karel requested the ability to scan the mount table in backwards order
because that's what libmount currently does in order to get the latest
mount first. We currently don't support this in listmount(). Add a new
LISTMOUNT_RESERVE flag to allow listing mounts in reverse order. For
example, listing all child mounts of /sys without LISTMOUNT_REVERSE
gives:

    /sys/kernel/security @ mnt_id: 4294968369
    /sys/fs/cgroup @ mnt_id: 4294968370
    /sys/firmware/efi/efivars @ mnt_id: 4294968371
    /sys/fs/bpf @ mnt_id: 4294968372
    /sys/kernel/tracing @ mnt_id: 4294968373
    /sys/kernel/debug @ mnt_id: 4294968374
    /sys/fs/fuse/connections @ mnt_id: 4294968375
    /sys/kernel/config @ mnt_id: 4294968376

whereas with LISTMOUNT_RESERVE it gives:

    /sys/kernel/config @ mnt_id: 4294968376
    /sys/fs/fuse/connections @ mnt_id: 4294968375
    /sys/kernel/debug @ mnt_id: 4294968374
    /sys/kernel/tracing @ mnt_id: 4294968373
    /sys/fs/bpf @ mnt_id: 4294968372
    /sys/firmware/efi/efivars @ mnt_id: 4294968371
    /sys/fs/cgroup @ mnt_id: 4294968370
    /sys/kernel/security @ mnt_id: 4294968369

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c             | 62 ++++++++++++++++++++++++++++++++++++++--------
 include/uapi/linux/mount.h |  1 +
 2 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 507f310dbf33..911c149c7979 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1448,6 +1448,30 @@ static struct mount *mnt_find_id_at(struct mnt_namespace *ns, u64 mnt_id)
 	return ret;
 }
 
+/*
+ * Returns the mount which either has the specified mnt_id, or has the next
+ * greater id before the specified one.
+ */
+static struct mount *mnt_find_id_at_reverse(struct mnt_namespace *ns, u64 mnt_id)
+{
+	struct rb_node *node = ns->mounts.rb_node;
+	struct mount *ret = NULL;
+
+	while (node) {
+		struct mount *m = node_to_mount(node);
+
+		if (mnt_id >= m->mnt_id_unique) {
+			ret = node_to_mount(node);
+			if (mnt_id == m->mnt_id_unique)
+				break;
+			node = node->rb_right;
+		} else {
+			node = node->rb_left;
+		}
+	}
+	return ret;
+}
+
 #ifdef CONFIG_PROC_FS
 
 /* iterator; we want it to have access to namespace_sem, thus here... */
@@ -5042,14 +5066,22 @@ SYSCALL_DEFINE4(statmount, const struct mnt_id_req __user *, req,
 	return ret;
 }
 
-static struct mount *listmnt_next(struct mount *curr)
+static struct mount *listmnt_next(struct mount *curr, bool reverse)
 {
-	return node_to_mount(rb_next(&curr->mnt_node));
+	struct rb_node *node;
+
+	if (reverse)
+		node = rb_prev(&curr->mnt_node);
+	else
+		node = rb_next(&curr->mnt_node);
+
+	return node_to_mount(node);
 }
 
 static ssize_t do_listmount(struct mount *first, struct path *orig,
 			    u64 mnt_parent_id, u64 __user *mnt_ids,
-			    size_t nr_mnt_ids, const struct path *root)
+			    size_t nr_mnt_ids, const struct path *root,
+			    bool reverse)
 {
 	struct mount *r;
 	ssize_t ret;
@@ -5066,7 +5098,7 @@ static ssize_t do_listmount(struct mount *first, struct path *orig,
 	if (ret)
 		return ret;
 
-	for (ret = 0, r = first; r && nr_mnt_ids; r = listmnt_next(r)) {
+	for (ret = 0, r = first; r && nr_mnt_ids; r = listmnt_next(r, reverse)) {
 		if (r->mnt_id_unique == mnt_parent_id)
 			continue;
 		if (!is_path_reachable(r, r->mnt.mnt_root, orig))
@@ -5090,9 +5122,10 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, u64 __user *,
 	struct path orig;
 	u64 mnt_parent_id, last_mnt_id;
 	const size_t maxcount = (size_t)-1 >> 3;
+	bool reverse_order;
 	ssize_t ret;
 
-	if (flags)
+	if (flags & ~LISTMOUNT_REVERSE)
 		return -EINVAL;
 
 	if (unlikely(nr_mnt_ids > maxcount))
@@ -5118,12 +5151,21 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, u64 __user *,
 			return -ENOENT;
 		orig.dentry = orig.mnt->mnt_root;
 	}
-	if (!last_mnt_id)
-		first = node_to_mount(rb_first(&ns->mounts));
-	else
-		first = mnt_find_id_at(ns, last_mnt_id + 1);
+	reverse_order = flags & LISTMOUNT_REVERSE;
+	if (!last_mnt_id) {
+		if (reverse_order)
+			first = node_to_mount(rb_last(&ns->mounts));
+		else
+			first = node_to_mount(rb_first(&ns->mounts));
+	} else {
+		if (reverse_order)
+			first = mnt_find_id_at_reverse(ns, last_mnt_id - 1);
+		else
+			first = mnt_find_id_at(ns, last_mnt_id + 1);
+	}
 
-	return do_listmount(first, &orig, mnt_parent_id, mnt_ids, nr_mnt_ids, &root);
+	return do_listmount(first, &orig, mnt_parent_id, mnt_ids, nr_mnt_ids,
+			    &root, reverse_order);
 }
 
 
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index ad5478dbad00..88d78de1519f 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -207,5 +207,6 @@ struct mnt_id_req {
  * Special @mnt_id values that can be passed to listmount
  */
 #define LSMT_ROOT		0xffffffffffffffff	/* root mount */
+#define LISTMOUNT_REVERSE	(1 << 0) /* List later mounts first */
 
 #endif /* _UAPI_LINUX_MOUNT_H */

-- 
2.43.0

Re: [PATCH 0/4] fs: allow listmount() with reversed ordering

[Mateusz Guzik]

On Fri, Jun 07, 2024 at 04:55:33PM +0200, Christian Brauner wrote:
> util-linux is about to implement listmount() and statmount() support.
> Karel requested the ability to scan the mount table in backwards order
> because that's what libmount currently does in order to get the latest
> mount first. We currently don't support this in listmount(). Add a new
> LISTMOUNT_RESERVE flag to allow listing mounts in reverse order. For
> example, listing all child mounts of /sys without LISTMOUNT_REVERSE
> gives:
> 
>     /sys/kernel/security @ mnt_id: 4294968369
>     /sys/fs/cgroup @ mnt_id: 4294968370
>     /sys/firmware/efi/efivars @ mnt_id: 4294968371
>     /sys/fs/bpf @ mnt_id: 4294968372
>     /sys/kernel/tracing @ mnt_id: 4294968373
>     /sys/kernel/debug @ mnt_id: 4294968374
>     /sys/fs/fuse/connections @ mnt_id: 4294968375
>     /sys/kernel/config @ mnt_id: 4294968376
> 
> whereas with LISTMOUNT_RESERVE it gives:
> 
>     /sys/kernel/config @ mnt_id: 4294968376
>     /sys/fs/fuse/connections @ mnt_id: 4294968375
>     /sys/kernel/debug @ mnt_id: 4294968374
>     /sys/kernel/tracing @ mnt_id: 4294968373
>     /sys/fs/bpf @ mnt_id: 4294968372
>     /sys/firmware/efi/efivars @ mnt_id: 4294968371
>     /sys/fs/cgroup @ mnt_id: 4294968370
>     /sys/kernel/security @ mnt_id: 4294968369
> 
> A few smaller cleanups included in this series.
> 

I have some remarks about listmount, they are not patchset-specific
though.

If I'm reading things correctly put_user is performed with namespace_sem
held? While it probably works, it is fundamentally unsound behavior --
said put_user introduces funky lock orderings which don't need to be
there and can go off cpu indefinitely waiting for the fault to be
serviced.

Either the code needs to pre-wire user memory or it needs to export to
userspace in chunks while dropping the lock in-between (say 16 or
whatever IDs per batch, something stack-tolerable).

From cursory reading dropping the lock looks fine.

This has no bearing on the feature, but since you are cleaning up the
syscall apart from it perhaps you will be willing to sort this out.

Bonus question is why lockdep is not yelling about it. At best only
select locks should be allowed to take a page fault with.

Re: [PATCH 0/4] fs: allow listmount() with reversed ordering

[Josef Bacik]

On Fri, Jun 07, 2024 at 04:55:33PM +0200, Christian Brauner wrote:
> util-linux is about to implement listmount() and statmount() support.
> Karel requested the ability to scan the mount table in backwards order
> because that's what libmount currently does in order to get the latest
> mount first. We currently don't support this in listmount(). Add a new
> LISTMOUNT_RESERVE flag to allow listing mounts in reverse order. For
> example, listing all child mounts of /sys without LISTMOUNT_REVERSE
> gives:
> 

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

for the entire series, thanks,

Josef

Re: [PATCH 0/4] fs: allow listmount() with reversed ordering

[Christian Brauner]

> This has no bearing on the feature, but since you are cleaning up the
> syscall apart from it perhaps you will be willing to sort this out.

I'm aware of this and I did rewrite statmount() so it doesn't do that
before we merged it. listmount() can do the same. Please see #vfs.misc.

Re: [PATCH 4/4] listmount: allow listing in reverse order

[Matthew Wilcox]

On Fri, Jun 07, 2024 at 04:55:37PM +0200, Christian Brauner wrote:
> util-linux is about to implement listmount() and statmount() support.
> Karel requested the ability to scan the mount table in backwards order
> because that's what libmount currently does in order to get the latest
> mount first. We currently don't support this in listmount(). Add a new
> LISTMOUNT_RESERVE flag to allow listing mounts in reverse order. For

s/RESERVE/REVERSE/g

> whereas with LISTMOUNT_RESERVE it gives:

Re: [PATCH 4/4] listmount: allow listing in reverse order

[Christian Brauner]

On Wed, Jun 12, 2024 at 02:54:55PM GMT, Matthew Wilcox wrote:
> On Fri, Jun 07, 2024 at 04:55:37PM +0200, Christian Brauner wrote:
> > util-linux is about to implement listmount() and statmount() support.
> > Karel requested the ability to scan the mount table in backwards order
> > because that's what libmount currently does in order to get the latest
> > mount first. We currently don't support this in listmount(). Add a new
> > LISTMOUNT_RESERVE flag to allow listing mounts in reverse order. For
> 
> s/RESERVE/REVERSE/g

Thanks! Fixed!