linux-kernel - Re: [PATCH 3/3] pid: only take pidmap

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: <aSNp-TDtv0ZoILJ3@redhat.com>
Date: Sun, 23 Nov 2025 21:09:55 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: brauner@...nel.org, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org, linux-mm@...ck.org
Subject: Re: [PATCH 3/3] pid: only take pidmap_lock once on alloc

On 11/23, Mateusz Guzik wrote:
>
> This reduces contention on the lock during parallel clone/exit.
>
> It remains the primary bottleneck in such a case.
>
> While here tidy up the code.

Not sure I can review... But FWIW this patch looks good to me after the
very quick glance. I'll try to actually read it tomorrow.

But please find a couple of minor "can't resist" nits below.

> +	for (tmp = ns, i = ns->level; i >= 0; i--) {
> +		int tid = set_tid[ns->level - i];
>
>  		if (tid) {
>  			nr = idr_alloc(&tmp->idr, NULL, tid,
> @@ -235,10 +261,8 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
>  			 * a partially initialized PID (see below).
>  			 */
>  			nr = idr_alloc_cyclic(&tmp->idr, NULL, pid_min,
> -					      pid_max, GFP_ATOMIC);
> +					      pid_max[ns->level - i], GFP_ATOMIC);
>  		}
> -		spin_unlock(&pidmap_lock);
> -		idr_preload_end();
>
>  		if (nr < 0) {
>  			retval = (nr == -ENOSPC) ? -EAGAIN : nr;

So. With or without this patch we have

	if (tid) {
		nr = idr_alloc(...);

		if (nr == -ENOSPC)
			nr = -EEXIST;
	} else {
		nr = idr_alloc_cyclic(...);
	}

	if (nr < 0) {
		retval = (nr == -ENOSPC) ? -EAGAIN : nr;
		goto out_free;
	}

and somehow this looks annoying to me... Perhaps it makes sense to make this
code more symmetric (and imo more readable) ?

	if (tid) {
		nr = idr_alloc(...);

		if (nr == -ENOSPC)
			nr = -EEXIST;
	} else {
		nr = idr_alloc_cyclic(...);

		if (nr == -ENOSPC)
			nr = -EAGAIN;
	}

	if (nr < 0)
		retval = nr;
		goto out_free;
	}

> -	idr_preload(GFP_KERNEL);
> -	spin_lock(&pidmap_lock);
> -	if (!(ns->pid_allocated & PIDNS_ADDING))
> -		goto out_unlock;
> +	if (unlikely(!(ns->pid_allocated & PIDNS_ADDING)))
> +		goto out_free;
>  	pidfs_add_pid(pid);
> -	for ( ; upid >= pid->numbers; --upid) {
> +	for (upid = pid->numbers + ns->level; upid >= pid->numbers; --upid) {
>  		/* Make the PID visible to find_pid_ns. */
>  		idr_replace(&upid->ns->idr, pid, upid->nr);
>  		upid->ns->pid_allocated++;

So.. unless I am totally confused the current code has another
idr_preload + idr_preload_end around pidfs_add_pid().

AFAICS, this makes no sense, and your patch removes it. But perhaps this
deserves a note in the changelog or even a separate patch?

And another stupid question... I don't understand fs/pidfs.c, but it looks
a bit strange to me that pidfs_add_pid() is called before the

	for (...)
		idr_replace(...);

loop. I don't see any problem, but to me it would look a bit better to do
pidfs_add_pid(pid) when this pid is fully initialized...

Oleg.