| Message ID | 20230127011535.1265297-3-riel@surriel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ipc,namespace: fix free vs allocation race | expand |
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); > }
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 :)
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); }
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(-)