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

["Taylor R Campbell via gnats" <gnats-admin%NetBSD.org@localhost>]

The following reply was made to PR kern/59175; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Chuck Silvers <chuq%chuq.com@localhost>
Cc: Thomas Klausner <wiz%NetBSD.org@localhost>, gnats-bugs%NetBSD.org@localhost,
	netbsd-bugs%NetBSD.org@localhost, martin%NetBSD.org@localhost, dholland%NetBSD.org@localhost
Subject: Re: kern/59175: posix_spawn hang, hanging other process too
Date: Sat, 12 Apr 2025 22:41:41 +0000

 > Date: Sun, 23 Mar 2025 13:39:31 -0700
 > From: Chuck Silvers <chuq%chuq.com@localhost>
 >=20
 > 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 shari=
 ng
 > 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 =3D 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
 > =20
 > -	map =3D &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 =3D=3D 1)', add
 KASSERT(map->nentries =3D=3D 0) to drive in the point that it is empty
 now.
 
 > -	if (ovm->vm_refcnt =3D=3D 1
 > -	    && topdown =3D=3D ((ovm->vm_map.flags & VM_MAP_TOPDOWN) !=3D 0)) {
 > +	map =3D &ovm->vm_map;
 > +	if (ovm->vm_refcnt =3D=3D 1 && map->nentries !=3D 0) {
 > =20
 >  		/*
 >  		 * 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.)