[PATCH v2 0/8] fs: lockless mntns lookup

[PATCH v2 0/8] fs: lockless mntns lookup

[Christian Brauner]

Hey,

Currently we take the read lock when looking for a mount namespace to
list mounts in. We can make this lockless. The simple search case can
just use a sequence counter to detect concurrent changes to the rbtree.

For walking the list of mount namespaces sequentially via nsfs we keep a
separate rcu list as rb_prev() and rb_next() aren't usable safely with
rcu.

Since creating mount namespaces is a relatively rare event compared with
querying mounts in a foreign mount namespace this is worth it. Once
libmount and systemd pick up this mechanism to list mounts in foreign
mount namespaces this will be used very frequently.
doing.

Thanks!
Christian

---
Changes in v2:
- Remove mnt_ns_find_it_at() by switching to rb_find_rcu().
- Add separate list to lookup sequential mount namespaces.
- Link to v1: https://lore.kernel.org/r/20241210-work-mount-rbtree-lockless-v1-0-338366b9bbe4@kernel.org

---
Christian Brauner (8):
      mount: remove inlude/nospec.h include
      fs: add mount namespace to rbtree late
      fs: lockless mntns rbtree lookup
      rculist: add list_bidir_{del,prev}_rcu()
      fs: lockless mntns lookup for nsfs
      fs: simplify rwlock to spinlock
      selftests: remove unneeded include
      samples: add test-list-all-mounts

 fs/mount.h                            |  20 +--
 fs/namespace.c                        | 158 ++++++++++++++---------
 fs/nsfs.c                             |   5 +-
 include/linux/rculist.h               |  43 +++++++
 samples/vfs/.gitignore                |   1 +
 samples/vfs/Makefile                  |   2 +-
 samples/vfs/test-list-all-mounts.c    | 235 ++++++++++++++++++++++++++++++++++
 tools/testing/selftests/pidfd/pidfd.h |   1 -
 8 files changed, 387 insertions(+), 78 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241207-work-mount-rbtree-lockless-7d4071b74f18

[PATCH v2 1/8] mount: remove inlude/nospec.h include

[Christian Brauner]

It's not needed, so remove it.

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

diff --git a/fs/namespace.c b/fs/namespace.c
index 23e81c2a1e3fee7d97df2a84a69438a677933654..c3dbe6a7ab6b1c77c2693cc75941da89fa921048 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -32,7 +32,6 @@
 #include <linux/fs_context.h>
 #include <linux/shmem_fs.h>
 #include <linux/mnt_idmapping.h>
-#include <linux/nospec.h>
 
 #include "pnode.h"
 #include "internal.h"

-- 
2.45.2

[PATCH v2 2/8] fs: add mount namespace to rbtree late

[Christian Brauner]

There's no point doing that under the namespace semaphore it just gives
the false impression that it protects the mount namespace rbtree and it
simply doesn't.

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

diff --git a/fs/namespace.c b/fs/namespace.c
index c3dbe6a7ab6b1c77c2693cc75941da89fa921048..10fa18dd66018fadfdc9d18c59a851eed7bd55ad 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3983,7 +3983,6 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
 		while (p->mnt.mnt_root != q->mnt.mnt_root)
 			p = next_mnt(skip_mnt_tree(p), old);
 	}
-	mnt_ns_tree_add(new_ns);
 	namespace_unlock();
 
 	if (rootmnt)
@@ -3991,6 +3990,7 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
 	if (pwdmnt)
 		mntput(pwdmnt);
 
+	mnt_ns_tree_add(new_ns);
 	return new_ns;
 }
 

-- 
2.45.2

[PATCH v2 3/8] fs: lockless mntns rbtree lookup

[Christian Brauner]

Currently we use a read-write lock but for the simple search case we can
make this lockless. Creating a new mount namespace is a rather rare
event compared with querying mounts in a foreign mount namespace. Once
this is picked up by e.g., systemd to list mounts in another mount in
it's isolated services or in containers this will be used a lot so this
seems worthwhile doing.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/mount.h     |   5 ++-
 fs/namespace.c | 119 +++++++++++++++++++++++++++++++++++----------------------
 2 files changed, 77 insertions(+), 47 deletions(-)

diff --git a/fs/mount.h b/fs/mount.h
index 185fc56afc13338f8185fe818051444d540cbd5b..3c3763d8ae821d6a117c528808dbc94d0251f964 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -16,7 +16,10 @@ struct mnt_namespace {
 	u64 event;
 	unsigned int		nr_mounts; /* # of mounts in the namespace */
 	unsigned int		pending_mounts;
-	struct rb_node		mnt_ns_tree_node; /* node in the mnt_ns_tree */
+	union {
+		struct rb_node	mnt_ns_tree_node; /* node in the mnt_ns_tree */
+		struct rcu_head mnt_ns_rcu;
+	};
 	refcount_t		passive; /* number references not pinning @mounts */
 } __randomize_layout;
 
diff --git a/fs/namespace.c b/fs/namespace.c
index 10fa18dd66018fadfdc9d18c59a851eed7bd55ad..9463b9ab95f0a5db32cfe5fc5564d7f25ce3e06f 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -79,6 +79,8 @@ static DECLARE_RWSEM(namespace_sem);
 static HLIST_HEAD(unmounted);	/* protected by namespace_sem */
 static LIST_HEAD(ex_mountpoints); /* protected by namespace_sem */
 static DEFINE_RWLOCK(mnt_ns_tree_lock);
+static seqcount_rwlock_t mnt_ns_tree_seqcount = SEQCNT_RWLOCK_ZERO(mnt_ns_tree_seqcount, &mnt_ns_tree_lock);
+
 static struct rb_root mnt_ns_tree = RB_ROOT; /* protected by mnt_ns_tree_lock */
 
 struct mount_kattr {
@@ -105,17 +107,6 @@ EXPORT_SYMBOL_GPL(fs_kobj);
  */
 __cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
 
-static int mnt_ns_cmp(u64 seq, const struct mnt_namespace *ns)
-{
-	u64 seq_b = ns->seq;
-
-	if (seq < seq_b)
-		return -1;
-	if (seq > seq_b)
-		return 1;
-	return 0;
-}
-
 static inline struct mnt_namespace *node_to_mnt_ns(const struct rb_node *node)
 {
 	if (!node)
@@ -123,19 +114,41 @@ static inline struct mnt_namespace *node_to_mnt_ns(const struct rb_node *node)
 	return rb_entry(node, struct mnt_namespace, mnt_ns_tree_node);
 }
 
-static bool mnt_ns_less(struct rb_node *a, const struct rb_node *b)
+static int mnt_ns_cmp(struct rb_node *a, const struct rb_node *b)
 {
 	struct mnt_namespace *ns_a = node_to_mnt_ns(a);
 	struct mnt_namespace *ns_b = node_to_mnt_ns(b);
 	u64 seq_a = ns_a->seq;
+	u64 seq_b = ns_b->seq;
+
+	if (seq_a < seq_b)
+		return -1;
+	if (seq_a > seq_b)
+		return 1;
+	return 0;
+}
 
-	return mnt_ns_cmp(seq_a, ns_b) < 0;
+static inline void mnt_ns_tree_write_lock(void)
+{
+	write_lock(&mnt_ns_tree_lock);
+	write_seqcount_begin(&mnt_ns_tree_seqcount);
+}
+
+static inline void mnt_ns_tree_write_unlock(void)
+{
+	write_seqcount_end(&mnt_ns_tree_seqcount);
+	write_unlock(&mnt_ns_tree_lock);
 }
 
 static void mnt_ns_tree_add(struct mnt_namespace *ns)
 {
-	guard(write_lock)(&mnt_ns_tree_lock);
-	rb_add(&ns->mnt_ns_tree_node, &mnt_ns_tree, mnt_ns_less);
+	struct rb_node *node;
+
+	mnt_ns_tree_write_lock();
+	node = rb_find_add_rcu(&ns->mnt_ns_tree_node, &mnt_ns_tree, mnt_ns_cmp);
+	mnt_ns_tree_write_unlock();
+
+	WARN_ON_ONCE(node);
 }
 
 static void mnt_ns_release(struct mnt_namespace *ns)
@@ -150,41 +163,36 @@ static void mnt_ns_release(struct mnt_namespace *ns)
 }
 DEFINE_FREE(mnt_ns_release, struct mnt_namespace *, if (_T) mnt_ns_release(_T))
 
+static void mnt_ns_release_rcu(struct rcu_head *rcu)
+{
+	struct mnt_namespace *mnt_ns;
+
+	mnt_ns = container_of(rcu, struct mnt_namespace, mnt_ns_rcu);
+	mnt_ns_release(mnt_ns);
+}
+
 static void mnt_ns_tree_remove(struct mnt_namespace *ns)
 {
 	/* remove from global mount namespace list */
 	if (!is_anon_ns(ns)) {
-		guard(write_lock)(&mnt_ns_tree_lock);
+		mnt_ns_tree_write_lock();
 		rb_erase(&ns->mnt_ns_tree_node, &mnt_ns_tree);
+		mnt_ns_tree_write_unlock();
 	}
 
-	mnt_ns_release(ns);
+	call_rcu(&ns->mnt_ns_rcu, mnt_ns_release_rcu);
 }
 
-/*
- * Returns the mount namespace which either has the specified id, or has the
- * next smallest id afer the specified one.
- */
-static struct mnt_namespace *mnt_ns_find_id_at(u64 mnt_ns_id)
+static int mnt_ns_find(const void *key, const struct rb_node *node)
 {
-	struct rb_node *node = mnt_ns_tree.rb_node;
-	struct mnt_namespace *ret = NULL;
-
-	lockdep_assert_held(&mnt_ns_tree_lock);
-
-	while (node) {
-		struct mnt_namespace *n = node_to_mnt_ns(node);
+	const u64 mnt_ns_id = *(u64 *)key;
+	const struct mnt_namespace *ns = node_to_mnt_ns(node);
 
-		if (mnt_ns_id <= n->seq) {
-			ret = node_to_mnt_ns(node);
-			if (mnt_ns_id == n->seq)
-				break;
-			node = node->rb_left;
-		} else {
-			node = node->rb_right;
-		}
-	}
-	return ret;
+	if (mnt_ns_id < ns->seq)
+		return -1;
+	if (mnt_ns_id > ns->seq)
+		return 1;
+	return 0;
 }
 
 /*
@@ -194,18 +202,37 @@ static struct mnt_namespace *mnt_ns_find_id_at(u64 mnt_ns_id)
  * namespace the @namespace_sem must first be acquired. If the namespace has
  * already shut down before acquiring @namespace_sem, {list,stat}mount() will
  * see that the mount rbtree of the namespace is empty.
+ *
+ * Note the lookup is lockless protected by a sequence counter. We only
+ * need to guard against false negatives as false positives aren't
+ * possible. So if we didn't find a mount namespace and the sequence
+ * counter has changed we need to retry. If the sequence counter is
+ * still the same we know the search actually failed.
  */
 static struct mnt_namespace *lookup_mnt_ns(u64 mnt_ns_id)
 {
-       struct mnt_namespace *ns;
+	struct mnt_namespace *ns;
+	struct rb_node *node;
+	unsigned int seq;
+
+	guard(rcu)();
+	do {
+		seq = read_seqcount_begin(&mnt_ns_tree_seqcount);
+		node = rb_find_rcu(&mnt_ns_id, &mnt_ns_tree, mnt_ns_find);
+		if (node)
+			break;
+	} while (read_seqcount_retry(&mnt_ns_tree_seqcount, seq));
 
-       guard(read_lock)(&mnt_ns_tree_lock);
-       ns = mnt_ns_find_id_at(mnt_ns_id);
-       if (!ns || ns->seq != mnt_ns_id)
-               return NULL;
+	if (!node)
+		return NULL;
 
-       refcount_inc(&ns->passive);
-       return ns;
+	/*
+	 * The last reference count is put with after RCU delay so we
+	 * don't need to use refcount_inc_not_zero().
+	 */
+	ns = node_to_mnt_ns(node);
+	refcount_inc(&ns->passive);
+	return ns;
 }
 
 static inline void lock_mount_hash(void)

-- 
2.45.2

Re: [PATCH v2 3/8] fs: lockless mntns rbtree lookup

[Peter Zijlstra]

On Thu, Dec 12, 2024 at 12:56:02PM +0100, Christian Brauner wrote:


> @@ -16,7 +16,10 @@ struct mnt_namespace {
>  	u64 event;
>  	unsigned int		nr_mounts; /* # of mounts in the namespace */
>  	unsigned int		pending_mounts;
> -	struct rb_node		mnt_ns_tree_node; /* node in the mnt_ns_tree */
> +	union {
> +		struct rb_node	mnt_ns_tree_node; /* node in the mnt_ns_tree */
> +		struct rcu_head mnt_ns_rcu;
> +	};
>  	refcount_t		passive; /* number references not pinning @mounts */
>  } __randomize_layout;

>  static void mnt_ns_tree_remove(struct mnt_namespace *ns)
>  {
>  	/* remove from global mount namespace list */
>  	if (!is_anon_ns(ns)) {
> -		guard(write_lock)(&mnt_ns_tree_lock);
> +		mnt_ns_tree_write_lock();
>  		rb_erase(&ns->mnt_ns_tree_node, &mnt_ns_tree);
> +		mnt_ns_tree_write_unlock();
>  	}
>  
> -	mnt_ns_release(ns);
> +	call_rcu(&ns->mnt_ns_rcu, mnt_ns_release_rcu);
>  }

I'm not sure that union is sane; the above means you're overwriting the
tree node while a concurrent lookup might still see the node and want to
decent from it.

Re: [PATCH v2 3/8] fs: lockless mntns rbtree lookup

[Christian Brauner]

On Thu, Dec 12, 2024 at 01:09:29PM +0100, Peter Zijlstra wrote:
> On Thu, Dec 12, 2024 at 12:56:02PM +0100, Christian Brauner wrote:
> 
> 
> > @@ -16,7 +16,10 @@ struct mnt_namespace {
> >  	u64 event;
> >  	unsigned int		nr_mounts; /* # of mounts in the namespace */
> >  	unsigned int		pending_mounts;
> > -	struct rb_node		mnt_ns_tree_node; /* node in the mnt_ns_tree */
> > +	union {
> > +		struct rb_node	mnt_ns_tree_node; /* node in the mnt_ns_tree */
> > +		struct rcu_head mnt_ns_rcu;
> > +	};
> >  	refcount_t		passive; /* number references not pinning @mounts */
> >  } __randomize_layout;
> 
> >  static void mnt_ns_tree_remove(struct mnt_namespace *ns)
> >  {
> >  	/* remove from global mount namespace list */
> >  	if (!is_anon_ns(ns)) {
> > -		guard(write_lock)(&mnt_ns_tree_lock);
> > +		mnt_ns_tree_write_lock();
> >  		rb_erase(&ns->mnt_ns_tree_node, &mnt_ns_tree);
> > +		mnt_ns_tree_write_unlock();
> >  	}
> >  
> > -	mnt_ns_release(ns);
> > +	call_rcu(&ns->mnt_ns_rcu, mnt_ns_release_rcu);
> >  }
> 
> I'm not sure that union is sane; the above means you're overwriting the
> tree node while a concurrent lookup might still see the node and want to
> decent from it.

Though, you're right. I'll fix this up. Thanks for the reviews!

Re: [PATCH v2 3/8] fs: lockless mntns rbtree lookup

[Jeff Layton]

On Thu, 2024-12-12 at 12:56 +0100, Christian Brauner wrote:
> Currently we use a read-write lock but for the simple search case we can
> make this lockless. Creating a new mount namespace is a rather rare
> event compared with querying mounts in a foreign mount namespace. Once
> this is picked up by e.g., systemd to list mounts in another mount in
> it's isolated services or in containers this will be used a lot so this
> seems worthwhile doing.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/mount.h     |   5 ++-
>  fs/namespace.c | 119 +++++++++++++++++++++++++++++++++++----------------------
>  2 files changed, 77 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/mount.h b/fs/mount.h
> index 185fc56afc13338f8185fe818051444d540cbd5b..3c3763d8ae821d6a117c528808dbc94d0251f964 100644
> --- a/fs/mount.h
> +++ b/fs/mount.h
> @@ -16,7 +16,10 @@ struct mnt_namespace {
>  	u64 event;
>  	unsigned int		nr_mounts; /* # of mounts in the namespace */
>  	unsigned int		pending_mounts;
> -	struct rb_node		mnt_ns_tree_node; /* node in the mnt_ns_tree */
> +	union {
> +		struct rb_node	mnt_ns_tree_node; /* node in the mnt_ns_tree */
> +		struct rcu_head mnt_ns_rcu;
> +	};

I second Peter's concerns about the union here. You really want to
union the rcu structure with something that isn't ever touched during a
search for the object.

>  	refcount_t		passive; /* number references not pinning @mounts */
>  } __randomize_layout;
>  
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 10fa18dd66018fadfdc9d18c59a851eed7bd55ad..9463b9ab95f0a5db32cfe5fc5564d7f25ce3e06f 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -79,6 +79,8 @@ static DECLARE_RWSEM(namespace_sem);
>  static HLIST_HEAD(unmounted);	/* protected by namespace_sem */
>  static LIST_HEAD(ex_mountpoints); /* protected by namespace_sem */
>  static DEFINE_RWLOCK(mnt_ns_tree_lock);
> +static seqcount_rwlock_t mnt_ns_tree_seqcount = SEQCNT_RWLOCK_ZERO(mnt_ns_tree_seqcount, &mnt_ns_tree_lock);
> +
>  static struct rb_root mnt_ns_tree = RB_ROOT; /* protected by mnt_ns_tree_lock */
>  
>  struct mount_kattr {
> @@ -105,17 +107,6 @@ EXPORT_SYMBOL_GPL(fs_kobj);
>   */
>  __cacheline_aligned_in_smp DEFINE_SEQLOCK(mount_lock);
>  
> -static int mnt_ns_cmp(u64 seq, const struct mnt_namespace *ns)
> -{
> -	u64 seq_b = ns->seq;
> -
> -	if (seq < seq_b)
> -		return -1;
> -	if (seq > seq_b)
> -		return 1;
> -	return 0;
> -}
> -
>  static inline struct mnt_namespace *node_to_mnt_ns(const struct rb_node *node)
>  {
>  	if (!node)
> @@ -123,19 +114,41 @@ static inline struct mnt_namespace *node_to_mnt_ns(const struct rb_node *node)
>  	return rb_entry(node, struct mnt_namespace, mnt_ns_tree_node);
>  }
>  
> -static bool mnt_ns_less(struct rb_node *a, const struct rb_node *b)
> +static int mnt_ns_cmp(struct rb_node *a, const struct rb_node *b)
>  {
>  	struct mnt_namespace *ns_a = node_to_mnt_ns(a);
>  	struct mnt_namespace *ns_b = node_to_mnt_ns(b);
>  	u64 seq_a = ns_a->seq;
> +	u64 seq_b = ns_b->seq;
> +
> +	if (seq_a < seq_b)
> +		return -1;
> +	if (seq_a > seq_b)
> +		return 1;
> +	return 0;
> +}
>  
> -	return mnt_ns_cmp(seq_a, ns_b) < 0;
> +static inline void mnt_ns_tree_write_lock(void)
> +{
> +	write_lock(&mnt_ns_tree_lock);
> +	write_seqcount_begin(&mnt_ns_tree_seqcount);
> +}
> +
> +static inline void mnt_ns_tree_write_unlock(void)
> +{
> +	write_seqcount_end(&mnt_ns_tree_seqcount);
> +	write_unlock(&mnt_ns_tree_lock);
>  }
>  
>  static void mnt_ns_tree_add(struct mnt_namespace *ns)
>  {
> -	guard(write_lock)(&mnt_ns_tree_lock);
> -	rb_add(&ns->mnt_ns_tree_node, &mnt_ns_tree, mnt_ns_less);
> +	struct rb_node *node;
> +
> +	mnt_ns_tree_write_lock();
> +	node = rb_find_add_rcu(&ns->mnt_ns_tree_node, &mnt_ns_tree, mnt_ns_cmp);
> +	mnt_ns_tree_write_unlock();
> +
> +	WARN_ON_ONCE(node);
>  }
>  
>  static void mnt_ns_release(struct mnt_namespace *ns)
> @@ -150,41 +163,36 @@ static void mnt_ns_release(struct mnt_namespace *ns)
>  }
>  DEFINE_FREE(mnt_ns_release, struct mnt_namespace *, if (_T) mnt_ns_release(_T))
>  
> +static void mnt_ns_release_rcu(struct rcu_head *rcu)
> +{
> +	struct mnt_namespace *mnt_ns;
> +
> +	mnt_ns = container_of(rcu, struct mnt_namespace, mnt_ns_rcu);
> +	mnt_ns_release(mnt_ns);
> +}
> +
>  static void mnt_ns_tree_remove(struct mnt_namespace *ns)
>  {
>  	/* remove from global mount namespace list */
>  	if (!is_anon_ns(ns)) {
> -		guard(write_lock)(&mnt_ns_tree_lock);
> +		mnt_ns_tree_write_lock();
>  		rb_erase(&ns->mnt_ns_tree_node, &mnt_ns_tree);
> +		mnt_ns_tree_write_unlock();
>  	}
>  
> -	mnt_ns_release(ns);
> +	call_rcu(&ns->mnt_ns_rcu, mnt_ns_release_rcu);
>  }
>  
> -/*
> - * Returns the mount namespace which either has the specified id, or has the
> - * next smallest id afer the specified one.
> - */
> -static struct mnt_namespace *mnt_ns_find_id_at(u64 mnt_ns_id)
> +static int mnt_ns_find(const void *key, const struct rb_node *node)
>  {
> -	struct rb_node *node = mnt_ns_tree.rb_node;
> -	struct mnt_namespace *ret = NULL;
> -
> -	lockdep_assert_held(&mnt_ns_tree_lock);
> -
> -	while (node) {
> -		struct mnt_namespace *n = node_to_mnt_ns(node);
> +	const u64 mnt_ns_id = *(u64 *)key;
> +	const struct mnt_namespace *ns = node_to_mnt_ns(node);
>  
> -		if (mnt_ns_id <= n->seq) {
> -			ret = node_to_mnt_ns(node);
> -			if (mnt_ns_id == n->seq)
> -				break;
> -			node = node->rb_left;
> -		} else {
> -			node = node->rb_right;
> -		}
> -	}
> -	return ret;
> +	if (mnt_ns_id < ns->seq)
> +		return -1;
> +	if (mnt_ns_id > ns->seq)
> +		return 1;
> +	return 0;
>  }
>  
>  /*
> @@ -194,18 +202,37 @@ static struct mnt_namespace *mnt_ns_find_id_at(u64 mnt_ns_id)
>   * namespace the @namespace_sem must first be acquired. If the namespace has
>   * already shut down before acquiring @namespace_sem, {list,stat}mount() will
>   * see that the mount rbtree of the namespace is empty.
> + *
> + * Note the lookup is lockless protected by a sequence counter. We only
> + * need to guard against false negatives as false positives aren't
> + * possible. So if we didn't find a mount namespace and the sequence
> + * counter has changed we need to retry. If the sequence counter is
> + * still the same we know the search actually failed.
>   */
>  static struct mnt_namespace *lookup_mnt_ns(u64 mnt_ns_id)
>  {
> -       struct mnt_namespace *ns;
> +	struct mnt_namespace *ns;
> +	struct rb_node *node;
> +	unsigned int seq;
> +
> +	guard(rcu)();
> +	do {
> +		seq = read_seqcount_begin(&mnt_ns_tree_seqcount);
> +		node = rb_find_rcu(&mnt_ns_id, &mnt_ns_tree, mnt_ns_find);
> +		if (node)
> +			break;
> +	} while (read_seqcount_retry(&mnt_ns_tree_seqcount, seq));

I don't really get the need for a seqcount here. Typically we use those
when you could potentially find an object that's in some sort of
inconsistent state.

Is that the case here? It seems like you'd either find the object or
not. It seems like either outcome is OK without the need to retry the
search?

>  
> -       guard(read_lock)(&mnt_ns_tree_lock);
> -       ns = mnt_ns_find_id_at(mnt_ns_id);
> -       if (!ns || ns->seq != mnt_ns_id)
> -               return NULL;
> +	if (!node)
> +		return NULL;
>  
> -       refcount_inc(&ns->passive);
> -       return ns;
> +	/*
> +	 * The last reference count is put with after RCU delay so we
> +	 * don't need to use refcount_inc_not_zero().
> +	 */
> +	ns = node_to_mnt_ns(node);
> +	refcount_inc(&ns->passive);
> +	return ns;
>  }
>  
>  static inline void lock_mount_hash(void)
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 3/8] fs: lockless mntns rbtree lookup

[Christian Brauner]

> > +	guard(rcu)();
> > +	do {
> > +		seq = read_seqcount_begin(&mnt_ns_tree_seqcount);
> > +		node = rb_find_rcu(&mnt_ns_id, &mnt_ns_tree, mnt_ns_find);
> > +		if (node)
> > +			break;
> > +	} while (read_seqcount_retry(&mnt_ns_tree_seqcount, seq));
> 
> I don't really get the need for a seqcount here. Typically we use those
> when you could potentially find an object that's in some sort of
> inconsistent state.
> 
> Is that the case here? It seems like you'd either find the object or
> not. It seems like either outcome is OK without the need to retry the
> search?

The sequence count is to detect concurrent changes to the rbtree. As a
change to the rbtree might cause us to miss whole subtrees as it could
be rotated. So it's not about the object but about the rbtree.

[PATCH v2 4/8] rculist: add list_bidir_{del,prev}_rcu()

[Christian Brauner]

Currently there is no primite for retrieving the previous list member.
To do this we need a new deletion primite that doesn't poison the prev
pointer and a corresponding retrieval helper. Note that it is not valid
to ues both list_del_rcu() and list_bidir_del_rcu() on the same list.

Suggested-by: "Paul E. McKenney" <paulmck@kernel.org>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/rculist.h | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index 14dfa6008467e803d57f98cfa0275569f1c6a181..c81f9e5a789928ae6825c89325396d638b3e48c5 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -30,6 +30,14 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
  * way, we must not access it directly
  */
 #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
+/*
+ * Return the ->prev pointer of a list_head in an rcu safe way. Don't
+ * access it directly.
+ *
+ * In order to use list_bidir_prev_rcu() deletions must only be done via
+ * list_bidir_del() to avoid poisoning the ->prev pointer.
+ */
+#define list_bidir_prev_rcu(list) (*((struct list_head __rcu **)(&(list)->prev)))
 
 /**
  * list_tail_rcu - returns the prev pointer of the head of the list
@@ -158,6 +166,41 @@ static inline void list_del_rcu(struct list_head *entry)
 	entry->prev = LIST_POISON2;
 }
 
+/**
+ * list_bidir_del_rcu - deletes entry from list without re-initialization
+ * @entry: the element to delete from the list.
+ *
+ * In contrat to list_del_rcu() doesn't poison the previous pointer thus
+ * allowing to go backwards via list_prev_bidir_rcu().
+ *
+ * Note: list_empty() on entry does not return true after this,
+ * the entry is in an undefined state. It is useful for RCU based
+ * lockfree traversal.
+ *
+ * In particular, it means that we can not poison the forward
+ * pointers that may still be used for walking the list.
+ *
+ * The caller must take whatever precautions are necessary
+ * (such as holding appropriate locks) to avoid racing
+ * with another list-mutation primitive, such as list_bidir_del_rcu()
+ * or list_add_rcu(), running on this same list.
+ * However, it is perfectly legal to run concurrently with
+ * the _rcu list-traversal primitives, such as
+ * list_for_each_entry_rcu().
+ *
+ * Noe that the it is not allowed to use list_del_rcu() and
+ * list_bidir_del_rcu() on the same list.
+ *
+ * Note that the caller is not permitted to immediately free
+ * the newly deleted entry.  Instead, either synchronize_rcu()
+ * or call_rcu() must be used to defer freeing until an RCU
+ * grace period has elapsed.
+ */
+static inline void list_bidir_del_rcu(struct list_head *entry)
+{
+	__list_del_entry(entry);
+}
+
 /**
  * hlist_del_init_rcu - deletes entry from hash list with re-initialization
  * @n: the element to delete from the hash list.

-- 
2.45.2

Re: [PATCH v2 4/8] rculist: add list_bidir_{del,prev}_rcu()

[Paul E. McKenney]

On Thu, Dec 12, 2024 at 12:56:03PM +0100, Christian Brauner wrote:
> Currently there is no primite for retrieving the previous list member.

s/primite/primitive/g

To my surprise, there is an English word "primite".  According to Merriam
Webster, this is "the anterior member of a pair of gregarines in syzygy".
I fervently hope not to have much opportunity to use this word, especially
in reference to myself.  But I cannot escape the suspicion that Merriam
Webster might be engaging in a little trolling.  ;-)

> To do this we need a new deletion primite that doesn't poison the prev
> pointer and a corresponding retrieval helper. Note that it is not valid
> to ues both list_del_rcu() and list_bidir_del_rcu() on the same list.
> 
> Suggested-by: "Paul E. McKenney" <paulmck@kernel.org>
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good!  I have a few suggestions below, mostly grammar nits.

							Thanx, Paul

> ---
>  include/linux/rculist.h | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index 14dfa6008467e803d57f98cfa0275569f1c6a181..c81f9e5a789928ae6825c89325396d638b3e48c5 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -30,6 +30,14 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
>   * way, we must not access it directly
>   */
>  #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
> +/*
> + * Return the ->prev pointer of a list_head in an rcu safe way. Don't
> + * access it directly.
> + *
> + * In order to use list_bidir_prev_rcu() deletions must only be done via
> + * list_bidir_del() to avoid poisoning the ->prev pointer.

This should be list_bidir_del_rcu(), right?  If so, I suggest wording
this as follows or similar:

 * Any list traversed with list_bidir_prev_rcu() must never use
 * list_del_rcu().  Doing so will poison the ->prev pointer that
 * list_bidir_prev_rcu() relies on, which will result in segfaults.
 * To prevent these segfaults, use list_bidir_del_rcu() instead
 * of list_del_rcu().

> + */
> +#define list_bidir_prev_rcu(list) (*((struct list_head __rcu **)(&(list)->prev)))

We need a rcu_dereference() in there somewhere, otherwise the compiler
might ruin your day.

Huh.  You (quite reasonably) copy-pasta'd list_next_rcu().  So the
restriction is the same, the caller must use rcu_dereference.  Unless,
like seq_list_next_rcu(), you are never dereferencing it.  That said,
I am not so sure about the callers of unloaded_tainted_modules_seq_next()
and rxrpc_call_seq_next(), which inherit the same restriction.

If those two are used properly with rcu_dereference(), we have empirical
evidence indicating that things might be OK.  Otherwise, both need at
least an upgrade of their header comments.  ;-)

>  /**
>   * list_tail_rcu - returns the prev pointer of the head of the list
> @@ -158,6 +166,41 @@ static inline void list_del_rcu(struct list_head *entry)
>  	entry->prev = LIST_POISON2;
>  }
>  
> +/**
> + * list_bidir_del_rcu - deletes entry from list without re-initialization
> + * @entry: the element to delete from the list.
> + *
> + * In contrat to list_del_rcu() doesn't poison the previous pointer thus

Looks good, but while I am here, I might as well nitpick...

"In constrast".

> + * allowing to go backwards via list_prev_bidir_rcu().

"allowing backwards traversal via"

> + * Note: list_empty() on entry does not return true after this,
> + * the entry is in an undefined state. It is useful for RCU based

"because the entry is in a special undefined state that permits
RCU-based lockfree reverse traversal."

> + * lockfree traversal.

At which point, you don't need this paragraph break.

> + * In particular, it means that we can not poison the forward

"this means that ... forward and backwards"

> + * pointers that may still be used for walking the list.
> + *
> + * The caller must take whatever precautions are necessary
> + * (such as holding appropriate locks) to avoid racing
> + * with another list-mutation primitive, such as list_bidir_del_rcu()
> + * or list_add_rcu(), running on this same list.
> + * However, it is perfectly legal to run concurrently with
> + * the _rcu list-traversal primitives, such as
> + * list_for_each_entry_rcu().
> + *
> + * Noe that the it is not allowed to use list_del_rcu() and

"Note that list_del_rcu() and list_bidir_del_rcu() must not be used on
the same list at the same time."

If you want to leave off the "at the same time", I am good.  One could
argue that we should not call attention to the possibility of adding
this sort of complexity.  Let them need it badly first.  ;-)

> + * list_bidir_del_rcu() on the same list.
> + *
> + * Note that the caller is not permitted to immediately free
> + * the newly deleted entry.  Instead, either synchronize_rcu()
> + * or call_rcu() must be used to defer freeing until an RCU
> + * grace period has elapsed.
> + */
> +static inline void list_bidir_del_rcu(struct list_head *entry)
> +{
> +	__list_del_entry(entry);
> +}
> +
>  /**
>   * hlist_del_init_rcu - deletes entry from hash list with re-initialization
>   * @n: the element to delete from the hash list.
> 
> -- 
> 2.45.2
> 

Re: [PATCH v2 4/8] rculist: add list_bidir_{del,prev}_rcu()

[Christian Brauner]

On Thu, Dec 12, 2024 at 07:18:44AM -0800, Paul E. McKenney wrote:
> On Thu, Dec 12, 2024 at 12:56:03PM +0100, Christian Brauner wrote:
> > Currently there is no primite for retrieving the previous list member.
> 
> s/primite/primitive/g
> 
> To my surprise, there is an English word "primite".  According to Merriam
> Webster, this is "the anterior member of a pair of gregarines in syzygy".
> I fervently hope not to have much opportunity to use this word, especially
> in reference to myself.  But I cannot escape the suspicion that Merriam
> Webster might be engaging in a little trolling.  ;-)

:)

> 
> > To do this we need a new deletion primite that doesn't poison the prev
> > pointer and a corresponding retrieval helper. Note that it is not valid
> > to ues both list_del_rcu() and list_bidir_del_rcu() on the same list.
> > 
> > Suggested-by: "Paul E. McKenney" <paulmck@kernel.org>
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> Looks good!  I have a few suggestions below, mostly grammar nits.

Yes to all.

> 
> 							Thanx, Paul
> 
> > ---
> >  include/linux/rculist.h | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> > 
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index 14dfa6008467e803d57f98cfa0275569f1c6a181..c81f9e5a789928ae6825c89325396d638b3e48c5 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -30,6 +30,14 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> >   * way, we must not access it directly
> >   */
> >  #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
> > +/*
> > + * Return the ->prev pointer of a list_head in an rcu safe way. Don't
> > + * access it directly.
> > + *
> > + * In order to use list_bidir_prev_rcu() deletions must only be done via
> > + * list_bidir_del() to avoid poisoning the ->prev pointer.
> 
> This should be list_bidir_del_rcu(), right?  If so, I suggest wording
> this as follows or similar:
> 
>  * Any list traversed with list_bidir_prev_rcu() must never use
>  * list_del_rcu().  Doing so will poison the ->prev pointer that
>  * list_bidir_prev_rcu() relies on, which will result in segfaults.
>  * To prevent these segfaults, use list_bidir_del_rcu() instead
>  * of list_del_rcu().
> 
> > + */
> > +#define list_bidir_prev_rcu(list) (*((struct list_head __rcu **)(&(list)->prev)))
> 
> We need a rcu_dereference() in there somewhere, otherwise the compiler
> might ruin your day.
> 
> Huh.  You (quite reasonably) copy-pasta'd list_next_rcu().  So the

I expect the caller to do the rcu_dereference() just like with
list_next_rcu():

At first I made it include a rcu_dereference. But then I realized that
the api becomes rather wonky because list_next_rcu() would have to be
called with rcu_dereference() (most caller's do) and
list_bidir_prev_rcu() wouldn't.

That seemed very confusing to me so I just kept it aligned with
list_next_rcu(). Now, the correct follow-up cleanup would obviously be
to port both helpers to include the rcu_dereference() and thereby simply
all callers. For the few (mostly inside the header itself) we could just
add a __list_next_rcu() thing that doesn't include the
rcu_dereference().

> restriction is the same, the caller must use rcu_dereference.  Unless,
> like seq_list_next_rcu(), you are never dereferencing it.  That said,
> I am not so sure about the callers of unloaded_tainted_modules_seq_next()
> and rxrpc_call_seq_next(), which inherit the same restriction.
> 
> If those two are used properly with rcu_dereference(), we have empirical
> evidence indicating that things might be OK.  Otherwise, both need at
> least an upgrade of their header comments.  ;-)
> 
> >  /**
> >   * list_tail_rcu - returns the prev pointer of the head of the list
> > @@ -158,6 +166,41 @@ static inline void list_del_rcu(struct list_head *entry)
> >  	entry->prev = LIST_POISON2;
> >  }
> >  
> > +/**
> > + * list_bidir_del_rcu - deletes entry from list without re-initialization
> > + * @entry: the element to delete from the list.
> > + *
> > + * In contrat to list_del_rcu() doesn't poison the previous pointer thus
> 
> Looks good, but while I am here, I might as well nitpick...
> 
> "In constrast".
> 
> > + * allowing to go backwards via list_prev_bidir_rcu().
> 
> "allowing backwards traversal via"
> 
> > + * Note: list_empty() on entry does not return true after this,
> > + * the entry is in an undefined state. It is useful for RCU based
> 
> "because the entry is in a special undefined state that permits
> RCU-based lockfree reverse traversal."
> 
> > + * lockfree traversal.
> 
> At which point, you don't need this paragraph break.
> 
> > + * In particular, it means that we can not poison the forward
> 
> "this means that ... forward and backwards"
> 
> > + * pointers that may still be used for walking the list.
> > + *
> > + * The caller must take whatever precautions are necessary
> > + * (such as holding appropriate locks) to avoid racing
> > + * with another list-mutation primitive, such as list_bidir_del_rcu()
> > + * or list_add_rcu(), running on this same list.
> > + * However, it is perfectly legal to run concurrently with
> > + * the _rcu list-traversal primitives, such as
> > + * list_for_each_entry_rcu().
> > + *
> > + * Noe that the it is not allowed to use list_del_rcu() and
> 
> "Note that list_del_rcu() and list_bidir_del_rcu() must not be used on
> the same list at the same time."
> 
> If you want to leave off the "at the same time", I am good.  One could
> argue that we should not call attention to the possibility of adding
> this sort of complexity.  Let them need it badly first.  ;-)
> 
> > + * list_bidir_del_rcu() on the same list.
> > + *
> > + * Note that the caller is not permitted to immediately free
> > + * the newly deleted entry.  Instead, either synchronize_rcu()
> > + * or call_rcu() must be used to defer freeing until an RCU
> > + * grace period has elapsed.
> > + */
> > +static inline void list_bidir_del_rcu(struct list_head *entry)
> > +{
> > +	__list_del_entry(entry);
> > +}
> > +
> >  /**
> >   * hlist_del_init_rcu - deletes entry from hash list with re-initialization
> >   * @n: the element to delete from the hash list.
> > 
> > -- 
> > 2.45.2
> > 

Re: [PATCH v2 4/8] rculist: add list_bidir_{del,prev}_rcu()

[Paul E. McKenney]

On Thu, Dec 12, 2024 at 06:40:48PM +0100, Christian Brauner wrote:
> On Thu, Dec 12, 2024 at 07:18:44AM -0800, Paul E. McKenney wrote:
> > On Thu, Dec 12, 2024 at 12:56:03PM +0100, Christian Brauner wrote:
> > > Currently there is no primite for retrieving the previous list member.
> > 
> > s/primite/primitive/g
> > 
> > To my surprise, there is an English word "primite".  According to Merriam
> > Webster, this is "the anterior member of a pair of gregarines in syzygy".
> > I fervently hope not to have much opportunity to use this word, especially
> > in reference to myself.  But I cannot escape the suspicion that Merriam
> > Webster might be engaging in a little trolling.  ;-)
> 
> :)
> 
> > 
> > > To do this we need a new deletion primite that doesn't poison the prev
> > > pointer and a corresponding retrieval helper. Note that it is not valid
> > > to ues both list_del_rcu() and list_bidir_del_rcu() on the same list.
> > > 
> > > Suggested-by: "Paul E. McKenney" <paulmck@kernel.org>
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > 
> > Looks good!  I have a few suggestions below, mostly grammar nits.
> 
> Yes to all.
> 
> > 
> > 							Thanx, Paul
> > 
> > > ---
> > >  include/linux/rculist.h | 43 +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 43 insertions(+)
> > > 
> > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > index 14dfa6008467e803d57f98cfa0275569f1c6a181..c81f9e5a789928ae6825c89325396d638b3e48c5 100644
> > > --- a/include/linux/rculist.h
> > > +++ b/include/linux/rculist.h
> > > @@ -30,6 +30,14 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > >   * way, we must not access it directly
> > >   */
> > >  #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
> > > +/*
> > > + * Return the ->prev pointer of a list_head in an rcu safe way. Don't
> > > + * access it directly.
> > > + *
> > > + * In order to use list_bidir_prev_rcu() deletions must only be done via
> > > + * list_bidir_del() to avoid poisoning the ->prev pointer.
> > 
> > This should be list_bidir_del_rcu(), right?  If so, I suggest wording
> > this as follows or similar:
> > 
> >  * Any list traversed with list_bidir_prev_rcu() must never use
> >  * list_del_rcu().  Doing so will poison the ->prev pointer that
> >  * list_bidir_prev_rcu() relies on, which will result in segfaults.
> >  * To prevent these segfaults, use list_bidir_del_rcu() instead
> >  * of list_del_rcu().
> > 
> > > + */
> > > +#define list_bidir_prev_rcu(list) (*((struct list_head __rcu **)(&(list)->prev)))
> > 
> > We need a rcu_dereference() in there somewhere, otherwise the compiler
> > might ruin your day.
> > 
> > Huh.  You (quite reasonably) copy-pasta'd list_next_rcu().  So the
> 
> I expect the caller to do the rcu_dereference() just like with
> list_next_rcu():
> 
> At first I made it include a rcu_dereference. But then I realized that
> the api becomes rather wonky because list_next_rcu() would have to be
> called with rcu_dereference() (most caller's do) and
> list_bidir_prev_rcu() wouldn't.
> 
> That seemed very confusing to me so I just kept it aligned with
> list_next_rcu(). Now, the correct follow-up cleanup would obviously be
> to port both helpers to include the rcu_dereference() and thereby simply
> all callers. For the few (mostly inside the header itself) we could just
> add a __list_next_rcu() thing that doesn't include the
> rcu_dereference().

I am not sure what others think, but that sound better than status quo
to me.  And it is not like rcu_dereference() is costly, other than on
DEC Alpha, so we likely don't even need that __list_next_rcu().  Of
course, the opinion of actual hardware overrides my own, as always.

							Thanx, Paul

> > restriction is the same, the caller must use rcu_dereference.  Unless,
> > like seq_list_next_rcu(), you are never dereferencing it.  That said,
> > I am not so sure about the callers of unloaded_tainted_modules_seq_next()
> > and rxrpc_call_seq_next(), which inherit the same restriction.
> > 
> > If those two are used properly with rcu_dereference(), we have empirical
> > evidence indicating that things might be OK.  Otherwise, both need at
> > least an upgrade of their header comments.  ;-)
> > 
> > >  /**
> > >   * list_tail_rcu - returns the prev pointer of the head of the list
> > > @@ -158,6 +166,41 @@ static inline void list_del_rcu(struct list_head *entry)
> > >  	entry->prev = LIST_POISON2;
> > >  }
> > >  
> > > +/**
> > > + * list_bidir_del_rcu - deletes entry from list without re-initialization
> > > + * @entry: the element to delete from the list.
> > > + *
> > > + * In contrat to list_del_rcu() doesn't poison the previous pointer thus
> > 
> > Looks good, but while I am here, I might as well nitpick...
> > 
> > "In constrast".
> > 
> > > + * allowing to go backwards via list_prev_bidir_rcu().
> > 
> > "allowing backwards traversal via"
> > 
> > > + * Note: list_empty() on entry does not return true after this,
> > > + * the entry is in an undefined state. It is useful for RCU based
> > 
> > "because the entry is in a special undefined state that permits
> > RCU-based lockfree reverse traversal."
> > 
> > > + * lockfree traversal.
> > 
> > At which point, you don't need this paragraph break.
> > 
> > > + * In particular, it means that we can not poison the forward
> > 
> > "this means that ... forward and backwards"
> > 
> > > + * pointers that may still be used for walking the list.
> > > + *
> > > + * The caller must take whatever precautions are necessary
> > > + * (such as holding appropriate locks) to avoid racing
> > > + * with another list-mutation primitive, such as list_bidir_del_rcu()
> > > + * or list_add_rcu(), running on this same list.
> > > + * However, it is perfectly legal to run concurrently with
> > > + * the _rcu list-traversal primitives, such as
> > > + * list_for_each_entry_rcu().
> > > + *
> > > + * Noe that the it is not allowed to use list_del_rcu() and
> > 
> > "Note that list_del_rcu() and list_bidir_del_rcu() must not be used on
> > the same list at the same time."
> > 
> > If you want to leave off the "at the same time", I am good.  One could
> > argue that we should not call attention to the possibility of adding
> > this sort of complexity.  Let them need it badly first.  ;-)
> > 
> > > + * list_bidir_del_rcu() on the same list.
> > > + *
> > > + * Note that the caller is not permitted to immediately free
> > > + * the newly deleted entry.  Instead, either synchronize_rcu()
> > > + * or call_rcu() must be used to defer freeing until an RCU
> > > + * grace period has elapsed.
> > > + */
> > > +static inline void list_bidir_del_rcu(struct list_head *entry)
> > > +{
> > > +	__list_del_entry(entry);
> > > +}
> > > +
> > >  /**
> > >   * hlist_del_init_rcu - deletes entry from hash list with re-initialization
> > >   * @n: the element to delete from the hash list.
> > > 
> > > -- 
> > > 2.45.2
> > > 

[PATCH v2 5/8] fs: lockless mntns lookup for nsfs

[Christian Brauner]

We already made the rbtree lookup lockless for the simple lookup case.
However, walking the list of mount namespaces via nsfs still happens
with taking the read lock blocking concurrent additions of new mount
namespaces pointlessly. Plus, such additions are rare anyway so allow
lockless lookup of the previous and next mount namespace by keeping a
separate list. This also allows to make some things simpler in the code.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/mount.h     | 17 +++++++----------
 fs/namespace.c | 34 +++++++++++++++++++++-------------
 fs/nsfs.c      |  5 +----
 3 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/fs/mount.h b/fs/mount.h
index 3c3763d8ae821d6a117c528808dbc94d0251f964..b7edb4034c2131b758f953cefbf47d060e27e03a 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -17,7 +17,10 @@ struct mnt_namespace {
 	unsigned int		nr_mounts; /* # of mounts in the namespace */
 	unsigned int		pending_mounts;
 	union {
-		struct rb_node	mnt_ns_tree_node; /* node in the mnt_ns_tree */
+		struct {
+			struct list_head	mnt_ns_list;
+			struct rb_node		mnt_ns_tree_node; /* node in the mnt_ns_tree */
+		};
 		struct rcu_head mnt_ns_rcu;
 	};
 	refcount_t		passive; /* number references not pinning @mounts */
@@ -157,15 +160,9 @@ static inline void move_from_ns(struct mount *mnt, struct list_head *dt_list)
 }
 
 bool has_locked_children(struct mount *mnt, struct dentry *dentry);
-struct mnt_namespace *__lookup_next_mnt_ns(struct mnt_namespace *mnt_ns, bool previous);
-static inline struct mnt_namespace *lookup_next_mnt_ns(struct mnt_namespace *mntns)
-{
-	return __lookup_next_mnt_ns(mntns, false);
-}
-static inline struct mnt_namespace *lookup_prev_mnt_ns(struct mnt_namespace *mntns)
-{
-	return __lookup_next_mnt_ns(mntns, true);
-}
+struct mnt_namespace *get_sequential_mnt_ns(struct mnt_namespace *mnt_ns,
+					    bool previous);
+
 static inline struct mnt_namespace *to_mnt_ns(struct ns_common *ns)
 {
 	return container_of(ns, struct mnt_namespace, ns);
diff --git a/fs/namespace.c b/fs/namespace.c
index 9463b9ab95f0a5db32cfe5fc5564d7f25ce3e06f..a5e1b166be9430d47c295159292cb9028b2e2339 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -82,6 +82,7 @@ static DEFINE_RWLOCK(mnt_ns_tree_lock);
 static seqcount_rwlock_t mnt_ns_tree_seqcount = SEQCNT_RWLOCK_ZERO(mnt_ns_tree_seqcount, &mnt_ns_tree_lock);
 
 static struct rb_root mnt_ns_tree = RB_ROOT; /* protected by mnt_ns_tree_lock */
+static LIST_HEAD(mnt_ns_list); /* protected by mnt_ns_tree_lock */
 
 struct mount_kattr {
 	unsigned int attr_set;
@@ -146,6 +147,7 @@ static void mnt_ns_tree_add(struct mnt_namespace *ns)
 
 	mnt_ns_tree_write_lock();
 	node = rb_find_add_rcu(&ns->mnt_ns_tree_node, &mnt_ns_tree, mnt_ns_cmp);
+	list_add_tail_rcu(&ns->mnt_ns_list, &mnt_ns_list);
 	mnt_ns_tree_write_unlock();
 
 	WARN_ON_ONCE(node);
@@ -177,6 +179,7 @@ static void mnt_ns_tree_remove(struct mnt_namespace *ns)
 	if (!is_anon_ns(ns)) {
 		mnt_ns_tree_write_lock();
 		rb_erase(&ns->mnt_ns_tree_node, &mnt_ns_tree);
+		list_bidir_del_rcu(&ns->mnt_ns_list);
 		mnt_ns_tree_write_unlock();
 	}
 
@@ -2091,30 +2094,34 @@ struct ns_common *from_mnt_ns(struct mnt_namespace *mnt)
 	return &mnt->ns;
 }
 
-struct mnt_namespace *__lookup_next_mnt_ns(struct mnt_namespace *mntns, bool previous)
+struct mnt_namespace *get_sequential_mnt_ns(struct mnt_namespace *mntns, bool previous)
 {
-	guard(read_lock)(&mnt_ns_tree_lock);
-	for (;;) {
-		struct rb_node *node;
+	struct list_head *list;
+
+	guard(rcu)();
 
+	for (;;) {
 		if (previous)
-			node = rb_prev(&mntns->mnt_ns_tree_node);
+			list = rcu_dereference(list_bidir_prev_rcu(&mntns->mnt_ns_list));
 		else
-			node = rb_next(&mntns->mnt_ns_tree_node);
-		if (!node)
+			list = rcu_dereference(list_next_rcu(&mntns->mnt_ns_list));
+		if (list_is_head(list, &mnt_ns_list))
 			return ERR_PTR(-ENOENT);
 
-		mntns = node_to_mnt_ns(node);
-		node = &mntns->mnt_ns_tree_node;
+		mntns = list_entry_rcu(list, struct mnt_namespace, mnt_ns_list);
 
+		/*
+		 * The last passive reference count is put with RCU
+		 * delay so accessing the mount namespace is not just
+		 * safe it's members are all still valid.
+		 */
 		if (!ns_capable_noaudit(mntns->user_ns, CAP_SYS_ADMIN))
 			continue;
 
 		/*
-		 * Holding mnt_ns_tree_lock prevents the mount namespace from
-		 * being freed but it may well be on it's deathbed. We want an
-		 * active reference, not just a passive one here as we're
-		 * persisting the mount namespace.
+		 * We need an active reference count as we're persisting
+		 * the mount namespace and it might already be on its
+		 * deathbed.
 		 */
 		if (!refcount_inc_not_zero(&mntns->ns.count))
 			continue;
@@ -3931,6 +3938,7 @@ static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns, bool a
 	refcount_set(&new_ns->ns.count, 1);
 	refcount_set(&new_ns->passive, 1);
 	new_ns->mounts = RB_ROOT;
+	INIT_LIST_HEAD(&new_ns->mnt_ns_list);
 	RB_CLEAR_NODE(&new_ns->mnt_ns_tree_node);
 	init_waitqueue_head(&new_ns->poll);
 	new_ns->user_ns = get_user_ns(user_ns);
diff --git a/fs/nsfs.c b/fs/nsfs.c
index c675fc40ce2dc674f0dafce5c4924b910a73a23f..663f8656158d52d391ba80ef1d320197d3d654e0 100644
--- a/fs/nsfs.c
+++ b/fs/nsfs.c
@@ -274,10 +274,7 @@ static long ns_ioctl(struct file *filp, unsigned int ioctl,
 		if (usize < MNT_NS_INFO_SIZE_VER0)
 			return -EINVAL;
 
-		if (previous)
-			mnt_ns = lookup_prev_mnt_ns(to_mnt_ns(ns));
-		else
-			mnt_ns = lookup_next_mnt_ns(to_mnt_ns(ns));
+		mnt_ns = get_sequential_mnt_ns(to_mnt_ns(ns), previous);
 		if (IS_ERR(mnt_ns))
 			return PTR_ERR(mnt_ns);
 

-- 
2.45.2

Re: [PATCH v2 5/8] fs: lockless mntns lookup for nsfs

[Peter Zijlstra]

On Thu, Dec 12, 2024 at 12:56:04PM +0100, Christian Brauner wrote:

> @@ -146,6 +147,7 @@ static void mnt_ns_tree_add(struct mnt_namespace *ns)
>  
>  	mnt_ns_tree_write_lock();
>  	node = rb_find_add_rcu(&ns->mnt_ns_tree_node, &mnt_ns_tree, mnt_ns_cmp);
> +	list_add_tail_rcu(&ns->mnt_ns_list, &mnt_ns_list);
>  	mnt_ns_tree_write_unlock();

This only works if the entries are inserted in order -- if not, you can
do something like:

  prev = rb_prev(&ns->mnt_ns_tree_node);
  if (!prev) {
    // no previous, add to head
    list_add(&ns->mnt_ns_list, &mnt_ns_list); 
  } else {
    // add after the previous tree node
    prev_ns = container_of(prev, struct mnt_namespace, mnt_ns_tree_node);
    list_add_tail(&ns->mnt_ns_list, &prev_ns->mnt_ns_list);
  }

Re: [PATCH v2 5/8] fs: lockless mntns lookup for nsfs

[Christian Brauner]

On Thu, Dec 12, 2024 at 01:48:17PM +0100, Peter Zijlstra wrote:
> On Thu, Dec 12, 2024 at 12:56:04PM +0100, Christian Brauner wrote:
> 
> > @@ -146,6 +147,7 @@ static void mnt_ns_tree_add(struct mnt_namespace *ns)
> >  
> >  	mnt_ns_tree_write_lock();
> >  	node = rb_find_add_rcu(&ns->mnt_ns_tree_node, &mnt_ns_tree, mnt_ns_cmp);
> > +	list_add_tail_rcu(&ns->mnt_ns_list, &mnt_ns_list);
> >  	mnt_ns_tree_write_unlock();
> 
> This only works if the entries are inserted in order -- if not, you can
> do something like:

If I understand your concern correctly then the entries should always be
inserted in order. Mount namespaces are sequentially allocated
serialized on the namespace semaphore "namespace_sem. So each mount
namespace receives a unique 64bit sequence number. If ten mount
namespaces are created with 1, 2, 3, ..., 10 then they are inserted into
the rbtree in that order. And so they should be added to that list in
the same order. That's why I kept it that simple.

Although I need to drop "fs: add mount namespace to rbtree late" to keep
that guarantee. Did I understand you correctly?

> 
>   prev = rb_prev(&ns->mnt_ns_tree_node);
>   if (!prev) {
>     // no previous, add to head
>     list_add(&ns->mnt_ns_list, &mnt_ns_list); 
>   } else {
>     // add after the previous tree node
>     prev_ns = container_of(prev, struct mnt_namespace, mnt_ns_tree_node);
>     list_add_tail(&ns->mnt_ns_list, &prev_ns->mnt_ns_list);
>   }

Re: [PATCH v2 5/8] fs: lockless mntns lookup for nsfs

[Christian Brauner]

> > This only works if the entries are inserted in order -- if not, you can
> > do something like:
> 
> If I understand your concern correctly then the entries should always be
> inserted in order. Mount namespaces are sequentially allocated
> serialized on the namespace semaphore "namespace_sem. So each mount
> namespace receives a unique 64bit sequence number. If ten mount
> namespaces are created with 1, 2, 3, ..., 10 then they are inserted into
> the rbtree in that order. And so they should be added to that list in
> the same order. That's why I kept it that simple.

Nope, you were right, I was wrong. We allocate the sequence number
before we hold the namespace semaphore. I misremembered that code.
I'll fix this up as you suggested. Thanks!

[PATCH v2 6/8] fs: simplify rwlock to spinlock

[Christian Brauner]

We're not taking the read_lock() anymore now that all lookup is losless.
Just use a simple spinlock.

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

diff --git a/fs/namespace.c b/fs/namespace.c
index a5e1b166be9430d47c295159292cb9028b2e2339..984ec416ca7618260a38b86a16b66dcdba6e62ed 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -78,8 +78,8 @@ static struct kmem_cache *mnt_cache __ro_after_init;
 static DECLARE_RWSEM(namespace_sem);
 static HLIST_HEAD(unmounted);	/* protected by namespace_sem */
 static LIST_HEAD(ex_mountpoints); /* protected by namespace_sem */
-static DEFINE_RWLOCK(mnt_ns_tree_lock);
-static seqcount_rwlock_t mnt_ns_tree_seqcount = SEQCNT_RWLOCK_ZERO(mnt_ns_tree_seqcount, &mnt_ns_tree_lock);
+static DEFINE_SPINLOCK(mnt_ns_tree_lock);
+static seqcount_spinlock_t mnt_ns_tree_seqcount = SEQCNT_SPINLOCK_ZERO(mnt_ns_tree_seqcount, &mnt_ns_tree_lock);
 
 static struct rb_root mnt_ns_tree = RB_ROOT; /* protected by mnt_ns_tree_lock */
 static LIST_HEAD(mnt_ns_list); /* protected by mnt_ns_tree_lock */
@@ -131,14 +131,14 @@ static int mnt_ns_cmp(struct rb_node *a, const struct rb_node *b)
 
 static inline void mnt_ns_tree_write_lock(void)
 {
-	write_lock(&mnt_ns_tree_lock);
+	spin_lock(&mnt_ns_tree_lock);
 	write_seqcount_begin(&mnt_ns_tree_seqcount);
 }
 
 static inline void mnt_ns_tree_write_unlock(void)
 {
 	write_seqcount_end(&mnt_ns_tree_seqcount);
-	write_unlock(&mnt_ns_tree_lock);
+	spin_unlock(&mnt_ns_tree_lock);
 }
 
 static void mnt_ns_tree_add(struct mnt_namespace *ns)

-- 
2.45.2

[PATCH v2 7/8] selftests: remove unneeded include

[Christian Brauner]

The pidfd header will be included in a sample program and this pulls in
all the mount definitions that would be causing problems.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 tools/testing/selftests/pidfd/pidfd.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
index 88d6830ee004df3c7a9d3ebcdab89d5775e9ab9b..3a96053e52e7bbf5f7f85908c2093e9023b1d3d6 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -12,7 +12,6 @@
 #include <stdlib.h>
 #include <string.h>
 #include <syscall.h>
-#include <sys/mount.h>
 #include <sys/types.h>
 #include <sys/wait.h>
 

-- 
2.45.2

[PATCH v2 8/8] samples: add test-list-all-mounts

[Christian Brauner]

Add a sample program illustrating how to list all mounts in all mount
namespaces.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 samples/vfs/.gitignore             |   1 +
 samples/vfs/Makefile               |   2 +-
 samples/vfs/test-list-all-mounts.c | 235 +++++++++++++++++++++++++++++++++++++
 3 files changed, 237 insertions(+), 1 deletion(-)

diff --git a/samples/vfs/.gitignore b/samples/vfs/.gitignore
index 79212d91285bca72b0ff85f28aaccd2e803ac092..8694dd17b318768b975ece5c7cd450c2cca67318 100644
--- a/samples/vfs/.gitignore
+++ b/samples/vfs/.gitignore
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 /test-fsmount
+/test-list-all-mounts
 /test-statx
diff --git a/samples/vfs/Makefile b/samples/vfs/Makefile
index 6377a678134acf0d682151d751d2f5042dbf5e0a..301be72a52a0e376c7ebe235cc2058992919cc78 100644
--- a/samples/vfs/Makefile
+++ b/samples/vfs/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-userprogs-always-y += test-fsmount test-statx
+userprogs-always-y += test-fsmount test-statx test-list-all-mounts
 
 userccflags += -I usr/include
diff --git a/samples/vfs/test-list-all-mounts.c b/samples/vfs/test-list-all-mounts.c
new file mode 100644
index 0000000000000000000000000000000000000000..f372d5aea4717fd1ab3d4b3f9af79316cd5dd3d3
--- /dev/null
+++ b/samples/vfs/test-list-all-mounts.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2024 Christian Brauner <brauner@kernel.org>
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <limits.h>
+#include <linux/types.h>
+#include <stdio.h>
+#include <sys/ioctl.h>
+#include <sys/syscall.h>
+
+#include "../../tools/testing/selftests/pidfd/pidfd.h"
+
+#define die_errno(format, ...)                                             \
+	do {                                                               \
+		fprintf(stderr, "%m | %s: %d: %s: " format "\n", __FILE__, \
+			__LINE__, __func__, ##__VA_ARGS__);                \
+		exit(EXIT_FAILURE);                                        \
+	} while (0)
+
+/* Get the id for a mount namespace */
+#define NS_GET_MNTNS_ID _IO(0xb7, 0x5)
+/* Get next mount namespace. */
+
+struct mnt_ns_info {
+	__u32 size;
+	__u32 nr_mounts;
+	__u64 mnt_ns_id;
+};
+
+#define MNT_NS_INFO_SIZE_VER0 16 /* size of first published struct */
+
+/* Get information about namespace. */
+#define NS_MNT_GET_INFO _IOR(0xb7, 10, struct mnt_ns_info)
+/* Get next namespace. */
+#define NS_MNT_GET_NEXT _IOR(0xb7, 11, struct mnt_ns_info)
+/* Get previous namespace. */
+#define NS_MNT_GET_PREV _IOR(0xb7, 12, struct mnt_ns_info)
+
+#define PIDFD_GET_MNT_NAMESPACE _IO(0xFF, 3)
+
+#ifndef __NR_listmount
+#define __NR_listmount 458
+#endif
+
+#ifndef __NR_statmount
+#define __NR_statmount 457
+#endif
+
+/* @mask bits for statmount(2) */
+#define STATMOUNT_SB_BASIC		0x00000001U /* Want/got sb_... */
+#define STATMOUNT_MNT_BASIC		0x00000002U /* Want/got mnt_... */
+#define STATMOUNT_PROPAGATE_FROM	0x00000004U /* Want/got propagate_from */
+#define STATMOUNT_MNT_ROOT		0x00000008U /* Want/got mnt_root  */
+#define STATMOUNT_MNT_POINT		0x00000010U /* Want/got mnt_point */
+#define STATMOUNT_FS_TYPE		0x00000020U /* Want/got fs_type */
+#define STATMOUNT_MNT_NS_ID		0x00000040U /* Want/got mnt_ns_id */
+#define STATMOUNT_MNT_OPTS		0x00000080U /* Want/got mnt_opts */
+
+#define STATX_MNT_ID_UNIQUE 0x00004000U /* Want/got extended stx_mount_id */
+
+struct statmount {
+	__u32 size;
+	__u32 mnt_opts;
+	__u64 mask;
+	__u32 sb_dev_major;
+	__u32 sb_dev_minor;
+	__u64 sb_magic;
+	__u32 sb_flags;
+	__u32 fs_type;
+	__u64 mnt_id;
+	__u64 mnt_parent_id;
+	__u32 mnt_id_old;
+	__u32 mnt_parent_id_old;
+	__u64 mnt_attr;
+	__u64 mnt_propagation;
+	__u64 mnt_peer_group;
+	__u64 mnt_master;
+	__u64 propagate_from;
+	__u32 mnt_root;
+	__u32 mnt_point;
+	__u64 mnt_ns_id;
+	__u64 __spare2[49];
+	char str[];
+};
+
+struct mnt_id_req {
+	__u32 size;
+	__u32 spare;
+	__u64 mnt_id;
+	__u64 param;
+	__u64 mnt_ns_id;
+};
+
+#define MNT_ID_REQ_SIZE_VER1 32 /* sizeof second published struct */
+
+#define LSMT_ROOT 0xffffffffffffffff /* root mount */
+
+static int __statmount(__u64 mnt_id, __u64 mnt_ns_id, __u64 mask,
+		       struct statmount *stmnt, size_t bufsize,
+		       unsigned int flags)
+{
+	struct mnt_id_req req = {
+		.size		= MNT_ID_REQ_SIZE_VER1,
+		.mnt_id		= mnt_id,
+		.param		= mask,
+		.mnt_ns_id	= mnt_ns_id,
+	};
+
+	return syscall(__NR_statmount, &req, stmnt, bufsize, flags);
+}
+
+static struct statmount *sys_statmount(__u64 mnt_id, __u64 mnt_ns_id,
+				       __u64 mask, unsigned int flags)
+{
+	size_t bufsize = 1 << 15;
+	struct statmount *stmnt = NULL, *tmp = NULL;
+	int ret;
+
+	for (;;) {
+		tmp = realloc(stmnt, bufsize);
+		if (!tmp)
+			goto out;
+
+		stmnt = tmp;
+		ret = __statmount(mnt_id, mnt_ns_id, mask, stmnt, bufsize, flags);
+		if (!ret)
+			return stmnt;
+
+		if (errno != EOVERFLOW)
+			goto out;
+
+		bufsize <<= 1;
+		if (bufsize >= UINT_MAX / 2)
+			goto out;
+	}
+
+out:
+	free(stmnt);
+	return NULL;
+}
+
+static ssize_t sys_listmount(__u64 mnt_id, __u64 last_mnt_id, __u64 mnt_ns_id,
+			     __u64 list[], size_t num, unsigned int flags)
+{
+	struct mnt_id_req req = {
+		.size		= MNT_ID_REQ_SIZE_VER1,
+		.mnt_id		= mnt_id,
+		.param		= last_mnt_id,
+		.mnt_ns_id	= mnt_ns_id,
+	};
+
+	return syscall(__NR_listmount, &req, list, num, flags);
+}
+
+int main(int argc, char *argv[])
+{
+#define LISTMNT_BUFFER 10
+	__u64 list[LISTMNT_BUFFER], last_mnt_id = 0;
+	int ret, pidfd, fd_mntns;
+	struct mnt_ns_info info = {};
+
+	pidfd = sys_pidfd_open(getpid(), 0);
+	if (pidfd < 0)
+		die_errno("pidfd_open failed");
+
+	fd_mntns = ioctl(pidfd, PIDFD_GET_MNT_NAMESPACE, 0);
+	if (fd_mntns < 0)
+		die_errno("ioctl(PIDFD_GET_MNT_NAMESPACE) failed");
+
+	ret = ioctl(fd_mntns, NS_MNT_GET_INFO, &info);
+	if (ret < 0)
+		die_errno("ioctl(NS_GET_MNTNS_ID) failed");
+
+	printf("Listing %u mounts for mount namespace %llu\n",
+	       info.nr_mounts, info.mnt_ns_id);
+	for (;;) {
+		ssize_t nr_mounts;
+next:
+		nr_mounts = sys_listmount(LSMT_ROOT, last_mnt_id,
+					  info.mnt_ns_id, list, LISTMNT_BUFFER,
+					  0);
+		if (nr_mounts <= 0) {
+			int fd_mntns_next;
+
+			printf("Finished listing %u mounts for mount namespace %llu\n\n",
+			       info.nr_mounts, info.mnt_ns_id);
+			fd_mntns_next = ioctl(fd_mntns, NS_MNT_GET_NEXT, &info);
+			if (fd_mntns_next < 0) {
+				if (errno == ENOENT) {
+					printf("Finished listing all mount namespaces\n");
+					exit(0);
+				}
+				die_errno("ioctl(NS_MNT_GET_NEXT) failed");
+			}
+			close(fd_mntns);
+			fd_mntns = fd_mntns_next;
+			last_mnt_id = 0;
+			printf("Listing %u mounts for mount namespace %llu\n",
+			       info.nr_mounts, info.mnt_ns_id);
+			goto next;
+		}
+
+		for (size_t cur = 0; cur < nr_mounts; cur++) {
+			struct statmount *stmnt;
+
+			last_mnt_id = list[cur];
+
+			stmnt = sys_statmount(last_mnt_id, info.mnt_ns_id,
+					      STATMOUNT_SB_BASIC |
+					      STATMOUNT_MNT_BASIC |
+					      STATMOUNT_MNT_ROOT |
+					      STATMOUNT_MNT_POINT |
+					      STATMOUNT_MNT_NS_ID |
+					      STATMOUNT_MNT_OPTS |
+					      STATMOUNT_FS_TYPE, 0);
+			if (!stmnt) {
+				printf("Failed to statmount(%llu) in mount namespace(%llu)\n",
+				       last_mnt_id, info.mnt_ns_id);
+				continue;
+			}
+
+			printf("mnt_id:\t\t%llu\nmnt_parent_id:\t%llu\nfs_type:\t%s\nmnt_root:\t%s\nmnt_point:\t%s\nmnt_opts:\t%s\n\n",
+			       stmnt->mnt_id,
+			       stmnt->mnt_parent_id,
+			       stmnt->str + stmnt->fs_type,
+			       stmnt->str + stmnt->mnt_root,
+			       stmnt->str + stmnt->mnt_point,
+			       stmnt->str + stmnt->mnt_opts);
+			free(stmnt);
+		}
+	}
+
+	exit(0);
+}

-- 
2.45.2