From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Drewry Subject: Re: [PATCH 2/2] exec: move core_pattern pipe helper into the crashing namespace Date: Fri, 17 Sep 2010 22:14:23 -0500 Message-ID: References: <20100917152639.0e88341a@basil.nowhere.org> <1284736618-27153-2-git-send-email-wad@chromium.org> <20100918012939.GA25046@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Andi Kleen , linux-kernel@vger.kernel.org, Alexander Viro , Andrew Morton , KOSAKI Motohiro , Roland McGrath , Neil Horman , "Eric W. Biederman" , containers@lists.linux-foundation.org, Eugene Teo , Tejun Heo , Serge Hallyn , Alexey Dobriyan , linux-fsdevel@vger.kernel.org To: Oleg Nesterov Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Fri, Sep 17, 2010 at 9:34 PM, Will Drewry wrote: > On Fri, Sep 17, 2010 at 8:29 PM, Oleg Nesterov wrot= e: >> On 09/17, Will Drewry wrote: >>> >>> Instead, this change implements the more complex option two. =A0It >>> migrates the ____call_usermodehelper() thread into the same namespa= ces >>> as the dumping process. =A0It does not assign a pid in that namespa= ce so >>> the collector will appear to be pid 0. >> >> Hmm... You mean, it won't visible in that namespace? Afacis, it shou= ld >> have the correct pid in the init ns, no? > > Exactly - it will be unmapped in its new pid namespace, returning 0 > only in that case, as you point out below! > >> >> I am a bit worried task_active_pid_ns() !=3D nsproxy->pid_ns, but pe= rhaps >> this is OK... Say, sys_getpid() returns 0, strange. >> >>> + =A0 =A0 =A0 =A0 =A0 =A0 /* Run the core_collector in the crashing= namespaces */ >>> + =A0 =A0 =A0 =A0 =A0 =A0 if (copy_namespaces_unattached(0, current= , >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &pipe_params.nsproxy, &pi= pe_params.fs)) { >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING "%s f= ailed to copy namespaces\n", >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0__func__); >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argv_free(helper_argv); >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto fail_dropcount; >>> + =A0 =A0 =A0 =A0 =A0 =A0 } >> >> This looks overcomplicated to me, or I missed something. >> >> I do not understand why should we do this beforehand, and why we nee= d >> copy_namespaces_unattached(). >> >> Can't you just pass current to umh_pipe_setup() (or another helper) = as >> the argument? Then this helper can copy ->fs and ->nsproxy itself. > > I wasn't sure if it was reasonable to pass the current task_struct > over, but I certainly can. > >> In fact, I do not understand why create_new_namespaces() is used. It >> is called with flags =3D=3D 0 anyway, can't we just do >> >> =A0 =A0 =A0 =A0ns =3D coredumping_task->nsproxy; >> =A0 =A0 =A0 =A0get_nsproxy(ns); >> =A0 =A0 =A0 =A0switch_task_namespaces(current, ns); >> >> ? > > So that was my first thought (which I tried). =A0I did exactly what y= ou > suggested to the khelper thread, and the lack of the fs struct bit me= =2E > =A0Since the older patch from Eric Biederman (setns()) had taken the > route of deflecting the work through create_new_namespaces(), I did > too. =A0I figured it would ensure that any namespacing behavior that > needed to be done would be done. > > In practice, this seems to amount to just adding a refcount to all th= e > namespaces and creating a new nsproxy which isn't really needed. =A0M= ost > likely, doing what you've suggested above plus the copy_fs_struct and > the swap out will do the trick. =A0I'll try it out and see. =A0That's= make > it much clearer I think. That seems to work -- I'll post an update shortly with your comments and Neil's integrated - hopefully accurately. Any other thoughts on style/cleanup/etc will certainly be appreciated. cheers! will