On Mon, Oct 2, 2017 at 8:52 PM, Rik van Riel <r...@surriel.com> wrote:
> On Mon, 2017-10-02 at 17:21 +0200, Oleg Nesterov wrote:
>> Hi Rik,
>>
>> On 10/02, Rik van Riel wrote:
>> >
>> > Gargi and I are looking at that code, and trying to figure out
>> > exactly what needs to be done to make all of this correct.
>>
>> see another email I sent to Gargi a minute ago,
>>
>> > 2) With pid_ns_prepare_proc out of the way, we can put all the code
>> > from below where the call to pid_ns_prepare_proc is now (except
>> > error handing) into the main loop of pid allocation, so we can
>> > do all that stuff under the pidmap_lock:
>> >
>> > for (i = ns->level; i >= 0; i--) {
>> > ...
>> > idr_alloc_cyclic(...)
>> > get_pid_ns(ns);
>> > atomic_set(&pid->count, 1);
>> > for (...)
>> > INIT_HLIST_HEAD(...)
>> > ns->nr_allocated++;
>> > ...
>> > }
>>
>> I do not see how this can fix the problem with not-fully-initialized
>> pid returned by find_pid_ns().
>>
>> As for PIDNS_ADDING/PIDNS_HASH_ADDING, _perhaps_ we can cleanup this
>> logic
>> a bit and do the check earlier, but imo this needs another/separate
>> change.
>>
>> I'd suggest to keep the current logic and the order of initialization
>> and
>> just do
>>
>> for (i = ns->level; i >= 0; i--) {
>> ...
>>
>> // do not expose the new pid to find_pid_ns() until it
>> // is fully initialized
>> nr = idr_alloc_cyclic(&tmp->idr, /*pid*/ NULL, ...);
>> ...
>> }
>>
>> ...
>>
>> spin_lock_irq(&pidmap_lock);
>> if (!(ns->nr_hashed & PIDNS_HASH_ADDING))
>> goto out_unlock;
>> for ( ; upid >= pid->numbers; --upid) {
>> - hlist_add_head_rcu(&upid->pid_chain,
>> - &pid_hash[pid_hashfn(upid->nr, upid-
>> >ns)]);
>> + // finally make it visible to find_pid_ns()
>> + idr_replace(upid->ns-idr, pid, upid->nr);
>> upid->ns->nr_hashed++;
>> }
>> spin_unlock_irq(&pidmap_lock);
>>
>> Or I missed something?
Thanks for detailed explanation Oleg!
>
> You are right, that would both fix the problem, and keep the error
> paths relatively simple.
>
> Gargi, what do you think?
I understand better now. Thanks!
Best,
Gargi
>
> --
> All Rights Reversed.
