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]

Date:   Wed, 27 Sep 2017 09:09:54 -0400
From:   Rik van Riel <riel@...riel.com>
To:     Gargi Sharma <gs051095@...il.com>, linux-kernel@...r.kernel.org
Cc:     julia.lawall@...6.fr, akpm@...ux-foundation.org, mingo@...nel.org,
        pasha.tatashin@...cle.com, ktkhai@...tuozzo.com, oleg@...hat.com
Subject: Re: [PATCH v2 1/2] pid: Replace pid bitmap implementation with IDR
 API

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.

> @@ -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.

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

-- 
All Rights Reversed.
Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)