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