linux-kernel - Re: [PATCH 2/2] exec: move core_pattern pipe helper into the crashing namespace

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 17 Sep 2010 21:34:16 -0500
From:	Will Drewry <wad@...omium.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andi Kleen <andi@...stfloor.org>, linux-kernel@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Roland McGrath <roland@...hat.com>,
	Neil Horman <nhorman@...driver.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	containers@...ts.linux-foundation.org,
	Eugene Teo <eteo@...hat.com>, Tejun Heo <tj@...nel.org>,
	Serge Hallyn <serue@...ibm.com>,
	Alexey Dobriyan <adobriyan@...il.com>,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 2/2] exec: move core_pattern pipe helper into the crashing namespace

On Fri, Sep 17, 2010 at 8:29 PM, Oleg Nesterov <oleg@...hat.com> wrote:
> On 09/17, Will Drewry wrote:
>>
>> Instead, this change implements the more complex option two.  It
>> migrates the ____call_usermodehelper() thread into the same namespaces
>> as the dumping process.  It does not assign a pid in that namespace so
>> the collector will appear to be pid 0.
>
> Hmm... You mean, it won't visible in that namespace? Afacis, it should
> 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() != nsproxy->pid_ns, but perhaps
> this is OK... Say, sys_getpid() returns 0, strange.
>
>> +             /* Run the core_collector in the crashing namespaces */
>> +             if (copy_namespaces_unattached(0, current,
>> +                     &pipe_params.nsproxy, &pipe_params.fs)) {
>> +                     printk(KERN_WARNING "%s failed to copy namespaces\n",
>> +                            __func__);
>> +                     argv_free(helper_argv);
>> +                     goto fail_dropcount;
>> +             }
>
> This looks overcomplicated to me, or I missed something.
>
> I do not understand why should we do this beforehand, and why we need
> 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 == 0 anyway, can't we just do
>
>        ns = coredumping_task->nsproxy;
>        get_nsproxy(ns);
>        switch_task_namespaces(current, ns);
>
> ?

So that was my first thought (which I tried).  I did exactly what you
suggested to the khelper thread, and the lack of the fs struct bit me.
 Since the older patch from Eric Biederman (setns()) had taken the
route of deflecting the work through create_new_namespaces(), I did
too.  I 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 the
namespaces and creating a new nsproxy which isn't really needed.  Most
likely, doing what you've suggested above plus the copy_fs_struct and
the swap out will do the trick.  I'll try it out and see.  That's make
it much clearer I think.

thanks!
will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/