Prev: [RFC][PATCH 0/8] perf pmu interface
Next: $100,000 OFFER
From: Oleg Nesterov on 18 Jun 2010 12:20
On 06/18, Oleg Nesterov wrote:
>
> On 06/18, Louis Rilling wrote:
> >
> > On 17/06/10 23:20 +0200, Oleg Nesterov wrote:
> > > On 06/16, Louis Rilling wrote:
> > > >
> > > > 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.
> > > >
> > > > Pin proc_mnt's in copy_process(), so that proc_flush_task() becomes safe
> > > > whatever the ordering of tasks.
> > >
> > > I must have missed something, but can't we just move mntput() ?
> >
> > See the log of the commit introducing pid_ns_release_proc() (6f4e6433):
> >
> > Sice the namespace holds the vfsmnt, vfsmnt holds the superblock and the
> > superblock holds the namespace we must explicitly break this circle to destroy
> > all the stuff. This is done after the init of the namespace dies.
>
> I see thanks.

Not sure I ever understood this code. Certainly I can't say I understand
it now. Still, do we really need this circle? I am almost sure the patch
below is not right (and it wasn't tested at all), but could you take a
look?

Note: afaics we have another problem. What if copy_process(CLONE_NEWPID)
fails after pid_ns_prepare_proc() ? Who will do mntput() ? This patch
should fix this too (again, ___if___ it correct).

Oleg.

include/linux/pid_namespace.h | 1 +
kernel/pid_namespace.c | 3 +++
fs/proc/base.c | 4 ----
fs/proc/root.c | 18 ++++++++++++++----
4 files changed, 18 insertions(+), 8 deletions(-)

--- 34-rc1/include/linux/pid_namespace.h~PID_NS 2010-06-18 17:48:56.000000000 +0200
+++ 34-rc1/include/linux/pid_namespace.h 2010-06-18 17:53:11.000000000 +0200
@@ -26,6 +26,7 @@ struct pid_namespace {
struct pid_namespace *parent;
#ifdef CONFIG_PROC_FS
struct vfsmount *proc_mnt;
+ struct work_struct proc_put;
#endif
#ifdef CONFIG_BSD_PROCESS_ACCT
struct bsd_acct_struct *bacct;
--- 34-rc1/kernel/pid_namespace.c~PID_NS 2010-06-18 17:48:56.000000000 +0200
+++ 34-rc1/kernel/pid_namespace.c 2010-06-18 17:53:44.000000000 +0200
@@ -10,6 +10,7 @@

#include <linux/pid.h>
#include <linux/pid_namespace.h>
+#include <linux/proc_fs.h>
#include <linux/syscalls.h>
#include <linux/err.h>
#include <linux/acct.h>
@@ -109,6 +110,8 @@ static void destroy_pid_namespace(struct
{
int i;

+ pid_ns_release_proc(ns);
+
for (i = 0; i < PIDMAP_ENTRIES; i++)
kfree(ns->pidmap[i].page);
kmem_cache_free(pid_ns_cachep, ns);
--- 34-rc1/fs/proc/base.c~PID_NS 2010-06-18 17:48:56.000000000 +0200
+++ 34-rc1/fs/proc/base.c 2010-06-18 17:49:45.000000000 +0200
@@ -2720,10 +2720,6 @@ void proc_flush_task(struct task_struct
proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
tgid->numbers[i].nr);
}
-
- upid = &pid->numbers[pid->level];
- if (upid->nr == 1)
- pid_ns_release_proc(upid->ns);
}

static struct dentry *proc_pid_instantiate(struct inode *dir,
--- 34-rc1/fs/proc/root.c~PID_NS 2010-06-18 17:48:56.000000000 +0200
+++ 34-rc1/fs/proc/root.c 2010-06-18 17:56:57.000000000 +0200
@@ -31,7 +31,7 @@ static int proc_set_super(struct super_b
struct pid_namespace *ns;

ns = (struct pid_namespace *)data;
- sb->s_fs_info = get_pid_ns(ns);
+ sb->s_fs_info = ns;
return set_anon_super(sb, NULL);
}

@@ -74,7 +74,7 @@ static int proc_get_sb(struct file_syste
ei = PROC_I(sb->s_root->d_inode);
if (!ei->pid) {
rcu_read_lock();
- ei->pid = get_pid(find_pid_ns(1, ns));
+ ei->pid = find_pid_ns(1, ns);
rcu_read_unlock();
}

@@ -92,7 +92,6 @@ static void proc_kill_sb(struct super_bl

ns = (struct pid_namespace *)sb->s_fs_info;
kill_anon_super(sb);
- put_pid_ns(ns);
}

static struct file_system_type proc_fs_type = {
@@ -216,7 +215,18 @@ int pid_ns_prepare_proc(struct pid_names
return 0;
}

-void pid_ns_release_proc(struct pid_namespace *ns)
+static void proc_mntput(struct work_struct *work)
{
+ struct pid_namespace *ns = container_of(work, struct pid_namespace, proc_put);
+
+ PROC_I(ns->proc_mnt->mnt_sb->s_root->d_inode)->pid = NULL;
mntput(ns->proc_mnt);
}
+
+void pid_ns_release_proc(struct pid_namespace *ns)
+{
+ if (ns->proc_mnt) {
+ INIT_WORK(&ns->proc_put, proc_mntput);
+ schedule_work(&ns->proc_put);
+ }
+}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 18 Jun 2010 13:40
On 06/18, Louis Rilling wrote:
>
> On 17/06/10 23:36 +0200, Oleg Nesterov wrote:
> > On 06/17, Eric W. Biederman wrote:
> > >
> > > The task->children isn't changed until __unhash_process() which runs
> > > after flush_proc_task().
> >
> > Yes. But this is only the current implementation detail.
> > It would be nice to cleanup the code so that EXIT_DEAD tasks are
> > never sit in ->children list.
> >
> > > 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.
> >
> > See above...
> >
> > Even if we modify do_wait() or add the new variant, how the caller
> > can wait for EXIT_DEAD tasks? I don't think we want to modify
> > release_task() to do __wake_up_parent() or something similar.
>
> Indeed, I was thinking about calling __wake_up_parent() from release_task()
> once parent->children becomes empty.
>
> Not sure about the performance impact though. Maybe some WAIT_NO_CHILDREN flag
> in parent->signal could limit it. But if EXIT_DEAD children are removed from
> ->children before release_task(), I'm afraid that this becomes impossible.

Thinking more, even the current do_wait() from zap_pid_ns_processes()
is not really good. Suppose that some none-init thread is ptraced, then
zap_pid_ns_processes() will hange until the tracer does do_wait() or
exits.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Louis Rilling on 18 Jun 2010 13:40
On 18/06/10 18:08 +0200, Oleg Nesterov wrote:
> On 06/18, Oleg Nesterov wrote:
> >
> > On 06/18, Louis Rilling wrote:
> > >
> > > On 17/06/10 23:20 +0200, Oleg Nesterov wrote:
> > > > On 06/16, Louis Rilling wrote:
> > > > >
> > > > > 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.
> > > > >
> > > > > Pin proc_mnt's in copy_process(), so that proc_flush_task() becomes safe
> > > > > whatever the ordering of tasks.
> > > >
> > > > I must have missed something, but can't we just move mntput() ?
> > >
> > > See the log of the commit introducing pid_ns_release_proc() (6f4e6433):
> > >
> > > Sice the namespace holds the vfsmnt, vfsmnt holds the superblock and the
> > > superblock holds the namespace we must explicitly break this circle to destroy
> > > all the stuff. This is done after the init of the namespace dies.
> >
> > I see thanks.
>
> Not sure I ever understood this code. Certainly I can't say I understand
> it now. Still, do we really need this circle? I am almost sure the patch
> below is not right (and it wasn't tested at all), but could you take a
> look?

I won't pretend understanding better than you do. Still I can try to give my 2
cents.

Overall, I don't feel comfortable at being able to have a living proc_mnt
with a dead pid_ns. On the contrary, proc_mnt is almost never used from pid_ns.
proc_flush_task() excepted, the only users I saw use
current->nsproxy->pid_ns->proc_mnt, which makes them safe.

>
> Note: afaics we have another problem. What if copy_process(CLONE_NEWPID)
> fails after pid_ns_prepare_proc() ? Who will do mntput() ? This patch
> should fix this too (again, ___if___ it correct).

Sounds true.

>
> Oleg.
>
> include/linux/pid_namespace.h | 1 +
> kernel/pid_namespace.c | 3 +++
> fs/proc/base.c | 4 ----
> fs/proc/root.c | 18 ++++++++++++++----
> 4 files changed, 18 insertions(+), 8 deletions(-)
>
> --- 34-rc1/include/linux/pid_namespace.h~PID_NS 2010-06-18 17:48:56.000000000 +0200
> +++ 34-rc1/include/linux/pid_namespace.h 2010-06-18 17:53:11.000000000 +0200
> @@ -26,6 +26,7 @@ struct pid_namespace {
> struct pid_namespace *parent;
> #ifdef CONFIG_PROC_FS
> struct vfsmount *proc_mnt;
> + struct work_struct proc_put;
> #endif
> #ifdef CONFIG_BSD_PROCESS_ACCT
> struct bsd_acct_struct *bacct;
> --- 34-rc1/kernel/pid_namespace.c~PID_NS 2010-06-18 17:48:56.000000000 +0200
> +++ 34-rc1/kernel/pid_namespace.c 2010-06-18 17:53:44.000000000 +0200
> @@ -10,6 +10,7 @@
>
> #include <linux/pid.h>
> #include <linux/pid_namespace.h>
> +#include <linux/proc_fs.h>
> #include <linux/syscalls.h>
> #include <linux/err.h>
> #include <linux/acct.h>
> @@ -109,6 +110,8 @@ static void destroy_pid_namespace(struct
> {
> int i;
>
> + pid_ns_release_proc(ns);
> +
> for (i = 0; i < PIDMAP_ENTRIES; i++)
> kfree(ns->pidmap[i].page);
> kmem_cache_free(pid_ns_cachep, ns);
> --- 34-rc1/fs/proc/base.c~PID_NS 2010-06-18 17:48:56.000000000 +0200
> +++ 34-rc1/fs/proc/base.c 2010-06-18 17:49:45.000000000 +0200
> @@ -2720,10 +2720,6 @@ void proc_flush_task(struct task_struct
> proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
> tgid->numbers[i].nr);
> }
> -
> - upid = &pid->numbers[pid->level];
> - if (upid->nr == 1)
> - pid_ns_release_proc(upid->ns);
> }
>
> static struct dentry *proc_pid_instantiate(struct inode *dir,
> --- 34-rc1/fs/proc/root.c~PID_NS 2010-06-18 17:48:56.000000000 +0200
> +++ 34-rc1/fs/proc/root.c 2010-06-18 17:56:57.000000000 +0200
> @@ -31,7 +31,7 @@ static int proc_set_super(struct super_b
> struct pid_namespace *ns;
>
> ns = (struct pid_namespace *)data;
> - sb->s_fs_info = get_pid_ns(ns);
> + sb->s_fs_info = ns;
> return set_anon_super(sb, NULL);
> }
>
> @@ -74,7 +74,7 @@ static int proc_get_sb(struct file_syste
> ei = PROC_I(sb->s_root->d_inode);
> if (!ei->pid) {
> rcu_read_lock();
> - ei->pid = get_pid(find_pid_ns(1, ns));
> + ei->pid = find_pid_ns(1, ns);

I don't think that this is correct. IIUC, proc_delete_inode() calls put_pid() on
ei->pid. So either a special case is added in proc_delete_inode(), or we try to
live with get_pid() here. I'm actually not sure that we can pretend that this
pid remains valid if we don't get_pid() here.

Thanks,

Louis

> rcu_read_unlock();
> }
>
> @@ -92,7 +92,6 @@ static void proc_kill_sb(struct super_bl
>
> ns = (struct pid_namespace *)sb->s_fs_info;
> kill_anon_super(sb);
> - put_pid_ns(ns);
> }
>
> static struct file_system_type proc_fs_type = {
> @@ -216,7 +215,18 @@ int pid_ns_prepare_proc(struct pid_names
> return 0;
> }
>
> -void pid_ns_release_proc(struct pid_namespace *ns)
> +static void proc_mntput(struct work_struct *work)
> {
> + struct pid_namespace *ns = container_of(work, struct pid_namespace, proc_put);
> +
> + PROC_I(ns->proc_mnt->mnt_sb->s_root->d_inode)->pid = NULL;
> mntput(ns->proc_mnt);
> }
> +
> +void pid_ns_release_proc(struct pid_namespace *ns)
> +{
> + if (ns->proc_mnt) {
> + INIT_WORK(&ns->proc_put, proc_mntput);
> + schedule_work(&ns->proc_put);
> + }
> +}
>
>

--
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
From: Oleg Nesterov on 18 Jun 2010 14:00
On 06/18, Louis Rilling wrote:
>
> On 18/06/10 18:08 +0200, Oleg Nesterov wrote:
> >
> > Not sure I ever understood this code. Certainly I can't say I understand
> > it now. Still, do we really need this circle? I am almost sure the patch
> > below is not right (and it wasn't tested at all), but could you take a
> > look?
>
> I won't pretend understanding better than you do. Still I can try to give my 2
> cents.
>
> Overall, I don't feel comfortable at being able to have a living proc_mnt
> with a dead pid_ns.

Yes, this should be fixed, I already realized this. work->func(ns) is
called when ns is already fixed.

Otherwise, nobody should use ns->proc_mount when ns is already dead/freed.

> > Note: afaics we have another problem. What if copy_process(CLONE_NEWPID)
> > fails after pid_ns_prepare_proc() ? Who will do mntput() ? This patch
> > should fix this too (again, ___if___ it correct).
>
> Sounds true.

Good.

> > @@ -74,7 +74,7 @@ static int proc_get_sb(struct file_syste
> > ei = PROC_I(sb->s_root->d_inode);
> > if (!ei->pid) {
> > rcu_read_lock();
> > - ei->pid = get_pid(find_pid_ns(1, ns));
> > + ei->pid = find_pid_ns(1, ns);
>
> I don't think that this is correct. IIUC, proc_delete_inode() calls put_pid() on
> ei->pid.

Yes,

> So either a special case is added in proc_delete_inode(), or we try to
> live with get_pid() here. I'm actually not sure that we can pretend that this
> pid remains valid if we don't get_pid() here.

But please see another change below,

> > +static void proc_mntput(struct work_struct *work)
> > {
> > + struct pid_namespace *ns = container_of(work, struct pid_namespace, proc_put);
> > +
> > + PROC_I(ns->proc_mnt->mnt_sb->s_root->d_inode)->pid = NULL;
> > mntput(ns->proc_mnt);
> > }

it clears ei->pid.

We are called from free_pid_ns() path, this ->pid must not have any reference.
Any get_pid() implies get_pid_ns().

What do you think?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 18 Jun 2010 17:30
On 06/18, Oleg Nesterov wrote:
>
> work->func(ns) is
> called when ns is already fixed.
^^^^^
(I meant freed)

We can move kmem_cache_free(ns) into work->func(), or we can optimize
the usage of schedule_work(), something like the patch below.

Once again, it is completely untested, I do not pretend I understand
this code today, and I am sure the patch is wrong. I only try to discuss
the idea to break the circular reference. In any case, I am not proud
of this patch ;)

Or we should do the more sophisticated change suggested by Pavel. But
I'd like to avoid the changes in do_wait/release_task, this doesn't
look right to me.

Oleg.

include/linux/pid_namespace.h | 1 +
kernel/pid_namespace.c | 35 ++++++++++++++++++++++++++++++++++-
fs/proc/base.c | 4 ----
fs/proc/root.c | 10 ++++++----
4 files changed, 41 insertions(+), 9 deletions(-)

--- 34-rc1/include/linux/pid_namespace.h~PID_NS 2010-06-18 17:48:56.000000000 +0200
+++ 34-rc1/include/linux/pid_namespace.h 2010-06-18 22:37:45.000000000 +0200
@@ -26,6 +26,7 @@ struct pid_namespace {
struct pid_namespace *parent;
#ifdef CONFIG_PROC_FS
struct vfsmount *proc_mnt;
+ struct list_head dead_node;
#endif
#ifdef CONFIG_BSD_PROCESS_ACCT
struct bsd_acct_struct *bacct;
--- 34-rc1/kernel/pid_namespace.c~PID_NS 2010-06-18 17:48:56.000000000 +0200
+++ 34-rc1/kernel/pid_namespace.c 2010-06-18 22:52:41.000000000 +0200
@@ -10,6 +10,7 @@

#include <linux/pid.h>
#include <linux/pid_namespace.h>
+#include <linux/proc_fs.h>
#include <linux/syscalls.h>
#include <linux/err.h>
#include <linux/acct.h>
@@ -105,15 +106,47 @@ out:
return ERR_PTR(-ENOMEM);
}

-static void destroy_pid_namespace(struct pid_namespace *ns)
+static LIST_HEAD(dead_list);
+static DEFINE_SPINLOCK(dead_lock);
+
+static void do_destroy_pid_namespace(struct pid_namespace *ns)
{
int i;

+ pid_ns_release_proc(ns);
+
for (i = 0; i < PIDMAP_ENTRIES; i++)
kfree(ns->pidmap[i].page);
kmem_cache_free(pid_ns_cachep, ns);
}

+static void dead_work_func(struct work_struct *unused)
+{
+ LIST_HEAD(list);
+ unsigned long flags;
+ struct pid_namespace *ns, *tmp;
+
+ spin_lock_irqsave(&dead_lock, flags);
+ list_splice_init(&dead_list, &list);
+ spin_unlock_irqrestore(&dead_lock, flags);
+
+ list_for_each_entry_safe(ns, tmp, &list, dead_node)
+ do_destroy_pid_namespace(ns);
+}
+
+static DECLARE_WORK(dead_work, dead_work_func);
+
+static void destroy_pid_namespace(struct pid_namespace *ns)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&dead_lock, flags);
+ list_add(&ns->dead_node, &dead_list);
+ spin_unlock_irqrestore(&dead_lock, flags);
+
+ schedule_work(&dead_work);
+}
+
struct pid_namespace *copy_pid_ns(unsigned long flags, struct pid_namespace *old_ns)
{
if (!(flags & CLONE_NEWPID))
--- 34-rc1/fs/proc/base.c~PID_NS 2010-06-18 17:48:56.000000000 +0200
+++ 34-rc1/fs/proc/base.c 2010-06-18 17:49:45.000000000 +0200
@@ -2720,10 +2720,6 @@ void proc_flush_task(struct task_struct
proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
tgid->numbers[i].nr);
}
-
- upid = &pid->numbers[pid->level];
- if (upid->nr == 1)
- pid_ns_release_proc(upid->ns);
}

static struct dentry *proc_pid_instantiate(struct inode *dir,
--- 34-rc1/fs/proc/root.c~PID_NS 2010-06-18 17:48:56.000000000 +0200
+++ 34-rc1/fs/proc/root.c 2010-06-18 22:54:03.000000000 +0200
@@ -31,7 +31,7 @@ static int proc_set_super(struct super_b
struct pid_namespace *ns;

ns = (struct pid_namespace *)data;
- sb->s_fs_info = get_pid_ns(ns);
+ sb->s_fs_info = ns;
return set_anon_super(sb, NULL);
}

@@ -74,7 +74,7 @@ static int proc_get_sb(struct file_syste
ei = PROC_I(sb->s_root->d_inode);
if (!ei->pid) {
rcu_read_lock();
- ei->pid = get_pid(find_pid_ns(1, ns));
+ ei->pid = find_pid_ns(1, ns);
rcu_read_unlock();
}

@@ -92,7 +92,6 @@ static void proc_kill_sb(struct super_bl

ns = (struct pid_namespace *)sb->s_fs_info;
kill_anon_super(sb);
- put_pid_ns(ns);
}

static struct file_system_type proc_fs_type = {
@@ -218,5 +217,8 @@ int pid_ns_prepare_proc(struct pid_names

void pid_ns_release_proc(struct pid_namespace *ns)
{
- mntput(ns->proc_mnt);
+ if (ns->proc_mnt) {
+ PROC_I(ns->proc_mnt->mnt_sb->s_root->d_inode)->pid = NULL;
+ mntput(ns->proc_mnt);
+ }
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: [RFC][PATCH 0/8] perf pmu interface
Next: $100,000 OFFER