OpenVZ Forum - RDF feed
type: channel
items: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
items: Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
items: [PATCH] create_new_namespaces: fix improper return of NULL

- merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: The patch titled Merge sys_clone()/sys_unshare() nsproxy and namespace handling has been removed from the -mm tree. Its filename was merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch This patch was dropped because it was merged into mainline or a subsystem tree ------------------------------------------------------ Subject: Merge sys_clone()/sys_unshare() nsproxy and namespace handling From: Badari Pulavarty <pbadari@us.ibm.com> sys_clone() and sys_unshare() both makes copies of nsproxy and its associated namespaces. But they have different code paths. This patch merges all the nsproxy and its associated namespace copy/clone handling (as much as possible). Posted on container list earlier for feedback. - Create a new nsproxy and its associated namespaces and pass it back to caller to attach it to right process. - Changed all copy_*_ns() routines to return a new copy of namespace instead of attaching it to task->nsproxy. - Moved the CAP_SYS_ADMIN checks out of copy_*_ns() routines. - Removed unnessary !ns checks from copy_*_ns() and added BUG_ON() just incase. - Get rid of all individual unshare_*_ns() routines and make use of copy_*_ns() instead. [akpm@osdl.org: cleanups, warning fix] [clg@fr.ibm.com: remove dup_namespaces() declaration] [serue@us.ibm.com: fix CONFIG_IPC_NS=n, clone(CLONE_NEWIPC) retval] [akpm@linux-foundation.org: fix build with CONFIG_SYSVIPC=n] Signed-off-by: Badari Pulavarty <pbadari@us.ibm.com> Signed-off-by: Serge Hallyn <serue@us.ibm.com> Cc: Cedric Le Goater <clg@fr.ibm.com> Cc: "Eric W. Biederman" <ebiederm@xmission.com> Cc: <containers@lists.osdl.org> Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> Cc: Oleg Nesterov <oleg@tv-sign.ru> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- fs/namespace.c | 30 +----- include/linux/ipc.h | 11 +- include/linux/mnt_namespace.h | 5 - include/linux/nsproxy.h | 3 include/linux/pid_namespace.h | 2 include/linux/utsname.h | 19 ---- ipc/util.c | 53 ++---------- kernel/fork.c | 85 +-------…
creator: akpm
date: 2007-05-09T02:45:35-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On Tue, May 08, 2007 at 07:45:35PM -0700, akpm@linux-foundation.org wrote: > > The patch titled > Merge sys_clone()/sys_unshare() nsproxy and namespace handling > has been removed from the -mm tree. Its filename was > merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch > > This patch was dropped because it was merged into mainline or a subsystem tree > > ------------------------------------------------------ > Subject: Merge sys_clone()/sys_unshare() nsproxy and namespace handling > From: Badari Pulavarty <pbadari@us.ibm.com> > > sys_clone() and sys_unshare() both makes copies of nsproxy and its associated > namespaces. But they have different code paths. > > This patch merges all the nsproxy and its associated namespace copy/clone > handling (as much as possible). Posted on container list earlier for > feedback. > > > - Create a new nsproxy and its associated namespaces and pass it back to > caller to attach it to right process. > > - Changed all copy_*_ns() routines to return a new copy of namespace > instead of attaching it to task->nsproxy. > > - Moved the CAP_SYS_ADMIN checks out of copy_*_ns() routines. > > - Removed unnessary !ns checks from copy_*_ns() and added BUG_ON() > just incase. > > - Get rid of all individual unshare_*_ns() routines and make use of > copy_*_ns() instead. > .. [zapped] ... > + * Called from unshare. Unshare all the namespaces part of nsproxy. > + * On sucess, returns the new nsproxy and a reference to old nsproxy > + * to make sure it stays around. > + */ > +int unshare_nsproxy_namespaces(unsigned long unshare_flags, > + struct nsproxy **new_nsp, struct fs_struct *new_fs) > +{ this makes sys_unshare leak and nsproxy (reference) can be tested with the following command sequence: vcmd -nu ^17 -- vcmd -nu ^17 -- sleep 10 (and some nsproxy accounting/debugging as used in Linux-VServer) we probably want to drop the reference to the old nsproxy in sys_unshare() but I do not see a good reason to take the reference in the first place (at …
creator: Herbert Poetzl
date: 2007-06-16T19:17:42-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On 06/16, Herbert Poetzl wrote: > > On Tue, May 08, 2007 at 07:45:35PM -0700, akpm@linux-foundation.org wrote: > > > > The patch titled > > Merge sys_clone()/sys_unshare() nsproxy and namespace handling > > has been removed from the -mm tree. Its filename was > > merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch > > > > This patch was dropped because it was merged into mainline or a subsystem tree > > > > .. [zapped] ... > > > + * Called from unshare. Unshare all the namespaces part of nsproxy. > > + * On sucess, returns the new nsproxy and a reference to old nsproxy > > + * to make sure it stays around. > > + */ > > +int unshare_nsproxy_namespaces(unsigned long unshare_flags, > > + struct nsproxy **new_nsp, struct fs_struct *new_fs) > > +{ > > this makes sys_unshare leak and nsproxy (reference) > > can be tested with the following command sequence: > vcmd -nu ^17 -- vcmd -nu ^17 -- sleep 10 I know almost nothing about this stuff, could you please explain in brief what this command does and how do you detect a leak? > (and some nsproxy accounting/debugging as used in > Linux-VServer) > > we probably want to drop the reference to the old > nsproxy in sys_unshare() but I do not see a good reason > to take the reference in the first place (at least not > with the code in mainline 2.6.22-rc4) At first glance, sys_unshare() drops the reference to the old nsproxy, old_nsproxy = current->nsproxy; current->nsproxy = new_nsproxy; new_nsproxy = old_nsproxy; ... if (new_nsproxy) put_nsproxy(new_nsproxy); However, nsproxy's code is full of strange unneeded get/put calls, for example: struct uts_namespace *copy_utsname(int flags, struct uts_namespace *old_ns) { struct uts_namespace *new_ns; BUG_ON(!old_ns); get_uts_ns(old_ns); if (!(flags & CLONE_NEWUTS)) return old_ns; new_ns = clone_uts_ns(old_ns); put_uts_ns(old_ns); return new_ns; } I think it would be better to do struct uts_namespace *copy_utsname(int flags, struct uts_namespace *old_ns) { struct uts_namespace *new_n…
creator: Oleg Nesterov
date: 2007-06-17T14:38:30-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On 06/17, Oleg Nesterov wrote: > > Let's look at copy_namespaces(), it does the same "get_xxx() in advance", but > -EPERM forgets to do put_nsproxy(), so we definitely have a leak in copy_process(). Ugh, I am sorry, EPERM does put_nsproxy(). Still I can't understand why copy_namespaces() does get_nsproxy() unconditionally. Oleg. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2007-06-17T14:49:15-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On 06/17, Oleg Nesterov wrote: > > However, nsproxy's code is full of strange unneeded get/put calls, for > example: > > struct uts_namespace *copy_utsname(int flags, struct uts_namespace *old_ns) > { > struct uts_namespace *new_ns; > > BUG_ON(!old_ns); > get_uts_ns(old_ns); > > if (!(flags & CLONE_NEWUTS)) > return old_ns; > > new_ns = clone_uts_ns(old_ns); > > put_uts_ns(old_ns); > return new_ns; > } Perhaps I missed something again, but this looks wrong to me. copy_utsname() assumes that old_ns != NULL. OK, it should not. However, clone_uts_ns() returns NULL if kmalloc() fails. create_new_namespaces() checks IS_ERR(new_ns), but IS_ERR(NULL) = false. So the next copy_namespaces/unshare_nsproxy_namespaces will oops ? The same for all ->xxx_ns fields. Oleg. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2007-06-17T16:30:04-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On Sun, Jun 17, 2007 at 06:38:30PM +0400, Oleg Nesterov wrote: > On 06/16, Herbert Poetzl wrote: > > > > On Tue, May 08, 2007 at 07:45:35PM -0700, akpm@linux-foundation.org wrote: > > > > > > The patch titled > > > Merge sys_clone()/sys_unshare() nsproxy and namespace handling > > > has been removed from the -mm tree. Its filename was > > > merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch > > > > > > This patch was dropped because it was merged into mainline or a subsystem tree > > > > > > > .. [zapped] ... > > > > > + * Called from unshare. Unshare all the namespaces part of nsproxy. > > > + * On sucess, returns the new nsproxy and a reference to old nsproxy > > > + * to make sure it stays around. > > > + */ > > > +int unshare_nsproxy_namespaces(unsigned long unshare_flags, > > > + struct nsproxy **new_nsp, struct fs_struct *new_fs) > > > +{ > > > > this makes sys_unshare leak and nsproxy (reference) > > > > can be tested with the following command sequence: > > vcmd -nu ^17 -- vcmd -nu ^17 -- sleep 10 > > I know almost nothing about this stuff, could you please explain in > brief what this command does ... yeah, sure, it basically calls sys_unshare() with bit 17 (CLONE_NEWNS) set then invokes the chained command, so we get a sleep which is in a separate namespace, unshared from a namespace != the main one ... > ... and how do you detect a leak? > > (and some nsproxy accounting/debugging as used in > > Linux-VServer) on Linux-VServer,we have accounting for those proxies (and several other namespace related stuff) because we already suspected leakage and reference bugs in this area some time ago ... btw, I also suggested to put a similar functionality in mainline for the time being, but it was ignored, as usual ... > > we probably want to drop the reference to the old > > nsproxy in sys_unshare() but I do not see a good reason > > to take the reference in the first place (at least not > > with the code in mainline 2.6.22-rc4) > > At first glance, sys_unshare(…
creator: Herbert Poetzl
date: 2007-06-17T17:09:42-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On 06/17, Herbert Poetzl wrote: > > On Sun, Jun 17, 2007 at 06:38:30PM +0400, Oleg Nesterov wrote: > > > > At first glance, sys_unshare() drops the reference to > > the old nsproxy, > > okay, the 'current' task has an nsproxy, and keeps > a reference to that (let's assume it is the only > task using this nsproxy, then the count will be 1) > > unshare_nsproxy_namespaces() now does get_nsproxy() > which makes the count=2, then it creates a new > nsproxy (which will get count=1), and returns ... Ah yes, stupid me. Looks like we should just remove get/put old_ns from unshare_nsproxy_namespaces() as you suggested. Thanks! Oleg. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2007-06-17T18:28:22-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: Oleg Nesterov wrote: > On 06/17, Oleg Nesterov wrote: >> Let's look at copy_namespaces(), it does the same "get_xxx() in advance", but >> -EPERM forgets to do put_nsproxy(), so we definitely have a leak in copy_process(). > > Ugh, I am sorry, EPERM does put_nsproxy(). Still I can't understand why > copy_namespaces() does get_nsproxy() unconditionally. well, if you're cloning a new task and not unsharing some of the namespaces you still want to increase the refcount on the nsproxy bc a new task is now referencing it. nop ? C. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Cedric Le Goater
date: 2007-06-18T11:51:20-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: > on Linux-VServer,we have accounting for those > proxies (and several other namespace related stuff) > because we already suspected leakage and reference > bugs in this area some time ago ... btw, I also > suggested to put a similar functionality in mainline > for the time being, but it was ignored, as usual ... something like a kmem_cache ? and we are not ignoring vserver ! :) Cheers, C. Add a kmem_cache to manage nsproxy objects. Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> --- kernel/nsproxy.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) Index: 2.6.22-rc4-mm2/kernel/nsproxy.c =================================================================== --- 2.6.22-rc4-mm2.orig/kernel/nsproxy.c +++ 2.6.22-rc4-mm2/kernel/nsproxy.c @@ -21,6 +21,8 @@ #include <linux/utsname.h> #include <linux/pid_namespace.h> +static struct kmem_cache *nsproxy_cachep; + struct nsproxy init_nsproxy = INIT_NSPROXY(init_nsproxy); static inline void get_nsproxy(struct nsproxy *ns) @@ -43,9 +45,11 @@ static inline struct nsproxy *clone_nspr { struct nsproxy *ns; - ns = kmemdup(orig, sizeof(struct nsproxy), GFP_KERNEL); - if (ns) + ns = kmem_cache_alloc(nsproxy_cachep, GFP_KERNEL); + if (ns) { + memcpy(ns, orig, sizeof(struct nsproxy)); atomic_set(&ns->count, 1); + } return ns; } @@ -109,7 +113,7 @@ out_uts: if (new_nsp->mnt_ns) put_mnt_ns(new_nsp->mnt_ns); out_ns: - kfree(new_nsp); + kmem_cache_free(nsproxy_cachep, new_nsp); return ERR_PTR(err); } @@ -160,7 +164,7 @@ void free_nsproxy(struct nsproxy *ns) put_pid_ns(ns->pid_ns); if (ns->user_ns) put_user_ns(ns->user_ns); - kfree(ns); + kmem_cache_free(nsproxy_cachep, ns); } /* @@ -191,3 +195,12 @@ int unshare_nsproxy_namespaces(unsigned } return err; } + +static int __init nsproxy_cache_init(void) +{ + nsproxy_cachep = kmem_cache_create("nsproxy", sizeof(struct nsproxy), + 0, SLAB_PANIC, NULL, NULL); + return 0; +} + +module_init(nsproxy_cache_init); _______________________________________________ Containers mailin…
creator: Cedric Le Goater
date: 2007-06-18T12:02:19-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On 06/18, Cedric Le Goater wrote: > > Oleg Nesterov wrote: > > On 06/17, Oleg Nesterov wrote: > >> Let's look at copy_namespaces(), it does the same "get_xxx() in advance", but > >> -EPERM forgets to do put_nsproxy(), so we definitely have a leak in copy_process(). > > > > Ugh, I am sorry, EPERM does put_nsproxy(). Still I can't understand why > > copy_namespaces() does get_nsproxy() unconditionally. > > well, if you're cloning a new task and not unsharing some of the namespaces > you still want to increase the refcount on the nsproxy bc a new task is now > referencing it. nop ? Yes, but copy_namespaces() does get_nsproxy() unconditionally, and then it does put_nsproxy() if not unsharing, which is not good imho. IOW, I think the patch below makes the code a bit better. copy_namespaces() doesn't need put_nsproxy() at all. Oleg. --- t/kernel/nsproxy.c~ 2007-06-18 16:10:53.000000000 +0400 +++ t/kernel/nsproxy.c 2007-06-18 16:13:02.000000000 +0400 @@ -103,31 +103,24 @@ int copy_namespaces(int flags, struct ta { struct nsproxy *old_ns = tsk->nsproxy; struct nsproxy *new_ns; - int err = 0; if (!old_ns) return 0; - get_nsproxy(old_ns); - - if (!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC))) + if (!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC))) { + get_nsproxy(old_ns); return 0; - - if (!capable(CAP_SYS_ADMIN)) { - err = -EPERM; - goto out; } + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + new_ns = create_new_namespaces(flags, tsk, tsk->fs); - if (IS_ERR(new_ns)) { - err = PTR_ERR(new_ns); - goto out; - } + if (IS_ERR(new_ns)) + return PTR_ERR(new_ns); tsk->nsproxy = new_ns; -out: - put_nsproxy(old_ns); - return err; + return 0; } void free_nsproxy(struct nsproxy *ns) _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2007-06-18T12:21:48-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: Oleg Nesterov wrote: > On 06/18, Cedric Le Goater wrote: >> Oleg Nesterov wrote: >>> On 06/17, Oleg Nesterov wrote: >>>> Let's look at copy_namespaces(), it does the same "get_xxx() in advance", but >>>> -EPERM forgets to do put_nsproxy(), so we definitely have a leak in copy_process(). >>> Ugh, I am sorry, EPERM does put_nsproxy(). Still I can't understand why >>> copy_namespaces() does get_nsproxy() unconditionally. >> well, if you're cloning a new task and not unsharing some of the namespaces >> you still want to increase the refcount on the nsproxy bc a new task is now >> referencing it. nop ? > > Yes, but copy_namespaces() does get_nsproxy() unconditionally, and then it does > put_nsproxy() if not unsharing, which is not good imho. > > IOW, I think the patch below makes the code a bit better. copy_namespaces() > doesn't need put_nsproxy() at all. Indeed it does and also : nsproxy.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) Thanks, C. > Oleg. > > --- t/kernel/nsproxy.c~ 2007-06-18 16:10:53.000000000 +0400 > +++ t/kernel/nsproxy.c 2007-06-18 16:13:02.000000000 +0400 > @@ -103,31 +103,24 @@ int copy_namespaces(int flags, struct ta > { > struct nsproxy *old_ns = tsk->nsproxy; > struct nsproxy *new_ns; > - int err = 0; > > if (!old_ns) > return 0; > > - get_nsproxy(old_ns); > - > - if (!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC))) > + if (!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC))) { > + get_nsproxy(old_ns); > return 0; > - > - if (!capable(CAP_SYS_ADMIN)) { > - err = -EPERM; > - goto out; > } > > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > new_ns = create_new_namespaces(flags, tsk, tsk->fs); > - if (IS_ERR(new_ns)) { > - err = PTR_ERR(new_ns); > - goto out; > - } > + if (IS_ERR(new_ns)) > + return PTR_ERR(new_ns); > > tsk->nsproxy = new_ns; > -out: > - put_nsproxy(old_ns); > - return err; > + return 0; > } > > void free_nsproxy(struct nsproxy *ns) > > _____________________________________________…
creator: Cedric Le Goater
date: 2007-06-18T12:25:28-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: Herbert Poetzl wrote: > On Sun, Jun 17, 2007 at 06:38:30PM +0400, Oleg Nesterov wrote: >> On 06/16, Herbert Poetzl wrote: >>> On Tue, May 08, 2007 at 07:45:35PM -0700, akpm@linux-foundation.org wrote: >>>> The patch titled >>>> Merge sys_clone()/sys_unshare() nsproxy and namespace handling >>>> has been removed from the -mm tree. Its filename was >>>> merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch >>>> >>>> This patch was dropped because it was merged into mainline or a subsystem tree >>>> >>> .. [zapped] ... >>> >>>> + * Called from unshare. Unshare all the namespaces part of nsproxy. >>>> + * On sucess, returns the new nsproxy and a reference to old nsproxy >>>> + * to make sure it stays around. >>>> + */ >>>> +int unshare_nsproxy_namespaces(unsigned long unshare_flags, >>>> + struct nsproxy **new_nsp, struct fs_struct *new_fs) >>>> +{ >>> this makes sys_unshare leak and nsproxy (reference) >>> >>> can be tested with the following command sequence: >>> vcmd -nu ^17 -- vcmd -nu ^17 -- sleep 10 >> I know almost nothing about this stuff, could you please explain in >> brief what this command does ... > > yeah, sure, it basically calls sys_unshare() with > bit 17 (CLONE_NEWNS) set then invokes the chained > command, so we get a sleep which is in a separate > namespace, unshared from a namespace != the main > one ... > >> ... and how do you detect a leak? > >>> (and some nsproxy accounting/debugging as used in >>> Linux-VServer) > > on Linux-VServer,we have accounting for those > proxies (and several other namespace related stuff) > because we already suspected leakage and reference > bugs in this area some time ago ... btw, I also > suggested to put a similar functionality in mainline > for the time being, but it was ignored, as usual ... > >>> we probably want to drop the reference to the old >>> nsproxy in sys_unshare() but I do not see a good reason >>> to take the reference in the first place (at least not >>> with the code in mainline 2.6.22-rc4) >> At f…
creator: Cedric Le Goater
date: 2007-06-18T12:37:25-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On Mon, Jun 18, 2007 at 02:37:25PM +0200, Cedric Le Goater wrote: > Herbert Poetzl wrote: > > On Sun, Jun 17, 2007 at 06:38:30PM +0400, Oleg Nesterov wrote: > >> On 06/16, Herbert Poetzl wrote: > >>> On Tue, May 08, 2007 at 07:45:35PM -0700, akpm@linux-foundation.org wrote: > >>>> The patch titled > >>>> Merge sys_clone()/sys_unshare() nsproxy and namespace handling > >>>> has been removed from the -mm tree. Its filename was > >>>> merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch > >>>> > >>>> This patch was dropped because it was merged into mainline or a subsystem tree > >>>> > >>> .. [zapped] ... > >>> > >>>> + * Called from unshare. Unshare all the namespaces part of nsproxy. > >>>> + * On sucess, returns the new nsproxy and a reference to old nsproxy > >>>> + * to make sure it stays around. > >>>> + */ > >>>> +int unshare_nsproxy_namespaces(unsigned long unshare_flags, > >>>> + struct nsproxy **new_nsp, struct fs_struct *new_fs) > >>>> +{ > >>> this makes sys_unshare leak and nsproxy (reference) > >>> > >>> can be tested with the following command sequence: > >>> vcmd -nu ^17 -- vcmd -nu ^17 -- sleep 10 > >> I know almost nothing about this stuff, could you please explain in > >> brief what this command does ... > > > > yeah, sure, it basically calls sys_unshare() with > > bit 17 (CLONE_NEWNS) set then invokes the chained > > command, so we get a sleep which is in a separate > > namespace, unshared from a namespace != the main > > one ... > > > >> ... and how do you detect a leak? > > > >>> (and some nsproxy accounting/debugging as used in > >>> Linux-VServer) > > > > on Linux-VServer,we have accounting for those > > proxies (and several other namespace related stuff) > > because we already suspected leakage and reference > > bugs in this area some time ago ... btw, I also > > suggested to put a similar functionality in mainline > > for the time being, but it was ignored, as usual ... > > > >>> we probably want to drop the reference to the old > >>> nspr…
creator: Herbert Poetzl
date: 2007-06-18T15:38:41-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On Mon, Jun 18, 2007 at 02:02:19PM +0200, Cedric Le Goater wrote: > > > on Linux-VServer,we have accounting for those > > proxies (and several other namespace related stuff) > > because we already suspected leakage and reference > > bugs in this area some time ago ... btw, I also > > suggested to put a similar functionality in mainline > > for the time being, but it was ignored, as usual ... > > something like a kmem_cache ? maybe, but even simplest accounting of the different 'objects' would be more then enough for the test phase (or maybe as statistics :) > and we are not ignoring vserver ! :) good for them, our project is called Linux-VServer :) best, Herbert > Cheers, > > C. > > Add a kmem_cache to manage nsproxy objects. > > Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> > --- > kernel/nsproxy.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > Index: 2.6.22-rc4-mm2/kernel/nsproxy.c > =================================================================== > --- 2.6.22-rc4-mm2.orig/kernel/nsproxy.c > +++ 2.6.22-rc4-mm2/kernel/nsproxy.c > @@ -21,6 +21,8 @@ > #include <linux/utsname.h> > #include <linux/pid_namespace.h> > > +static struct kmem_cache *nsproxy_cachep; > + > struct nsproxy init_nsproxy = INIT_NSPROXY(init_nsproxy); > > static inline void get_nsproxy(struct nsproxy *ns) > @@ -43,9 +45,11 @@ static inline struct nsproxy *clone_nspr > { > struct nsproxy *ns; > > - ns = kmemdup(orig, sizeof(struct nsproxy), GFP_KERNEL); > - if (ns) > + ns = kmem_cache_alloc(nsproxy_cachep, GFP_KERNEL); > + if (ns) { > + memcpy(ns, orig, sizeof(struct nsproxy)); > atomic_set(&ns->count, 1); > + } > return ns; > } > > @@ -109,7 +113,7 @@ out_uts: > if (new_nsp->mnt_ns) > put_mnt_ns(new_nsp->mnt_ns); > out_ns: > - kfree(new_nsp); > + kmem_cache_free(nsproxy_cachep, new_nsp); > return ERR_PTR(err); > } > > @@ -160,7 +164,7 @@ void free_nsproxy(struct nsproxy *ns) > put_pid_ns(ns->pid_ns); > if (ns->user_ns) > put_user_ns(ns->user_ns); > - kfree(…
creator: Herbert Poetzl
date: 2007-06-18T15:41:08-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On Mon, Jun 18, 2007 at 02:37:25PM +0200, Cedric Le Goater wrote: > Herbert Poetzl wrote: > > On Sun, Jun 17, 2007 at 06:38:30PM +0400, Oleg Nesterov wrote: > >> On 06/16, Herbert Poetzl wrote: > >>> On Tue, May 08, 2007 at 07:45:35PM -0700, akpm@linux-foundation.org wrote: > >>>> The patch titled > >>>> Merge sys_clone()/sys_unshare() nsproxy and namespace handling > >>>> has been removed from the -mm tree. Its filename was > >>>> merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch > >>>> > >>>> This patch was dropped because it was merged into mainline or a subsystem tree > >>>> > >>> .. [zapped] ... > >>> > >>>> + * Called from unshare. Unshare all the namespaces part of nsproxy. > >>>> + * On sucess, returns the new nsproxy and a reference to old nsproxy > >>>> + * to make sure it stays around. > >>>> + */ > >>>> +int unshare_nsproxy_namespaces(unsigned long unshare_flags, > >>>> + struct nsproxy **new_nsp, struct fs_struct *new_fs) > >>>> +{ > >>> this makes sys_unshare leak and nsproxy (reference) > >>> > >>> can be tested with the following command sequence: > >>> vcmd -nu ^17 -- vcmd -nu ^17 -- sleep 10 > >> I know almost nothing about this stuff, could you please explain in > >> brief what this command does ... > > > > yeah, sure, it basically calls sys_unshare() with > > bit 17 (CLONE_NEWNS) set then invokes the chained > > command, so we get a sleep which is in a separate > > namespace, unshared from a namespace != the main > > one ... > > > >> ... and how do you detect a leak? > > > >>> (and some nsproxy accounting/debugging as used in > >>> Linux-VServer) > > > > on Linux-VServer,we have accounting for those > > proxies (and several other namespace related stuff) > > because we already suspected leakage and reference > > bugs in this area some time ago ... btw, I also > > suggested to put a similar functionality in mainline > > for the time being, but it was ignored, as usual ... > > > >>> we probably want to drop the reference to the old > >>> nspr…
creator: Herbert Poetzl
date: 2007-06-18T15:48:05-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: [ ... ] >> If we're all happy with these fixes, i'll send them on lkml@ for review. >> They might deserve to be in 2.6.22. > > the patch looks like it should fix the issue > (will test that soon) but it leaves the comment > unmodified, which is now wrong ... > > * Called from unshare. Unshare all the namespaces part of nsproxy. > * On sucess, returns the new nsproxy and a reference to old nsproxy > * to make sure it stays around. right. I will fix that. Thanks, C. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Cedric Le Goater
date: 2007-06-18T16:44:57-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: [ ... ] >> It fixes the leak for me. I've run the ltp tests we >> have on namespace unsharing and i could see the no >> leaks in /proc/slabinfo. > >> Badari, >> >> That extra get_nsproxy() seemed a superfluous remain >> from the 2.6.20. >> Do you see any issues with it ? >> >> If we're all happy with these fixes, i'll send them on >> lkml@ for review. > > I'm not terribly happy with the current nsproxy > framework, although it improved somewhat ... > > I'm still missing some mechanism to 'mix' two > proxies according to a flagmask (which is required > to enter a guest 'partially') ... We have that bind_ns() syscall that does that. I sent it last year but it didn't have much success. We still use and there may be room for improvement to make it altruistically useful. Here's the patch on a 2.6.21-mm2. It it's of any interest, I can refresh it on the latest -mm. Thanks, C. The following patch defines the new bind_ns syscall specific to nsproxy and namespaces, which allows a process to bind : 1 - its nsproxy to some identifier 2 - to another nsproxy using an identifier or -pid Here's a sample user space program to use it. #include <stdio.h> #include <stdlib.h> #include <sched.h> #include <unistd.h> #include <string.h> #include <errno.h> #include <libgen.h> #include <sys/syscall.h> #ifndef HAVE_UNSHARE #if __i386__ # define __NR_unshare 310 #elif __x86_64__ # define __NR_unshare 272 #elif __ia64__ # define __NR_unshare 1296 #elif __s390x__ # define __NR_unshare 303 #elif __powerpc__ # define __NR_unshare 282 #else # error "Architecture not supported" #endif #endif /* HAVE_UNSHARE */ #if __i386__ # define __NR_bind_ns 326 #elif __ia64__ # define __NR_bind_ns 1305 #elif __powerpc__ # define __NR_bind_ns 304 #elif __s390x__ # define __NR_bind_ns 314 #elif __x86_64__ # define __NR_bind_ns 284 #else # error "Architecture not supported" #endif #ifndef CLONE_NEWUTS #define CLONE_NEWUTS 0x04000000 #endif #ifndef CLONE_NEWIPC #define CLONE_NEWIPC 0x08000000 #endif #ifndef CLONE_N…
creator: Cedric Le Goater
date: 2007-06-18T16:54:33-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: Herbert Poetzl wrote: > On Mon, Jun 18, 2007 at 02:02:19PM +0200, Cedric Le Goater wrote: >>> on Linux-VServer,we have accounting for those >>> proxies (and several other namespace related stuff) >>> because we already suspected leakage and reference >>> bugs in this area some time ago ... btw, I also >>> suggested to put a similar functionality in mainline >>> for the time being, but it was ignored, as usual ... >> something like a kmem_cache ? > > maybe, but even simplest accounting of the > different 'objects' would be more then enough > for the test phase (or maybe as statistics :) Sure but Linux-VServer would certainly beneficiate from a nsproxy cache. Some scenarii do a lot of unshare(), nop ? Anyway, i wouldn't know how to measure the difference :) but it certainly helped me to identify the leak real fast ! >> and we are not ignoring vserver ! :) > > good for them, our project is called Linux-VServer :) arg :) Cheers, C. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Cedric Le Goater
date: 2007-06-18T17:00:04-00:00

Re: - merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch removed from -mm tree
type: item
description: On Mon, 2007-06-18 at 14:37 +0200, Cedric Le Goater wrote: > Herbert Poetzl wrote: > > On Sun, Jun 17, 2007 at 06:38:30PM +0400, Oleg Nesterov wrote: > >> On 06/16, Herbert Poetzl wrote: > >>> On Tue, May 08, 2007 at 07:45:35PM -0700, akpm@linux-foundation.org wrote: > >>>> The patch titled > >>>> Merge sys_clone()/sys_unshare() nsproxy and namespace handling > >>>> has been removed from the -mm tree. Its filename was > >>>> merge-sys_clone-sys_unshare-nsproxy-and-namespace.patch > >>>> > >>>> This patch was dropped because it was merged into mainline or a subsystem tree > >>>> > >>> .. [zapped] ... > >>> > >>>> + * Called from unshare. Unshare all the namespaces part of nsproxy. > >>>> + * On sucess, returns the new nsproxy and a reference to old nsproxy > >>>> + * to make sure it stays around. > >>>> + */ > >>>> +int unshare_nsproxy_namespaces(unsigned long unshare_flags, > >>>> + struct nsproxy **new_nsp, struct fs_struct *new_fs) > >>>> +{ > >>> this makes sys_unshare leak and nsproxy (reference) > >>> > >>> can be tested with the following command sequence: > >>> vcmd -nu ^17 -- vcmd -nu ^17 -- sleep 10 > >> I know almost nothing about this stuff, could you please explain in > >> brief what this command does ... > > > > yeah, sure, it basically calls sys_unshare() with > > bit 17 (CLONE_NEWNS) set then invokes the chained > > command, so we get a sleep which is in a separate > > namespace, unshared from a namespace != the main > > one ... > > > >> ... and how do you detect a leak? > > > >>> (and some nsproxy accounting/debugging as used in > >>> Linux-VServer) > > > > on Linux-VServer,we have accounting for those > > proxies (and several other namespace related stuff) > > because we already suspected leakage and reference > > bugs in this area some time ago ... btw, I also > > suggested to put a similar functionality in mainline > > for the time being, but it was ignored, as usual ... > > > >>> we probably want to drop the reference to the old > >>> nsproxy in …
creator: Badari Pulavarty
date: 2007-06-18T17:35:14-00:00

[PATCH] create_new_namespaces: fix improper return of NULL
type: item
description: Untested. dup_mnt_ns() and clone_uts_ns() return NULL on failure. This is wrong, create_new_namespaces() uses ERR_PTR() to catch an error. This means that the subsequent create_new_namespaces() will hit BUG_ON() in copy_mnt_ns() or copy_utsname(). Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru> --- ns/fs/namespace.c~1_NS_NULL 2007-05-21 13:57:56.000000000 +0400 +++ ns/fs/namespace.c 2007-06-19 17:26:35.000000000 +0400 @@ -1457,7 +1457,7 @@ static struct mnt_namespace *dup_mnt_ns( new_ns = kmalloc(sizeof(struct mnt_namespace), GFP_KERNEL); if (!new_ns) - return NULL; + return ERR_PTR(-ENOMEM); atomic_set(&new_ns->count, 1); INIT_LIST_HEAD(&new_ns->list); @@ -1471,7 +1471,7 @@ static struct mnt_namespace *dup_mnt_ns( if (!new_ns->root) { up_write(&namespace_sem); kfree(new_ns); - return NULL; + return ERR_PTR(-ENOMEM); } spin_lock(&vfsmount_lock); list_add_tail(&new_ns->list, &new_ns->root->mnt_list); --- ns/kernel/utsname.c~1_NS_NULL 2007-05-21 13:57:59.000000000 +0400 +++ ns/kernel/utsname.c 2007-06-19 17:35:22.000000000 +0400 @@ -13,6 +13,7 @@ #include <linux/uts.h> #include <linux/utsname.h> #include <linux/version.h> +#include <linux/err.h> /* * Clone a new ns copying an original utsname, setting refcount to 1 @@ -24,10 +25,11 @@ static struct uts_namespace *clone_uts_n struct uts_namespace *ns; ns = kmalloc(sizeof(struct uts_namespace), GFP_KERNEL); - if (ns) { - memcpy(&ns->name, &old_ns->name, sizeof(ns->name)); - kref_init(&ns->kref); - } + if (!ns) + return ERR_PTR(-ENOMEM); + + memcpy(&ns->name, &old_ns->name, sizeof(ns->name)); + kref_init(&ns->kref); return ns; } _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2007-06-19T13:51:29-00:00