Re: kern/59175: posix_spawn hang, hanging other process too

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Sun, 23 Mar 2025 13:39:31 -0700
> From: Chuck Silvers <chuq%chuq.com@localhost>
> 
> Here's an alternative patch that I prefer.  It allocates a new vmspace
> for the process at the time the new process is created, rather than sharing
> some other vmspace temporarily.  This eliminates any risk of anything
> bad happening due to temporary sharing, since there isn't any sharing.

Approach is fine by me.  Some minor cosmetic issues in the patch --
let's get this committed once they're addressed:

> --- src/sys/kern/kern_exec.c	15 Mar 2025 12:11:09 -0000	1.526
> +++ src/sys/kern/kern_exec.c	22 Mar 2025 22:35:06 -0000
> ...
> @@ -2641,7 +2635,14 @@ do_posix_spawn(struct lwp *l1, pid_t *pi
> ...
> +	p2->p_vmspace = uvmspace_alloc(exec_vm_minaddr(VM_MIN_ADDRESS),
> +				       VM_MAXUSER_ADDRESS, true);

Four-space indent, not alignment with `(', for continuation lines.

> --- src/sys/uvm/uvm_map.c	16 Aug 2024 11:28:01 -0000	1.426
> +++ src/sys/uvm/uvm_map.c	22 Mar 2025 22:36:29 -0000
> ...
> @@ -4296,13 +4274,12 @@ uvmspace_exec(struct lwp *l, vaddr_t sta
>  	cpu_vmspace_exec(l, start, end);
>  #endif
>  
> -	map = &ovm->vm_map;
>  	/*
> -	 * see if more than one process is using this vmspace...
> +	 * If the existing is not shared but has entries, clear them out now.
>  	 */

This was confusing to me at first.  I think it would help to say:

	/*
	 * If p is the only process using the vmspace, we can reuse it
	 * rather than allocate a new vmspace -- but we have to make
	 * sure it's empty first.
	 */

and maybe below, under the new `if (ovm->vm_refcnt == 1)', add
KASSERT(map->nentries == 0) to drive in the point that it is empty
now.

> -	if (ovm->vm_refcnt == 1
> -	    && topdown == ((ovm->vm_map.flags & VM_MAP_TOPDOWN) != 0)) {
> +	map = &ovm->vm_map;
> +	if (ovm->vm_refcnt == 1 && map->nentries != 0) {
>  
>  		/*
>  		 * if p is the only process using its vmspace then we can safely

This comment in full reads:

		/*
		 * if p is the only process using its vmspace then we can safely
		 * recycle that vmspace for the program that is being exec'd.
		 * But only if TOPDOWN matches the requested value for the new
		 * vm space!
		 */

Your change invalidates the second sentence of this comment, so it
should be removed.  (The first sentence is better pulled up out of the
conditional.)