linux-kernel - Re: [PATCH v2 1/2] pid: Replace pid bitmap implementation with IDR API

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: <CAOCi2DF=rBqLg9BjzXO1brErgV_vixrGiD9eOiLRauUoLhJJbg@mail.gmail.com>
Date:   Wed, 27 Sep 2017 20:35:07 +0530
From:   Gargi Sharma <gs051095@...il.com>
To:     Rik van Riel <riel@...riel.com>
Cc:     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,
        Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH v2 1/2] pid: Replace pid bitmap implementation with IDR API

On Wed, Sep 27, 2017 at 6:39 PM, Rik van Riel <riel@...riel.com> wrote:
> On Wed, 2017-09-27 at 01:06 -0400, Gargi Sharma wrote:
>> This patch replaces the current bitmap implemetation for
>> Process ID allocation. Functions that are no longer required,
>> for example, free_pidmap(), alloc_pidmap(), etc. are removed.
>> The rest of the functions are modified to use the IDR API.
>> The change was made to make the PID allocation less complex by
>> replacing custom code with calls to generic API.
>
> I like where this is going. Just a few last comments from me :)
>
>> --- a/arch/powerpc/platforms/cell/spufs/sched.c
>> +++ b/arch/powerpc/platforms/cell/spufs/sched.c
>> @@ -1093,7 +1093,7 @@ static int show_spu_loadavg(struct seq_file *s,
>> void *private)
>>               LOAD_INT(c), LOAD_FRAC(c),
>>               count_active_contexts(),
>>               atomic_read(&nr_spu_contexts),
>> -             task_active_pid_ns(current)->last_pid);
>> +             task_active_pid_ns(current)->idr.idr_next-1);
>>       return 0;
>>  }
>
> Everywhere you use idr.idr_next or idr->idr_next directly,
> you could use idr_get_cursor(idr) instead, exposing less
> of the IDR internals to the rest of the code.
Yes, will change this in the next version.
>
>> @@ -240,17 +230,18 @@ void zap_pid_ns_processes(struct pid_namespace
>> *pid_ns)
>>        *
>>        */
>>       read_lock(&tasklist_lock);
>> -     nr = next_pidmap(pid_ns, 1);
>> -     while (nr > 0) {
>> -             rcu_read_lock();
>> +     pid = idr_get_next(&pid_ns->idr, &nr);
>> +     while (pid) {
>>
>> -             task = pid_task(find_vpid(nr), PIDTYPE_PID);
>> -             if (task && !__fatal_signal_pending(task))
>> -                     send_sig_info(SIGKILL, SEND_SIG_FORCED,
>> task);
>> +             rcu_read_lock();
>>
>> +             idr_for_each_entry_continue(&pid_ns->idr, pid, nr) {
>> +                     task = pid_task(pid, PIDTYPE_PID);
>> +                     if (task && !__fatal_signal_pending(task))
>> +                             send_sig_info(SIGKILL,
>> SEND_SIG_FORCED, task);
>> +             }
>>               rcu_read_unlock();
>>
>> -             nr = next_pidmap(pid_ns, nr);
>>       }
>>       read_unlock(&tasklist_lock);
>>
>
> I believe we should be fine with just the idr_for_each_entry_continue()
> surrounding the loop, and not need the while (pid) around that.
>
> That should still iterate over all the pids in the namespace, and
> simplify the code even more.
Yes, one loop suffices. I will fix this in the next version.

>
> You have done a great job understanding some complicated code, and
> simplifying it during your Outreachy internship.
Thanks!

Gargi
>
> --
> All Rights Reversed.