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] [day] [month] [year] [list]

Date:   Mon, 2 Oct 2017 21:57:06 +0530
From:   Gargi Sharma <gs051095@...il.com>
To:     Rik van Riel <riel@...riel.com>
Cc:     Oleg Nesterov <oleg@...hat.com>, linux-kernel@...r.kernel.org,
        Julia Lawall <julia.lawall@...6.fr>, akpm@...ux-foundation.org,
        mingo@...nel.org, pasha.tatashin@...cle.com, ktkhai@...tuozzo.com
Subject: Re: [PATCH v2 2/2] pid: Remove pidhash

On Mon, Oct 2, 2017 at 8:52 PM, Rik van Riel <riel@...riel.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.