Re: [PATCH v2 2/2] pid: Remove pidhash

[Oleg Nesterov <oleg@redhat.com>]

On 09/30, Gargi Sharma wrote:
>
> On Wed, Sep 27, 2017 at 9:58 PM, Oleg Nesterov <oleg@redhat.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.