linux-kernel - Re: [PATCH] procfs: Do not release pid_ns->proc

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]

Message-ID: <20100617142040.GB7400@localdomain>
Date:	Thu, 17 Jun 2010 16:20:40 +0200
From:	Louis Rilling <Louis.Rilling@...labs.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Pavel Emelyanov <xemul@...allels.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Pavel Emelyanov <xemul@...nvz.org>,
	Linux Containers <containers@...ts.osdl.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] procfs: Do not release pid_ns->proc_mnt too early

On Thu, Jun 17, 2010 at 06:41:49AM -0700, Eric W. Biederman wrote:
> Pavel Emelyanov <xemul@...allels.com> writes:
> 
> > On 06/16/2010 08:34 PM, Louis Rilling wrote:
> >> [ Resending, hopefully with all pieces ]
> >> 
> >> Detached tasks are not seen by zap_pid_ns_processes()->sys_wait4(), so
> >> that release_task()->proc_flush_task() of container init can be called
> >> before it is for some detached tasks in the namespace.
> 
> There are two ways we can go from here.
> 
> - Completely asynchronous children exiting.
> - Waiting for all of our children to exit.

Agreed.

> 
> Your patch appears to be a weird middle ground, that is hard to
> analyze, abusing the mount count as a thread count.
> 
> I have been weighing the options between them, and to me properly
> waiting for all the processes to exit in zap_pid_ns_processes as we
> currently try to do is in our advantage.  It is simple and it means
> one less cache line bounce for a write to global variable in the
> much more common fork/exit path that we have to deal with.
> 
> The task->children isn't changed until __unhash_process() which runs
> after flush_proc_task().  So we should be able to come up with
> a variant of do_wait() that zap_pid_ns_processes can use that does
> what we need.

Sounds correct.

> 
> Louis do you want to look at that?

Give me a few days to look at that. IMHO my patch fixes the bug (see the
comment below), which was an emergency at work. Coding an improved fix is
lower priority :)

> 
> >> Pin proc_mnt's in copy_process(), so that proc_flush_task() becomes safe
> >> whatever the ordering of tasks.
> >
> > See one comment below.
> >
> >> Signed-off-by: Louis Rilling <louis.rilling@...labs.com>
> >> ---
> >>  fs/proc/base.c          |   18 ++++++++++++++++++
> >>  include/linux/proc_fs.h |    4 ++++
> >>  kernel/fork.c           |    1 +
> >>  3 files changed, 23 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> index acb7ef8..d6cdd91 100644
> >> --- a/fs/proc/base.c
> >> +++ b/fs/proc/base.c
> >> @@ -2663,6 +2663,23 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
> >>  	.setattr	= proc_setattr,
> >>  };
> >>  
> >> +/*
> >> + * Pin all proc_mnt so that detached tasks can safely call proc_flush_task()
> >> + * after container init calls itself proc_flush_task().
> >> + */
> >> +void proc_new_task(struct task_struct *task)
> >> +{
> >> +	struct pid *pid;
> >> +	int i;
> >> +
> >> +	if (!task->pid)
> >> +		return;
> >> +
> >> +	pid = task_pid(task);
> >> +	for (i = 0; i <= pid->level; i++)
> >> +		mntget(pid->numbers[i].ns->proc_mnt);
> >
> > NAK. Pids live their own lives - task can change one, pid will
> > become orphan and will be destroyed, so you'll leak.
> 
> There is that nasty case in exec isn't there.  Why we ever made it
> part of the ABI that calling exec on a thread changes the pid of
> that thread to the pid of the thread group is beyond me.

You're right that I forgot about de_thread(). However, de_thread() does not
replace a task pid with an arbitrary other pid. The new pid lives in the same
pid namespaces, so that proc_flush_task() will call mntput() on the same
proc_mnts as the ones on which proc_new_task() called mntget().

Thanks,

Louis

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)