linux-kernel - RE: [PATCHv4] procfs: show hierarchy of pid 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]

Message-ID: <5871495633F38949900D2BF2DC04883E5E1649@G08CNEXMBPEKD02.g08.fujitsu.local>
Date:	Thu, 9 Oct 2014 10:02:37 +0000
From:	"Chen, Hanxiao" <chenhanxiao@...fujitsu.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	"containers@...ts.linux-foundation.org" 
	<containers@...ts.linux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Serge Hallyn <serge.hallyn@...ntu.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	David Howells <dhowells@...hat.com>,
	Richard Weinberger <richard.weinberger@...il.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	Vasiliy Kulikov <segooon@...il.com>,
	Mateusz Guzik <mguzik@...hat.com>
Subject: RE: [PATCHv4] procfs: show hierarchy of pid namespace



> -----Original Message-----
> From: Oleg Nesterov [mailto:oleg@...hat.com]
> Sent: Wednesday, October 08, 2014 11:13 PM
> To: Chen, Hanxiao/陈 晗霄
> Cc: containers@...ts.linux-foundation.org; linux-kernel@...r.kernel.org; Serge
> Hallyn; Eric W. Biederman; David Howells; Richard Weinberger; Pavel Emelyanov;
> Vasiliy Kulikov; Mateusz Guzik
> Subject: Re: [PATCHv4] procfs: show hierarchy of pid namespace
> 
> Sorry if this was already discussed, I have to admit that I ignored
> the previous discussion ;) And it is possible I misread this patch
> completely.
> 
> On 10/08, Chen Hanxiao wrote:
> >
> > This patch will show the hierarchy of pid namespace
> > by /proc/pidns_hierarchy like:
> >
> > [root@...alhost ~]#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
> 
> Well, personally I too think that the filenames look a bit strange,
> can't it just print the numbers?

Yes, let's print PID numbers only.
> 
> And, iiuc what this patch does... perhaps in this case we should
> simply add "struct list_head children" into struct pid_namespace?
> In this case the patch will be really simple. I dunno.
> 

If we had a children list in pid_namespace,
all we had to do is a iteration from pid 1 of current ns.
That would be nice.

> > +pidns_list_add(struct pid *pid, struct list_head *list_head,
> > +		struct pid_namespace *curr_ns)
> > +{
> > +	struct pidns_list *ent;
> > +	struct pid_namespace *ns;
> > +
> > +	if (is_child_reaper(pid)) {
> > +		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> 
> GFP_KERNEL under rcu_read_lock(). This is not safe without
> CONFIG_PREEMPT_RCU.

It should be GFP_ATOMIC, Matesuz have already pointed out
and I'v changed it in v3.
Sorry for that mistake.

> 
> > +		if (!ent)
> > +			return -ENOMEM;
> > +
> > +		ent->pid = pid;
> > +		ns = pid->numbers[pid->level].ns;
> > +		if (curr_ns) {
> > +			/* add pids who is the child of curr_ns */
> > +			for (; ns != NULL; ns = ns->parent)
> > +				if (ns == curr_ns)
> > +					list_add_tail(&ent->list, list_head);
> 
> afaics, it doesn't make sense to continue after list_add() ?

Oops, we need a break here.

> 
> > +static int proc_pidns_list_refresh(struct pid_namespace *curr_ns)
> > +{
> > +	struct pid *pid;
> > +	struct task_struct *p;
> > +	int rc;
> > +
> > +	/* collect pid in differet ns */
> > +	for_each_process(p) {
> 
> Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid()
> makes more sense?

Only tasks from our ns is valid.
But how could find_ge_pid() do that?

nr = 1;
while (nr < PID_MAX_LIMIT) {
	find_ge_pid(nr, curr_ns);
	list_add();
	nr++;
}
Perhaps that's not a good way.

> 
> > +		pid = task_pid(p);
> 
> Well, in theory you need barrier() here. Or perhaps we should add
> ACCESS_ONCE() into task_pid()...

You mean modify task_pid as:
return ACCESS_ONCE(task->pids[PIDTYPE_PID].pid;);

> 
> And imho it would be better to declare pidns_list/pidns_tree locally
> in nslist_proc_show() and pass them to the callees.

That's a good idea.
Will changed in the next version.

Thanks,
- Chen