linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RESEND][PATCH v2 0/2] ns, procfs: pid conversion between ns and showing pidns hierarchy
@ 2014-09-22  9:53 Chen Hanxiao
  2014-09-22  9:53 ` [RESEND][PATCHv2 1/2] procfs: show hierarchy of pid namespace Chen Hanxiao
  2014-09-22  9:53 ` [RESEND][PATCHv3 2/2] /proc/PID/status: show all sets of pid according to ns Chen Hanxiao
  0 siblings, 2 replies; 5+ messages in thread
From: Chen Hanxiao @ 2014-09-22  9:53 UTC (permalink / raw)
  To: containers, linux-kernel
  Cc: Serge Hallyn, Eric W. Biederman, Oleg Nesterov, David Howells,
	Richard Weinberger, Pavel Emelyanov, Vasiliy Kulikov

This series will expose pid inside containers
via procfs.
Also show the hierarchy of pid namespcae.
Then we could know how pid looks inside a container
and their ns relationships.

v2: use a procfs text file instead of dirs
    to show the hierarchy of pid namespace


Chen Hanxiao (2):
  procfs: show hierarchy of pid namespace
  /proc/PID/status: show all sets of pid according to ns

 fs/proc/Kconfig           |   6 ++
 fs/proc/Makefile          |   1 +
 fs/proc/array.c           |  17 ++++
 fs/proc/pidns_hierarchy.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 219 insertions(+)
 create mode 100644 fs/proc/pidns_hierarchy.c

-- 
1.9.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [RESEND][PATCHv2 1/2] procfs: show hierarchy of pid namespace
  2014-09-22  9:53 [RESEND][PATCH v2 0/2] ns, procfs: pid conversion between ns and showing pidns hierarchy Chen Hanxiao
@ 2014-09-22  9:53 ` Chen Hanxiao
  2014-09-22 14:16   ` Mateusz Guzik
  2014-09-22  9:53 ` [RESEND][PATCHv3 2/2] /proc/PID/status: show all sets of pid according to ns Chen Hanxiao
  1 sibling, 1 reply; 5+ messages in thread
From: Chen Hanxiao @ 2014-09-22  9:53 UTC (permalink / raw)
  To: containers, linux-kernel
  Cc: Serge Hallyn, Eric W. Biederman, Oleg Nesterov, David Howells,
	Richard Weinberger, Pavel Emelyanov, Vasiliy Kulikov

This patch will show the hierarchy of pid namespace
by /proc/pidns_hierarchy like:

[root@localhost ~]#cat /proc/pidns_hierarchy
/proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
/proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
/proc/1550/ns/pid

It shows the pid hierarchy below:

        init_pid_ns (not showed in /proc/pidns_hierarchy)
              │
┌──────────────┐
ns1                          ns2
│                            │
/proc/1550/ns/pid    /proc/18060/ns/pid
                              │
                              │
                             ns3
                              │
                     /proc/18102/ns/pid
                              │
                     ┌──────────┒
                     ns4                   ns5
                     │                    │
                     /proc/1534/ns/pid   /proc/1600/ns/pid

Every pid printed in pidns_hierarchy
is the init pid of that pid ns level.

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
v2: use procfs text file instead of dirs under /proc

 fs/proc/Kconfig           |   6 ++
 fs/proc/Makefile          |   1 +
 fs/proc/pidns_hierarchy.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 202 insertions(+)
 create mode 100644 fs/proc/pidns_hierarchy.c

diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 2183fcf..e2e2292 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -71,3 +71,9 @@ config PROC_PAGE_MONITOR
 	  /proc/pid/smaps, /proc/pid/clear_refs, /proc/pid/pagemap,
 	  /proc/kpagecount, and /proc/kpageflags. Disabling these
           interfaces will reduce the size of the kernel by approximately 4kb.
+
+config PROC_PID_HIERARCHY
+	bool "Enable /proc/pidns_hierarchy support" if EXPERT
+	depends on PROC_FS
+	help
+	  Show pid namespace hierarchy information
diff --git a/fs/proc/Makefile b/fs/proc/Makefile
index 7151ea4..33e384b 100644
--- a/fs/proc/Makefile
+++ b/fs/proc/Makefile
@@ -30,3 +30,4 @@ proc-$(CONFIG_PROC_KCORE)	+= kcore.o
 proc-$(CONFIG_PROC_VMCORE)	+= vmcore.o
 proc-$(CONFIG_PRINTK)	+= kmsg.o
 proc-$(CONFIG_PROC_PAGE_MONITOR)	+= page.o
+proc-$(CONFIG_PROC_PID_HIERARCHY)	+= pidns_hierarchy.o
diff --git a/fs/proc/pidns_hierarchy.c b/fs/proc/pidns_hierarchy.c
new file mode 100644
index 0000000..2042d92
--- /dev/null
+++ b/fs/proc/pidns_hierarchy.c
@@ -0,0 +1,195 @@
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/proc_fs.h>
+#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/pid_namespace.h>
+#include <linux/seq_file.h>
+#include <linux/mutex.h>
+
+/*
+ *  /proc/pidns_hierarchy
+ *  show the hierarchy of pid namespace
+ */
+
+#define NS_HIERARCHY	"pidns_hierarchy"
+
+static LIST_HEAD(pidns_list);
+static LIST_HEAD(pidns_tree);
+static DEFINE_MUTEX(pidns_list_lock);
+
+/* list for host pid collection */
+struct pidns_list {
+	struct list_head list;
+	struct pid *pid;
+};
+
+static void free_pidns_list(struct list_head *head)
+{
+	struct pidns_list *tmp, *pos;
+
+	list_for_each_entry_safe(pos, tmp, head, list) {
+		list_del(&pos->list);
+		kfree(pos);
+	}
+}
+
+/*
+ * Only add init pid in different namespaces
+ */
+static int
+pidns_list_really_add(struct pid *pid, struct list_head *list_head)
+{
+	struct pidns_list *tmp, *pos;
+
+	if (!is_child_reaper(pid))
+		return 0;
+
+	list_for_each_entry_safe(pos, tmp, list_head, list)
+		if (ns_of_pid(pid) == ns_of_pid(pos->pid))
+			return 0;
+
+	return 1;
+}
+
+static int
+pidns_list_add(struct pid *pid, struct list_head *list_head)
+{
+	struct pidns_list *ent;
+
+	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
+	if (!ent)
+		return -ENOMEM;
+
+	ent->pid = pid;
+	if (pidns_list_really_add(pid, list_head))
+		list_add_tail(&ent->list, list_head);
+
+	return 0;
+}
+
+static void
+pidns_list_filter(void)
+{
+	struct pidns_list *tmp, *pos;
+	struct pidns_list *tmp_t, *pos_t;
+	struct pid_namespace *ns0, *ns1;
+	struct pid *pid0, *pid1;
+	int flag = 0;
+
+	/* screen pid with relationship
+	 * in pidns_list, we may add pids like
+	 * ns0   ns1   ns2
+	 * pid1->pid2->pid3
+	 * we should keep pid3 and screen pid1, pid2
+	 */
+	list_for_each_entry_safe(pos, tmp, &pidns_list, list) {
+		list_for_each_entry_safe(pos_t, tmp_t, &pidns_list, list) {
+			flag = 0;
+			pid0 = pos->pid;
+			pid1 = pos_t->pid;
+			ns0 = pid0->numbers[pid0->level].ns;
+			ns1 = pid1->numbers[pid1->level].ns;
+			if (pos->pid->level < pos_t->pid->level)
+				for (; ns1 != NULL; ns1 = ns1->parent)
+					if (ns0 == ns1) {
+						flag = 1;
+						break;
+					}
+			if (flag == 1)
+				break;
+		}
+
+		if (flag == 0)
+			pidns_list_add(pos->pid, &pidns_tree);
+	}
+
+	free_pidns_list(&pidns_list);
+}
+
+/* collect pids in pidns_list,
+ * then remove duplicated ones,
+ * add the rest to pidns_tree
+ */
+static void proc_pidns_list_refresh(void)
+{
+	struct pid *pid;
+	struct task_struct *p;
+
+	/* collect pid in differet ns */
+	rcu_read_lock();
+	for_each_process(p) {
+		pid = task_pid(p);
+		if (pid && (pid->level > 0))
+			pidns_list_add(pid, &pidns_list);
+	}
+	rcu_read_unlock();
+
+	/* screen duplicate pids */
+	pidns_list_filter();
+}
+
+static int nslist_proc_show(struct seq_file *m, void *v)
+{
+	struct pidns_list *tmp, *pos;
+	struct pid_namespace *ns, *curr_ns;
+	struct pid *pid;
+	char pid_buf[32];
+	int i, curr_level;
+
+	curr_ns = task_active_pid_ns(current);
+
+	mutex_lock(&pidns_list_lock);
+	proc_pidns_list_refresh();
+
+	/* print pid namespace hierarchy */
+	list_for_each_entry_safe(pos, tmp, &pidns_tree, list) {
+		pid = pos->pid;
+		curr_level = -1;
+		ns = pid->numbers[pid->level].ns;
+		/* Check whether pid has relationship with current ns */
+		for (; ns != NULL; ns = ns->parent)
+			if (ns == curr_ns)
+				curr_level = curr_ns->level;
+
+		if (curr_level == -1)
+			continue;
+
+		for (i = curr_level + 1; i <= pid->level; i++) {
+			ns = pid->numbers[i].ns;
+			/* PID 1 in specific pid ns */
+			snprintf(pid_buf, 32, "/proc/%u/ns/pid",
+				pid_vnr(find_pid_ns(1, ns)));
+			seq_printf(m, "%s ", pid_buf);
+		}
+
+		seq_putc(m, '\n');
+	}
+
+	free_pidns_list(&pidns_tree);
+	mutex_unlock(&pidns_list_lock);
+
+	return 0;
+}
+
+static int nslist_proc_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, nslist_proc_show, NULL);
+}
+
+static const struct file_operations proc_nspid_nslist_fops = {
+	.open		= nslist_proc_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init pidns_hierarchy_init(void)
+{
+	proc_create(NS_HIERARCHY, S_IWUGO,
+		NULL, &proc_nspid_nslist_fops);
+
+	return 0;
+}
+fs_initcall(pidns_hierarchy_init);
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [RESEND][PATCHv3 2/2] /proc/PID/status: show all sets of pid according to ns
  2014-09-22  9:53 [RESEND][PATCH v2 0/2] ns, procfs: pid conversion between ns and showing pidns hierarchy Chen Hanxiao
  2014-09-22  9:53 ` [RESEND][PATCHv2 1/2] procfs: show hierarchy of pid namespace Chen Hanxiao
@ 2014-09-22  9:53 ` Chen Hanxiao
  1 sibling, 0 replies; 5+ messages in thread
From: Chen Hanxiao @ 2014-09-22  9:53 UTC (permalink / raw)
  To: containers, linux-kernel
  Cc: Serge Hallyn, Eric W. Biederman, Oleg Nesterov, David Howells,
	Richard Weinberger, Pavel Emelyanov, Vasiliy Kulikov

If some issues occurred inside a container guest, host user
could not know which process is in trouble just by guest pid:
the users of container guest only knew the pid inside containers.
This will bring obstacle for trouble shooting.

This patch adds four fields: NStgid, NSpid, NSpgid and NSsid:
a) In init_pid_ns, nothing changed;

b) In one pidns, will tell the pid inside containers:
NStgid:	21776   5       1
NSpid:  21776   5       1
NSpgid:	21776   5       1
NSsid:  21729   1       0
** Process id is 21776 in level 0, 5 in level 1, 1 in level 2.

c) If pidns is nested, it depends on which pidns are you in.
NStgid: 5       1
NSpid:  5       1
NSpgid: 5       1
NSsid:  1       0
** View from level 1

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
v2: add two new fields: NStgid and NSpid.
    keep fields of Tgid and Pid unchanged for back compatibility.
v3: add another two fielsd: NSpgid and NSsid.

 fs/proc/array.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index cd3653e..c30875d 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -193,6 +193,23 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
 		from_kgid_munged(user_ns, cred->egid),
 		from_kgid_munged(user_ns, cred->sgid),
 		from_kgid_munged(user_ns, cred->fsgid));
+	seq_puts(m, "NStgid:");
+	for (g = ns->level; g <= pid->level; g++)
+		seq_printf(m, "\t%d ",
+			task_tgid_nr_ns(p, pid->numbers[g].ns));
+	seq_puts(m, "\nNSpid:");
+	for (g = ns->level; g <= pid->level; g++)
+		seq_printf(m, "\t%d ",
+			task_pid_nr_ns(p, pid->numbers[g].ns));
+	seq_puts(m, "\nNSpgid:");
+	for (g = ns->level; g <= pid->level; g++)
+		seq_printf(m, "\t%d ",
+			task_pgrp_nr_ns(p, pid->numbers[g].ns));
+	seq_puts(m, "\nNSsid:");
+	for (g = ns->level; g <= pid->level; g++)
+		seq_printf(m, "\t%d ",
+			task_session_nr_ns(p, pid->numbers[g].ns));
+	seq_putc(m, '\n');
 
 	task_lock(p);
 	if (p->files)
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RESEND][PATCHv2 1/2] procfs: show hierarchy of pid namespace
  2014-09-22  9:53 ` [RESEND][PATCHv2 1/2] procfs: show hierarchy of pid namespace Chen Hanxiao
@ 2014-09-22 14:16   ` Mateusz Guzik
  2014-09-24  3:53     ` Chen, Hanxiao
  0 siblings, 1 reply; 5+ messages in thread
From: Mateusz Guzik @ 2014-09-22 14:16 UTC (permalink / raw)
  To: Chen Hanxiao
  Cc: containers, linux-kernel, Serge Hallyn, Eric W. Biederman,
	Oleg Nesterov, David Howells, Richard Weinberger,
	Pavel Emelyanov, Vasiliy Kulikov

On Mon, Sep 22, 2014 at 05:53:33PM +0800, Chen Hanxiao wrote:
> This patch will show the hierarchy of pid namespace
> by /proc/pidns_hierarchy like:
> 
> [root@localhost ~]#cat /proc/pidns_hierarchy
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> /proc/1550/ns/pid
> 

I don't really know the area, just had a quick look and the patch does
not seem right.

> +/*
> + *  /proc/pidns_hierarchy
> + *  show the hierarchy of pid namespace
> + */
> +
> +#define NS_HIERARCHY	"pidns_hierarchy"
> +
> +static LIST_HEAD(pidns_list);
> +static LIST_HEAD(pidns_tree);
> +static DEFINE_MUTEX(pidns_list_lock);
> +
> +/* list for host pid collection */
> +struct pidns_list {
> +	struct list_head list;
> +	struct pid *pid;
> +};
> +
> +static void free_pidns_list(struct list_head *head)
> +{
> +	struct pidns_list *tmp, *pos;
> +
> +	list_for_each_entry_safe(pos, tmp, head, list) {
> +		list_del(&pos->list);
> +		kfree(pos);
> +	}
> +}
> +
> +/*
> + * Only add init pid in different namespaces
> + */
> +static int
> +pidns_list_really_add(struct pid *pid, struct list_head *list_head)
> +{
> +	struct pidns_list *tmp, *pos;
> +
> +	if (!is_child_reaper(pid))
> +		return 0;
> +
> +	list_for_each_entry_safe(pos, tmp, list_head, list)
> +		if (ns_of_pid(pid) == ns_of_pid(pos->pid))
> +			return 0;
> +
> +	return 1;
> +}
> +
> +static int
> +pidns_list_add(struct pid *pid, struct list_head *list_head)
> +{
> +	struct pidns_list *ent;
> +
> +	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> +	if (!ent)
> +		return -ENOMEM;
> +

A call from proc_pidns_list_refresh will enter with rcu read-locked.
Is it really ok to sleep in such case?

> +	ent->pid = pid;
> +	if (pidns_list_really_add(pid, list_head))
> +		list_add_tail(&ent->list, list_head);
> +

Does not this leak memory if pidns_list_really_add returns 0?

Also, you just add stuff to the list and don't ref it in any way, so for
instance in proc_pidns_list_refresh below...

> +static void
> +pidns_list_filter(void)
> +{
> +	struct pidns_list *tmp, *pos;
> +	struct pidns_list *tmp_t, *pos_t;
> +	struct pid_namespace *ns0, *ns1;
> +	struct pid *pid0, *pid1;
> +	int flag = 0;
> +
> +	/* screen pid with relationship
> +	 * in pidns_list, we may add pids like
> +	 * ns0   ns1   ns2
> +	 * pid1->pid2->pid3
> +	 * we should keep pid3 and screen pid1, pid2
> +	 */
> +	list_for_each_entry_safe(pos, tmp, &pidns_list, list) {
> +		list_for_each_entry_safe(pos_t, tmp_t, &pidns_list, list) {

At this point maybe it would be beneficial to have a list of children
namespaces in pid_namespace?

> +			flag = 0;
> +			pid0 = pos->pid;
> +			pid1 = pos_t->pid;
> +			ns0 = pid0->numbers[pid0->level].ns;
> +			ns1 = pid1->numbers[pid1->level].ns;
> +			if (pos->pid->level < pos_t->pid->level)
> +				for (; ns1 != NULL; ns1 = ns1->parent)
> +					if (ns0 == ns1) {
> +						flag = 1;
> +						break;
> +					}
> +			if (flag == 1)
> +				break;
> +		}
> +
> +		if (flag == 0)
> +			pidns_list_add(pos->pid, &pidns_tree);
> +	}
> +
> +	free_pidns_list(&pidns_list);
> +}
> +

> +/* collect pids in pidns_list,
> + * then remove duplicated ones,
> + * add the rest to pidns_tree
> + */
> +static void proc_pidns_list_refresh(void)
> +{
> +	struct pid *pid;
> +	struct task_struct *p;
> +
> +	/* collect pid in differet ns */
> +	rcu_read_lock();
> +	for_each_process(p) {
> +		pid = task_pid(p);
> +		if (pid && (pid->level > 0))
> +			pidns_list_add(pid, &pidns_list);
> +	}
> +	rcu_read_unlock();
> +
> +	/* screen duplicate pids */
> +	pidns_list_filter();

What makes it safe to traverse the list after rcu_read_unlock?

> +}
> +
> +static int nslist_proc_show(struct seq_file *m, void *v)
> +{
> +	struct pidns_list *tmp, *pos;
> +	struct pid_namespace *ns, *curr_ns;
> +	struct pid *pid;
> +	char pid_buf[32];
> +	int i, curr_level;
> +
> +	curr_ns = task_active_pid_ns(current);
> +
> +	mutex_lock(&pidns_list_lock);
> +	proc_pidns_list_refresh();
> +

So this reiterates everything for each open? Maybe a generation counter
could be employed here to avoid unnecessary work.

> +	/* print pid namespace hierarchy */
> +	list_for_each_entry_safe(pos, tmp, &pidns_tree, list) {
> +		pid = pos->pid;
> +		curr_level = -1;
> +		ns = pid->numbers[pid->level].ns;
> +		/* Check whether pid has relationship with current ns */
> +		for (; ns != NULL; ns = ns->parent)
> +			if (ns == curr_ns)
> +				curr_level = curr_ns->level;
> +
> +		if (curr_level == -1)
> +			continue;
> +
> +		for (i = curr_level + 1; i <= pid->level; i++) {
> +			ns = pid->numbers[i].ns;
> +			/* PID 1 in specific pid ns */
> +			snprintf(pid_buf, 32, "/proc/%u/ns/pid",
> +				pid_vnr(find_pid_ns(1, ns)));
> +			seq_printf(m, "%s ", pid_buf);
> +		}
> +
> +		seq_putc(m, '\n');
> +	}
> +
> +	free_pidns_list(&pidns_tree);
> +	mutex_unlock(&pidns_list_lock);
> +
> +	return 0;
> +}
> +

-- 
Mateusz Guzik

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [RESEND][PATCHv2 1/2] procfs: show hierarchy of pid namespace
  2014-09-22 14:16   ` Mateusz Guzik
@ 2014-09-24  3:53     ` Chen, Hanxiao
  0 siblings, 0 replies; 5+ messages in thread
From: Chen, Hanxiao @ 2014-09-24  3:53 UTC (permalink / raw)
  To: Mateusz Guzik
  Cc: containers, linux-kernel, Serge Hallyn, Eric W. Biederman,
	Oleg Nesterov, David Howells, Richard Weinberger,
	Pavel Emelyanov, Vasiliy Kulikov

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6179 bytes --]

Hi,

> -----Original Message-----
> From: Mateusz Guzik [mailto:mguzik@redhat.com]
> On Mon, Sep 22, 2014 at 05:53:33PM +0800, Chen Hanxiao wrote:
> > This patch will show the hierarchy of pid namespace
> > by /proc/pidns_hierarchy like:
> >
> > [root@localhost ~]#cat /proc/pidns_hierarchy
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> > /proc/1550/ns/pid
> >
> 
> I don't really know the area, just had a quick look and the patch does
> not seem right.
> 
> > +/*
> > + *  /proc/pidns_hierarchy
> > + *  show the hierarchy of pid namespace
> > + */
> > +
> > +#define NS_HIERARCHY	"pidns_hierarchy"
> > +
> > +static LIST_HEAD(pidns_list);
> > +static LIST_HEAD(pidns_tree);
> > +static DEFINE_MUTEX(pidns_list_lock);
> > +
> > +/* list for host pid collection */
> > +struct pidns_list {
> > +	struct list_head list;
> > +	struct pid *pid;
> > +};
> > +
> > +static void free_pidns_list(struct list_head *head)
> > +{
> > +	struct pidns_list *tmp, *pos;
> > +
> > +	list_for_each_entry_safe(pos, tmp, head, list) {
> > +		list_del(&pos->list);
> > +		kfree(pos);
> > +	}
> > +}
> > +
> > +/*
> > + * Only add init pid in different namespaces
> > + */
> > +static int
> > +pidns_list_really_add(struct pid *pid, struct list_head *list_head)
> > +{
> > +	struct pidns_list *tmp, *pos;
> > +
> > +	if (!is_child_reaper(pid))
> > +		return 0;
> > +
> > +	list_for_each_entry_safe(pos, tmp, list_head, list)
> > +		if (ns_of_pid(pid) == ns_of_pid(pos->pid))
> > +			return 0;
> > +
> > +	return 1;
> > +}
> > +
> > +static int
> > +pidns_list_add(struct pid *pid, struct list_head *list_head)
> > +{
> > +	struct pidns_list *ent;
> > +
> > +	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> > +	if (!ent)
> > +		return -ENOMEM;
> > +
> 
> A call from proc_pidns_list_refresh will enter with rcu read-locked.
> Is it really ok to sleep in such case?
> 

The flag should be GFP_ATOMIC.

> > +	ent->pid = pid;
> > +	if (pidns_list_really_add(pid, list_head))
> > +		list_add_tail(&ent->list, list_head);
> > +
> 
> Does not this leak memory if pidns_list_really_add returns 0?
> 
Will fix.

> Also, you just add stuff to the list and don't ref it in any way, so for
> instance in proc_pidns_list_refresh below...

I add stuffs to list called 'pidns_list',
then screen them to form a new list called 'pidns_tree'.

> 
> > +static void
> > +pidns_list_filter(void)
> > +{
> > +	struct pidns_list *tmp, *pos;
> > +	struct pidns_list *tmp_t, *pos_t;
> > +	struct pid_namespace *ns0, *ns1;
> > +	struct pid *pid0, *pid1;
> > +	int flag = 0;
> > +
> > +	/* screen pid with relationship
> > +	 * in pidns_list, we may add pids like
> > +	 * ns0   ns1   ns2
> > +	 * pid1->pid2->pid3
> > +	 * we should keep pid3 and screen pid1, pid2
> > +	 */
> > +	list_for_each_entry_safe(pos, tmp, &pidns_list, list) {
> > +		list_for_each_entry_safe(pos_t, tmp_t, &pidns_list, list) {
> 
> At this point maybe it would be beneficial to have a list of children
> namespaces in pid_namespace?

pid_namespace don't have such member to identify who is its children.
It only knew who is its parent.
So I had to use a double loop to check
whether one pid be another pid's children.

> 
> > +			flag = 0;
> > +			pid0 = pos->pid;
> > +			pid1 = pos_t->pid;
> > +			ns0 = pid0->numbers[pid0->level].ns;
> > +			ns1 = pid1->numbers[pid1->level].ns;
> > +			if (pos->pid->level < pos_t->pid->level)
> > +				for (; ns1 != NULL; ns1 = ns1->parent)
> > +					if (ns0 == ns1) {
> > +						flag = 1;
> > +						break;
> > +					}
> > +			if (flag == 1)
> > +				break;
> > +		}
> > +
> > +		if (flag == 0)
> > +			pidns_list_add(pos->pid, &pidns_tree);
> > +	}
> > +
> > +	free_pidns_list(&pidns_list);
> > +}
> > +
> 
> > +/* collect pids in pidns_list,
> > + * then remove duplicated ones,
> > + * add the rest to pidns_tree
> > + */
> > +static void proc_pidns_list_refresh(void)
> > +{
> > +	struct pid *pid;
> > +	struct task_struct *p;
> > +
> > +	/* collect pid in differet ns */
> > +	rcu_read_lock();
> > +	for_each_process(p) {
> > +		pid = task_pid(p);
> > +		if (pid && (pid->level > 0))
> > +			pidns_list_add(pid, &pidns_list);
> > +	}
> > +	rcu_read_unlock();
> > +
> > +	/* screen duplicate pids */
> > +	pidns_list_filter();
> 
> What makes it safe to traverse the list after rcu_read_unlock?

pidns_list_filter should be moved before rcu_read_unlock.

> 
> > +}
> > +
> > +static int nslist_proc_show(struct seq_file *m, void *v)
> > +{
> > +	struct pidns_list *tmp, *pos;
> > +	struct pid_namespace *ns, *curr_ns;
> > +	struct pid *pid;
> > +	char pid_buf[32];
> > +	int i, curr_level;
> > +
> > +	curr_ns = task_active_pid_ns(current);
> > +
> > +	mutex_lock(&pidns_list_lock);
> > +	proc_pidns_list_refresh();
> > +
> 
> So this reiterates everything for each open? Maybe a generation counter
> could be employed here to avoid unnecessary work.

Yes.
We don't trace the life circle of pidns.
So an easy way to do this is to reiterate everything for each open.
After show hierarchy of pidns, all lists of pid collections
will be freed.

Thanks for your review.
New version will come soon.

- Chen

> 
> > +	/* print pid namespace hierarchy */
> > +	list_for_each_entry_safe(pos, tmp, &pidns_tree, list) {
> > +		pid = pos->pid;
> > +		curr_level = -1;
> > +		ns = pid->numbers[pid->level].ns;
> > +		/* Check whether pid has relationship with current ns */
> > +		for (; ns != NULL; ns = ns->parent)
> > +			if (ns == curr_ns)
> > +				curr_level = curr_ns->level;
> > +
> > +		if (curr_level == -1)
> > +			continue;
> > +
> > +		for (i = curr_level + 1; i <= pid->level; i++) {
> > +			ns = pid->numbers[i].ns;
> > +			/* PID 1 in specific pid ns */
> > +			snprintf(pid_buf, 32, "/proc/%u/ns/pid",
> > +				pid_vnr(find_pid_ns(1, ns)));
> > +			seq_printf(m, "%s ", pid_buf);
> > +		}
> > +
> > +		seq_putc(m, '\n');
> > +	}
> > +
> > +	free_pidns_list(&pidns_tree);
> > +	mutex_unlock(&pidns_list_lock);
> > +
> > +	return 0;
> > +}
> > +
> 
> --
> Mateusz Guzik
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2014-09-24  3:53 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-22  9:53 [RESEND][PATCH v2 0/2] ns, procfs: pid conversion between ns and showing pidns hierarchy Chen Hanxiao
2014-09-22  9:53 ` [RESEND][PATCHv2 1/2] procfs: show hierarchy of pid namespace Chen Hanxiao
2014-09-22 14:16   ` Mateusz Guzik
2014-09-24  3:53     ` Chen, Hanxiao
2014-09-22  9:53 ` [RESEND][PATCHv3 2/2] /proc/PID/status: show all sets of pid according to ns Chen Hanxiao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).