linux-kernel - Re: [PATCH v5 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] [day] [month] [year] [list]

Message-ID: <1507685048.21121.181.camel@surriel.com>
Date:   Tue, 10 Oct 2017 21:24:08 -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,
        ebiederm@...ssion.com, hch@...radead.org
Subject: Re: [PATCH v5 1/2] pid: Replace pid bitmap implementation with IDR
 API

On Tue, 2017-10-10 at 19:22 -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.
> 
> Signed-off-by: Gargi Sharma <gs051095@...il.com>

The patch looks mostly good, just a few style nitpicks remaining.

> @@ -308,8 +165,28 @@ struct pid *alloc_pid(struct pid_namespace *ns)
>  
>  	tmp = ns;
>  	pid->level = ns->level;
> +
>  	for (i = ns->level; i >= 0; i--) {
> -		nr = alloc_pidmap(tmp);
> +		int pid_min = 1;
> +
> +		idr_preload(GFP_KERNEL);
> +		spin_lock_irq(&pidmap_lock);
> +
> +		/*
> +		 * init really needs pid 1, but after reaching the
> maximum
> +		 * wrap back to RESERVED_PIDS
> +		 */
> +		if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS)
> +			pid_min = RESERVED_PIDS;
> +
> +		/* Store a null pointer so find_pid_ns does not find
> +		 * a partially initialized PID (see below).
> +		 */

This comment should start with an empty line, just like the one above:

	/*
	 * Store a null pointer so find_pid_ns does not find a
	 * partially initialized PID (see below).
	 */

> @@ -240,18 +230,15 @@ 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();
> +	nr = 2;
>  
> -		task = pid_task(find_vpid(nr), PIDTYPE_PID);
> +	rcu_read_lock();

I would place the rcu_read_lock() on the line above the
read_lock(&tasklist_lock) and the rcu_read_unlock() below
the read_unlock(&tasklist_lock).

Why? No real reason, except the rcu lock is "a larger lock"
than the read-write lock, and it seems to be how things
are done commonly...

I may be wrong here, and I hope somebody else corrects me
if I am wrong :)

> +	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);
>  	}
> +	rcu_read_unlock();
>  	read_unlock(&tasklist_lock);
>  
>  	/*

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