linux-kernel - Re: [PATCH v2 2/2] pid: Remove pidhash

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: <20171002150346.GA9481@redhat.com>
Date:   Mon, 2 Oct 2017 17:03:46 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Gargi Sharma <gs051095@...il.com>
Cc:     linux-kernel@...r.kernel.org, Rik van Riel <riel@...riel.com>,
        Julia Lawall <julia.lawall@...6.fr>, akpm@...ux-foundation.org,
        mingo@...nel.org, pasha.tatashin@...cle.com, ktkhai@...tuozzo.com,
        Eric Biederman <ebiederm@...ssion.com>
Subject: Re: [PATCH v2 2/2] pid: Remove pidhash

On 09/30, Gargi Sharma wrote:
>
> On Wed, Sep 27, 2017 at 9:58 PM, Oleg Nesterov <oleg@...hat.com> wrote:
> > If I was not clear...
> >
> > in short, after this patch the very first idr_alloc_cyclic() is already
> > wrong. Because, once again, the new not-fully-initialized pid can be found
> > by find_pid_ns().
>
>  If the PIDNS_ADDING check fails, I jump to the flag that performs
>  this
>  while (++i <= ns->level)
>                  idr_remove(&ns->idr, (pid->numbers + i)->nr);
>  So when find_pid_ns() is called, it will not find this pid.

You misunderstood.

OK, to simplify lets forget about namespaces, locking, everything. So after
this patch alloc_pid() roughly does:

	pid = kmem_cache_alloc();

	nr = idr_alloc_cyclic(idr, pid); // lets suppose it returns 1234

	/* WINDOW */

	for (type = 0; type < PIDTYPE_MAX; ++type)
		INIT_HLIST_HEAD(&pid->tasks[type]);

now suppose that in that WINDOW above another CPU does, just for example,
sys_tkill(1234, SIG) which implies find_task_by_vpid(1234) which does
pid_task(find_pid_ns(1234)).

find_pid_ns() returns the non-initialized pid above, because with this
patch it is idr_find() and this pid was already added by idr_alloc_cyclic().

Then pid_task(pid) returns garbage because pid->tasks[] was not initialised yet.

And of course we have the same problems with pid->count/numbers/etc.

See?

> > perhaps you should chane the previous patch to do
> > idr_alloc_cyclic(ptr = NULL) and use idr_replace() in this patch after
> > the PIDNS_HASH_ADDING check.
>
> I'm not sure if I understand this. Do we want to do this to make sure
> the pid namespace is
> initialized before the first process enters into
> the namespace? If yes,

No,

> how does idr_alloc_cyclic(ptr = NULL) help?

In this case find_pid_ns/idr_find will return NULL until we do
idr_replace(idr, pid) when this pid is fully initialized.

Oleg.