| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1342457578.4648.19.camel@lade.trondhjem.org>
Date: Mon, 16 Jul 2012 16:53:00 +0000
From: "Myklebust, Trond" <Trond.Myklebust@...app.com>
To: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
CC: Andrew Morton <akpm@...ux-foundation.org>,
Pavel Emelyanov <xemul@...nvz.org>,
Serge Hallyn <serge.hallyn@...onical.com>,
"KOSAKI Motohiro" <kosaki.motohiro@...fujitsu.com>,
Al Viro <viro@...iv.linux.org.uk>,
"Dmitry V. Levin" <ldv@...linux.org>,
"Kirill A. Shutemov" <kirill@...temov.name>,
Doug Ledford <dledford@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"containers@...ts.linux-foundation.org"
<containers@...ts.linux-foundation.org>
Subject: Re: [PATCH v2] ns: do not block exit_task_namespaces() for a long
time
On Mon, 2012-07-16 at 19:39 +0300, Kirill A. Shutemov wrote:
> On Mon, Jul 16, 2012 at 03:39:36PM +0000, Myklebust, Trond wrote:
> > On Mon, 2012-07-16 at 18:09 +0300, Kirill A. Shutemov wrote:
> > > From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
> > >
> > > On exiting of the last task in a namespace we need to trigger freeing of
> > > the namespace. Currently, we call synchronize_rcu() and free_nsproxy()
> > > directly on do_exit() path.
> > >
> > > On my machine synchronize_rcu() blocks for about 0.01 seconds. For
> > > comparing: normal exit_group() syscall takes less than 0.0003 seconds.
> > >
> > > Let's offload synchronize_rcu() and free_nsproxy() to a workqueue.
> > >
> > > I also move synchronize_rcu() inside free_nsproxy(). It fixes racy
> > > put_nsproxy() which calls free_nsproxy() without synchronize_rcu().
> > > I guess it was missed during switch to RCU (see cf7b708).
> > >
> > > Microbenchmark:
> > >
> > > : #define _GNU_SOURCE
> > > : #include <unistd.h>
> > > : #include <sched.h>
> > > : #include <stdlib.h>
> > > : #include <sys/wait.h>
> > > :
> > > : int
> > > : main(void)
> > > : {
> > > : int i;
> > > : for (i = 0; i < 1024; i++) {
> > > : if (fork()) {
> > > : wait(NULL);
> > > : continue;
> > > : }
> > > : unshare(CLONE_NEWIPC);
> > > : exit(0);
> > > : }
> > > : return 0;
> > > : }
> > >
> > > Before the patch:
> > >
> > > real 0m8.335s
> > > user 0m0.000s
> > > sys 0m0.265s
> > >
> > > After:
> > >
> > > real 0m0.569s
> > > user 0m0.001s
> > > sys 0m0.154s
> > >
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> > > Acked-by: Serge E. Hallyn <serge.hallyn@...ntu.com>
> > > ---
> > >
> > > v2:
> > > - Updated description.
> > >
> > > ---
> > > include/linux/nsproxy.h | 1 +
> > > kernel/nsproxy.c | 34 +++++++++++++++++++++++-----------
> > > 2 files changed, 24 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
> > > index cc37a55..1d26be7 100644
> > > --- a/include/linux/nsproxy.h
> > > +++ b/include/linux/nsproxy.h
> > > @@ -24,6 +24,7 @@ struct fs_struct;
> > > */
> > > struct nsproxy {
> > > atomic_t count;
> > > + struct work_struct free_nsproxy_work;
> > > struct uts_namespace *uts_ns;
> > > struct ipc_namespace *ipc_ns;
> > > struct mnt_namespace *mnt_ns;
> > > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> > > index b576f7f..ebc7d40 100644
> > > --- a/kernel/nsproxy.c
> > > +++ b/kernel/nsproxy.c
> > > @@ -41,13 +41,17 @@ struct nsproxy init_nsproxy = {
> > > #endif
> > > };
> > >
> > > +static void free_nsproxy_work(struct work_struct *work);
> > > +
> > > static inline struct nsproxy *create_nsproxy(void)
> > > {
> > > struct nsproxy *nsproxy;
> > >
> > > nsproxy = kmem_cache_alloc(nsproxy_cachep, GFP_KERNEL);
> > > - if (nsproxy)
> > > + if (nsproxy) {
> > > atomic_set(&nsproxy->count, 1);
> > > + INIT_WORK(&nsproxy->free_nsproxy_work, free_nsproxy_work);
> > > + }
> > > return nsproxy;
> > > }
> > >
> > > @@ -166,6 +170,14 @@ out:
> > >
> > > void free_nsproxy(struct nsproxy *ns)
> > > {
> > > + /*
> > > + * wait for others to get what they want from this nsproxy.
> > > + *
> > > + * cannot release this nsproxy via the call_rcu() since
> > > + * put_mnt_ns() will want to sleep
> > > + */
> > > + synchronize_rcu();
> > > +
> > > if (ns->mnt_ns)
> > > put_mnt_ns(ns->mnt_ns);
> > > if (ns->uts_ns)
> > > @@ -178,6 +190,14 @@ void free_nsproxy(struct nsproxy *ns)
> > > kmem_cache_free(nsproxy_cachep, ns);
> > > }
> > >
> > > +static void free_nsproxy_work(struct work_struct *work)
> > > +{
> > > + struct nsproxy *ns = container_of(work, struct nsproxy,
> > > + free_nsproxy_work);
> > > +
> > > + free_nsproxy(ns);
> > > +}
> > > +
> > > /*
> > > * Called from unshare. Unshare all the namespaces part of nsproxy.
> > > * On success, returns the new nsproxy.
> > > @@ -215,16 +235,8 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> > >
> > > rcu_assign_pointer(p->nsproxy, new);
> > >
> > > - if (ns && atomic_dec_and_test(&ns->count)) {
> > > - /*
> > > - * wait for others to get what they want from this nsproxy.
> > > - *
> > > - * cannot release this nsproxy via the call_rcu() since
> > > - * put_mnt_ns() will want to sleep
> > > - */
> > > - synchronize_rcu();
> > > - free_nsproxy(ns);
> > > - }
> > > + if (ns && atomic_dec_and_test(&ns->count))
> > > + schedule_work(&ns->free_nsproxy_work);
> >
> > What's wrong with using call_rcu()? The above will cause a workqueue
> > thread to block for no good reason.
>
> See comment to synchronize_rcu(). free_nsproxy() might sleep.
> call_rcu() callback invocation might happen from either softirq or process
> context, so we can't use it.
But call_rcu() should be allowed to call schedule_work(). At least you'd
be able to get rid of the 0.01s synchronize_rcu() sleep inside keventd.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@...app.com
www.netapp.com
Powered by blists - more mailing lists