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: <20141009213414.GA10705@redhat.com>
Date:	Thu, 9 Oct 2014 23:34:14 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Chen, Hanxiao" <chenhanxiao@...fujitsu.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

On 10/09, Chen, Hanxiao wrote:
>
> > From: Oleg Nesterov [mailto:oleg@...hat.com]
> >
> > 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++;
> }

something like this, except list_add() should obviously depend on
is_child_reaper() check.

This can be more optimal in sub-namespaces, you do not need to abuse
the global process list.

And if you change this code to use get_pid/put_pid, then you do not
need to hold rcu_read_lock() throughout, you only need it around
find_ge_pid + get_pid.

At the same time, for_each_process() in the global namespace can be
faster if there are a lot of sub-threads.

> Perhaps that's not a good way.

OK, I won't insist.

although it would be nice to know why do you think this is bad.

> > > +		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;);

Yes. But not now an not in this patch of course. I'd suggest to add
barrier() just in case.


> > 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.

Good. And I forgot to mention, in this case you do not need pidns_list_lock
at all afaics.

Oleg.

--
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/