All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] ipc,namespace: fix free vs allocation race
@ 2023-01-27  1:15 Rik van Riel
  2023-01-27  1:15 ` [PATCH 1/2] ipc,namespace: make ipc namespace allocation wait for pending free Rik van Riel
  2023-01-27  1:15 ` [PATCH 2/2] ipc,namespace: batch free ipc_namespace structures Rik van Riel
  0 siblings, 2 replies; 6+ messages in thread
From: Rik van Riel @ 2023-01-27  1:15 UTC (permalink / raw)
  To: viro, linux-kernel, kernel-team, linux-fsdevel, gscrivan

The IPC namespace code frees ipc_namespace structures asynchronously,
via a work queue item. This results in ipc_namespace structures being
freed very slowly, and the allocation path getting false failures
since the to-be-freed ipc_namespace structures have not been freed
yet.

Fix that by having the allocator wait when there are ipc_namespace
structures pending to be freed.

Also speed up the freeing of ipc_namespace structures. We had some
discussions about this last year, and ended up trying out various
"nicer" ideas that did not work, so I went back to the original,
with Al Viro's suggestion for a helper function:

https://lore.kernel.org/all/Yg8StKzTWh+7FLuA@zeniv-ca.linux.org.uk/

This series fixes both the false allocation failures, and the slow
freeing of ipc_namespace structures.

v2: a few more fs/namespace.c cleanups suggested by Al Viro (thank you!)



^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/2] ipc,namespace: make ipc namespace allocation wait for pending free
  2023-01-27  1:15 [PATCH v2 0/2] ipc,namespace: fix free vs allocation race Rik van Riel
@ 2023-01-27  1:15 ` Rik van Riel
  2023-01-27  1:15 ` [PATCH 2/2] ipc,namespace: batch free ipc_namespace structures Rik van Riel
  1 sibling, 0 replies; 6+ messages in thread
From: Rik van Riel @ 2023-01-27  1:15 UTC (permalink / raw)
  To: viro, linux-kernel, kernel-team, linux-fsdevel, gscrivan
  Cc: Rik van Riel, Chris Mason

Currently the ipc namespace allocation will fail when there are
ipc_namespace structures pending to be freed. This results in the
simple test case below, as well as some real world workloads, to
get allocation failures even when the number of ipc namespaces in
actual use is way below the limit.

int main()
{
	int i;

	for (i = 0; i < 100000; i++) {
		if (unshare(CLONE_NEWIPC) < 0)
			error(EXIT_FAILURE, errno, "unshare");
	}
}

Make the allocation of an ipc_namespace wait for pending frees,
so it will succeed.

real	6m19.197s
user	0m0.041s
sys	0m1.019s

Signed-off-by: Rik van Riel <riel@surriel.com>
Reported-by: Chris Mason <clm@meta.com>
---
 ipc/namespace.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/ipc/namespace.c b/ipc/namespace.c
index 8316ea585733..a26860a41dac 100644
--- a/ipc/namespace.c
+++ b/ipc/namespace.c
@@ -19,6 +19,12 @@
 
 #include "util.h"
 
+/*
+ * The work queue is used to avoid the cost of synchronize_rcu in kern_unmount.
+ */
+static void free_ipc(struct work_struct *unused);
+static DECLARE_WORK(free_ipc_work, free_ipc);
+
 static struct ucounts *inc_ipc_namespaces(struct user_namespace *ns)
 {
 	return inc_ucount(ns, current_euid(), UCOUNT_IPC_NAMESPACES);
@@ -37,9 +43,18 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns,
 	int err;
 
 	err = -ENOSPC;
+ again:
 	ucounts = inc_ipc_namespaces(user_ns);
-	if (!ucounts)
+	if (!ucounts) {
+		/*
+		 * IPC namespaces are freed asynchronously, by free_ipc_work.
+		 * If frees were pending, flush_work will wait, and
+		 * return true. Fail the allocation if no frees are pending.
+		 */
+		if (flush_work(&free_ipc_work))
+			goto again;
 		goto fail;
+	}
 
 	err = -ENOMEM;
 	ns = kzalloc(sizeof(struct ipc_namespace), GFP_KERNEL_ACCOUNT);
@@ -157,11 +172,6 @@ static void free_ipc(struct work_struct *unused)
 		free_ipc_ns(n);
 }
 
-/*
- * The work queue is used to avoid the cost of synchronize_rcu in kern_unmount.
- */
-static DECLARE_WORK(free_ipc_work, free_ipc);
-
 /*
  * put_ipc_ns - drop a reference to an ipc namespace.
  * @ns: the namespace to put
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] ipc,namespace: batch free ipc_namespace structures
  2023-01-27  1:15 [PATCH v2 0/2] ipc,namespace: fix free vs allocation race Rik van Riel
  2023-01-27  1:15 ` [PATCH 1/2] ipc,namespace: make ipc namespace allocation wait for pending free Rik van Riel
@ 2023-01-27  1:15 ` Rik van Riel
  2023-01-27  1:31   ` [PATCH v2 " Rik van Riel
  2023-01-27 11:03   ` [PATCH " Giuseppe Scrivano
  1 sibling, 2 replies; 6+ messages in thread
From: Rik van Riel @ 2023-01-27  1:15 UTC (permalink / raw)
  To: viro, linux-kernel, kernel-team, linux-fsdevel, gscrivan
  Cc: Rik van Riel, Chris Mason

Instead of waiting for an RCU grace period between each ipc_namespace
structure that is being freed, wait an RCU grace period for every batch
of ipc_namespace structures.

Thanks to Al Viro for the suggestion of the helper function.

This speeds up the run time of the test case that allocates ipc_namespaces
in a loop from 6 minutes, to a little over 1 second:

real	0m1.192s
user	0m0.038s
sys	0m1.152s

Signed-off-by: Rik van Riel <riel@surriel.com>
Reported-by: Chris Mason <clm@meta.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/namespace.c        | 10 ++++++++++
 include/linux/mount.h |  1 +
 ipc/namespace.c       | 13 ++++++++++---
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index ab467ee58341..296432ba3716 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1397,6 +1397,16 @@ struct vfsmount *mntget(struct vfsmount *mnt)
 }
 EXPORT_SYMBOL(mntget);
 
+/*
+ * Make a mount point inaccessible to new lookups.
+ * Because there may still be current users, the caller MUST WAIT
+ * for an RCU grace period before destroying the mount point.
+ */
+void mnt_make_shortterm(struct vfsmount *mnt)
+{
+	real_mount(mnt)->mnt_ns = NULL;
+}
+
 /**
  * path_is_mountpoint() - Check if path is a mount in the current namespace.
  * @path: path to check
diff --git a/include/linux/mount.h b/include/linux/mount.h
index 62475996fac6..ec55a031aa8c 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -88,6 +88,7 @@ extern void mnt_drop_write(struct vfsmount *mnt);
 extern void mnt_drop_write_file(struct file *file);
 extern void mntput(struct vfsmount *mnt);
 extern struct vfsmount *mntget(struct vfsmount *mnt);
+extern void mnt_make_shortterm(struct vfsmount *mnt);
 extern struct vfsmount *mnt_clone_internal(const struct path *path);
 extern bool __mnt_is_readonly(struct vfsmount *mnt);
 extern bool mnt_may_suid(struct vfsmount *mnt);
diff --git a/ipc/namespace.c b/ipc/namespace.c
index a26860a41dac..6ecc30effd3e 100644
--- a/ipc/namespace.c
+++ b/ipc/namespace.c
@@ -145,10 +145,11 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
 
 static void free_ipc_ns(struct ipc_namespace *ns)
 {
-	/* mq_put_mnt() waits for a grace period as kern_unmount()
-	 * uses synchronize_rcu().
+	/*
+	 * Caller needs to wait for an RCU grace period to have passed
+	 * after making the mount point inaccessible to new accesses.
 	 */
-	mq_put_mnt(ns);
+	mntput(ns->mq_mnt);
 	sem_exit_ns(ns);
 	msg_exit_ns(ns);
 	shm_exit_ns(ns);
@@ -168,6 +169,12 @@ static void free_ipc(struct work_struct *unused)
 	struct llist_node *node = llist_del_all(&free_ipc_list);
 	struct ipc_namespace *n, *t;
 
+	llist_for_each_entry_safe(n, t, node, mnt_llist)
+		mnt_make_shortterm(n->mq_mnt);
+
+	/* Wait for any last users to have gone away. */
+	synchronize_rcu();
+
 	llist_for_each_entry_safe(n, t, node, mnt_llist)
 		free_ipc_ns(n);
 }
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v2 2/2] ipc,namespace: batch free ipc_namespace structures
  2023-01-27  1:15 ` [PATCH 2/2] ipc,namespace: batch free ipc_namespace structures Rik van Riel
@ 2023-01-27  1:31   ` Rik van Riel
  2023-01-27 11:03   ` [PATCH " Giuseppe Scrivano
  1 sibling, 0 replies; 6+ messages in thread
From: Rik van Riel @ 2023-01-27  1:31 UTC (permalink / raw)
  To: viro, linux-kernel, kernel-team, linux-fsdevel, gscrivan; +Cc: Chris Mason

On Thu, 26 Jan 2023 20:15:35 -0500
Rik van Riel <riel@surriel.com> wrote:

> Instead of waiting for an RCU grace period between each ipc_namespace
> structure that is being freed, wait an RCU grace period for every batch
> of ipc_namespace structures.
> 
> Thanks to Al Viro for the suggestion of the helper function.

... and of course I forgot the "git add" before the "git commit --amend".
Sorry about that.

The real v2 of this patch, with Al's suggestions included is below.

---8<---

From 479378a794c5312acdb555b3deedc403134b2a70 Mon Sep 17 00:00:00 2001
From: Rik van Riel <riel@surriel.com>
Date: Thu, 26 Jan 2023 15:45:06 -0500
Subject: [PATCH] ipc,namespace: batch free ipc_namespace structures

Instead of waiting for an RCU grace period between each ipc_namespace
structure that is being freed, wait an RCU grace period for every batch
of ipc_namespace structures.

Thanks to Al Viro for the suggestion of the helper function.

This speeds up the run time of the test case that allocates ipc_namespaces
in a loop from 6 minutes, to a little over 1 second:

real	0m1.192s
user	0m0.038s
sys	0m1.152s

Signed-off-by: Rik van Riel <riel@surriel.com>
Reported-by: Chris Mason <clm@meta.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/namespace.c        | 18 ++++++++++++++----
 include/linux/mount.h |  1 +
 ipc/namespace.c       | 13 ++++++++++---
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index ab467ee58341..1ad4e5acef06 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1397,6 +1397,17 @@ struct vfsmount *mntget(struct vfsmount *mnt)
 }
 EXPORT_SYMBOL(mntget);
 
+/*
+ * Make a mount point inaccessible to new lookups.
+ * Because there may still be current users, the caller MUST WAIT
+ * for an RCU grace period before destroying the mount point.
+ */
+void mnt_make_shortterm(struct vfsmount *mnt)
+{
+	if (mnt)
+		real_mount(mnt)->mnt_ns = NULL;
+}
+
 /**
  * path_is_mountpoint() - Check if path is a mount in the current namespace.
  * @path: path to check
@@ -4573,8 +4584,8 @@ EXPORT_SYMBOL_GPL(kern_mount);
 void kern_unmount(struct vfsmount *mnt)
 {
 	/* release long term mount so mount point can be released */
-	if (!IS_ERR_OR_NULL(mnt)) {
-		real_mount(mnt)->mnt_ns = NULL;
+	if (!IS_ERR(mnt)) {
+		mnt_make_shortterm(mnt);
 		synchronize_rcu();	/* yecchhh... */
 		mntput(mnt);
 	}
@@ -4586,8 +4597,7 @@ void kern_unmount_array(struct vfsmount *mnt[], unsigned int num)
 	unsigned int i;
 
 	for (i = 0; i < num; i++)
-		if (mnt[i])
-			real_mount(mnt[i])->mnt_ns = NULL;
+		mnt_make_shortterm(mnt[i]);
 	synchronize_rcu_expedited();
 	for (i = 0; i < num; i++)
 		mntput(mnt[i]);
diff --git a/include/linux/mount.h b/include/linux/mount.h
index 62475996fac6..ec55a031aa8c 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -88,6 +88,7 @@ extern void mnt_drop_write(struct vfsmount *mnt);
 extern void mnt_drop_write_file(struct file *file);
 extern void mntput(struct vfsmount *mnt);
 extern struct vfsmount *mntget(struct vfsmount *mnt);
+extern void mnt_make_shortterm(struct vfsmount *mnt);
 extern struct vfsmount *mnt_clone_internal(const struct path *path);
 extern bool __mnt_is_readonly(struct vfsmount *mnt);
 extern bool mnt_may_suid(struct vfsmount *mnt);
diff --git a/ipc/namespace.c b/ipc/namespace.c
index a26860a41dac..6ecc30effd3e 100644
--- a/ipc/namespace.c
+++ b/ipc/namespace.c
@@ -145,10 +145,11 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
 
 static void free_ipc_ns(struct ipc_namespace *ns)
 {
-	/* mq_put_mnt() waits for a grace period as kern_unmount()
-	 * uses synchronize_rcu().
+	/*
+	 * Caller needs to wait for an RCU grace period to have passed
+	 * after making the mount point inaccessible to new accesses.
 	 */
-	mq_put_mnt(ns);
+	mntput(ns->mq_mnt);
 	sem_exit_ns(ns);
 	msg_exit_ns(ns);
 	shm_exit_ns(ns);
@@ -168,6 +169,12 @@ static void free_ipc(struct work_struct *unused)
 	struct llist_node *node = llist_del_all(&free_ipc_list);
 	struct ipc_namespace *n, *t;
 
+	llist_for_each_entry_safe(n, t, node, mnt_llist)
+		mnt_make_shortterm(n->mq_mnt);
+
+	/* Wait for any last users to have gone away. */
+	synchronize_rcu();
+
 	llist_for_each_entry_safe(n, t, node, mnt_llist)
 		free_ipc_ns(n);
 }
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ipc,namespace: batch free ipc_namespace structures
  2023-01-27  1:15 ` [PATCH 2/2] ipc,namespace: batch free ipc_namespace structures Rik van Riel
  2023-01-27  1:31   ` [PATCH v2 " Rik van Riel
@ 2023-01-27 11:03   ` Giuseppe Scrivano
  2023-01-27 18:16     ` Rik van Riel
  1 sibling, 1 reply; 6+ messages in thread
From: Giuseppe Scrivano @ 2023-01-27 11:03 UTC (permalink / raw)
  To: Rik van Riel; +Cc: viro, linux-kernel, kernel-team, linux-fsdevel, Chris Mason

Rik van Riel <riel@surriel.com> writes:

> Instead of waiting for an RCU grace period between each ipc_namespace
> structure that is being freed, wait an RCU grace period for every batch
> of ipc_namespace structures.
>
> Thanks to Al Viro for the suggestion of the helper function.
>
> This speeds up the run time of the test case that allocates ipc_namespaces
> in a loop from 6 minutes, to a little over 1 second:
>
> real	0m1.192s
> user	0m0.038s
> sys	0m1.152s
>
> Signed-off-by: Rik van Riel <riel@surriel.com>
> Reported-by: Chris Mason <clm@meta.com>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/namespace.c        | 10 ++++++++++
>  include/linux/mount.h |  1 +
>  ipc/namespace.c       | 13 ++++++++++---
>  3 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index ab467ee58341..296432ba3716 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1397,6 +1397,16 @@ struct vfsmount *mntget(struct vfsmount *mnt)
>  }
>  EXPORT_SYMBOL(mntget);
>  
> +/*
> + * Make a mount point inaccessible to new lookups.
> + * Because there may still be current users, the caller MUST WAIT
> + * for an RCU grace period before destroying the mount point.
> + */
> +void mnt_make_shortterm(struct vfsmount *mnt)
> +{
> +	real_mount(mnt)->mnt_ns = NULL;
> +}
> +
>  /**
>   * path_is_mountpoint() - Check if path is a mount in the current namespace.
>   * @path: path to check
> diff --git a/include/linux/mount.h b/include/linux/mount.h
> index 62475996fac6..ec55a031aa8c 100644
> --- a/include/linux/mount.h
> +++ b/include/linux/mount.h
> @@ -88,6 +88,7 @@ extern void mnt_drop_write(struct vfsmount *mnt);
>  extern void mnt_drop_write_file(struct file *file);
>  extern void mntput(struct vfsmount *mnt);
>  extern struct vfsmount *mntget(struct vfsmount *mnt);
> +extern void mnt_make_shortterm(struct vfsmount *mnt);
>  extern struct vfsmount *mnt_clone_internal(const struct path *path);
>  extern bool __mnt_is_readonly(struct vfsmount *mnt);
>  extern bool mnt_may_suid(struct vfsmount *mnt);
> diff --git a/ipc/namespace.c b/ipc/namespace.c
> index a26860a41dac..6ecc30effd3e 100644
> --- a/ipc/namespace.c
> +++ b/ipc/namespace.c
> @@ -145,10 +145,11 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
>  
>  static void free_ipc_ns(struct ipc_namespace *ns)
>  {
> -	/* mq_put_mnt() waits for a grace period as kern_unmount()
> -	 * uses synchronize_rcu().
> +	/*
> +	 * Caller needs to wait for an RCU grace period to have passed
> +	 * after making the mount point inaccessible to new accesses.
>  	 */
> -	mq_put_mnt(ns);

mq_put_mnt() is not needed anymore, should it be removed?

> +	mntput(ns->mq_mnt);
>  	sem_exit_ns(ns);
>  	msg_exit_ns(ns);
>  	shm_exit_ns(ns);
> @@ -168,6 +169,12 @@ static void free_ipc(struct work_struct *unused)
>  	struct llist_node *node = llist_del_all(&free_ipc_list);
>  	struct ipc_namespace *n, *t;
>  
> +	llist_for_each_entry_safe(n, t, node, mnt_llist)
> +		mnt_make_shortterm(n->mq_mnt);
> +
> +	/* Wait for any last users to have gone away. */
> +	synchronize_rcu();
> +
>  	llist_for_each_entry_safe(n, t, node, mnt_llist)
>  		free_ipc_ns(n);
>  }


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ipc,namespace: batch free ipc_namespace structures
  2023-01-27 11:03   ` [PATCH " Giuseppe Scrivano
@ 2023-01-27 18:16     ` Rik van Riel
  0 siblings, 0 replies; 6+ messages in thread
From: Rik van Riel @ 2023-01-27 18:16 UTC (permalink / raw)
  To: Giuseppe Scrivano
  Cc: viro, linux-kernel, kernel-team, linux-fsdevel, Chris Mason

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On Fri, 2023-01-27 at 12:03 +0100, Giuseppe Scrivano wrote:
> Rik van Riel <riel@surriel.com> writes:
> 
> > 
> > +++ b/ipc/namespace.c
> > @@ -145,10 +145,11 @@ void free_ipcs(struct ipc_namespace *ns,
> > struct ipc_ids *ids,
> >  
> >  static void free_ipc_ns(struct ipc_namespace *ns)
> >  {
> > -       /* mq_put_mnt() waits for a grace period as kern_unmount()
> > -        * uses synchronize_rcu().
> > +       /*
> > +        * Caller needs to wait for an RCU grace period to have
> > passed
> > +        * after making the mount point inaccessible to new
> > accesses.
> >          */
> > -       mq_put_mnt(ns);
> 
> mq_put_mnt() is not needed anymore, should it be removed?

Yes, indeed. Thank you!

I'll send a v3 of the series :)


-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-27 18:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-27  1:15 [PATCH v2 0/2] ipc,namespace: fix free vs allocation race Rik van Riel
2023-01-27  1:15 ` [PATCH 1/2] ipc,namespace: make ipc namespace allocation wait for pending free Rik van Riel
2023-01-27  1:15 ` [PATCH 2/2] ipc,namespace: batch free ipc_namespace structures Rik van Riel
2023-01-27  1:31   ` [PATCH v2 " Rik van Riel
2023-01-27 11:03   ` [PATCH " Giuseppe Scrivano
2023-01-27 18:16     ` Rik van Riel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.