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

["Chuck Silvers via gnats" <gnats-admin%NetBSD.org@localhost>]

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

From: Chuck Silvers <chuq%chuq.com@localhost>
To: Taylor R Campbell <riastradh%NetBSD.org@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: Sun, 23 Mar 2025 13:39:31 -0700

 --Eqn6h8LjFSAmGTae
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 On Fri, Mar 14, 2025 at 08:05:59PM +0000, Taylor R Campbell wrote:
 > > Date: Fri, 14 Mar 2025 15:14:37 +0000
 > > From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 > > 
 > > > Date: Fri, 14 Mar 2025 14:28:38 +0000
 > > > From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 > > > 
 > > > 1. do_posix_spawn in the parent p1 creates a process p2 with
 > > >    proc_alloc, which has:
 > > >    - p2->p_stat = SIDL (can't be looked up with proc_find)
 > > >    - p2->p_vmspace = &vmspace0 (initial vmspace, kernel's) or NULL
 > > >    - p2->p_psstrp = garbage from recycled struct proc or NULL
 > > > 
 > > >    In particular, if p2 is freshly allocated, it will have null
 > > >    p_vmspace and p_psstrp; if it was recycled, p_vmspace will have
 > > >    been set to proc0.p_vmspace=&vmspace0 shortly before recycling
 > > >    in uvm_proc_exit:
 > > >    https://nxr.netbsd.org/xref/src/sys/uvm/uvm_glue.c?r=1.182#414
 > > 
 > > Tiny correction: do_posix_spawn always initializes p2->p_vmspace to
 > > proc0.p_vmspace, i.e., &vmspace0, so NULL is not possible here, as of
 > > kern_exec.c rev. 1.348.
 > 
 > One more correction: p_psstrp is always inherited from the parent
 > because it lies between p_startcopy and p_endcopy, so it's never null
 > when copyin_psstrings tries to get at it.
 
 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.
 
 -Chuck
 
 --Eqn6h8LjFSAmGTae
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename="diff.spawn-vmspace.2"
 
 Index: src/sys/kern/kern_exec.c
 ===================================================================
 RCS file: /home/chs/netbsd/cvs/src/sys/kern/kern_exec.c,v
 retrieving revision 1.526
 diff -u -p -r1.526 kern_exec.c
 --- 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
 @@ -1245,14 +1245,8 @@ execve_runproc(struct lwp *l, struct exe
  	 * until we have awoken the parent below, or it will defeat
  	 * lazy pmap switching (on x86).
  	 */
 -	if (is_spawn)
 -		uvmspace_spawn(l, epp->ep_vm_minaddr,
 -		    epp->ep_vm_maxaddr,
 -		    epp->ep_flags & EXEC_TOPDOWN_VM);
 -	else
 -		uvmspace_exec(l, epp->ep_vm_minaddr,
 -		    epp->ep_vm_maxaddr,
 -		    epp->ep_flags & EXEC_TOPDOWN_VM);
 +	uvmspace_exec(l, epp->ep_vm_minaddr, epp->ep_vm_maxaddr,
 +	    epp->ep_flags & EXEC_TOPDOWN_VM);
  	vm = p->p_vmspace;
  
  	vm->vm_taddr = (void *)epp->ep_taddr;
 @@ -2641,7 +2635,14 @@ do_posix_spawn(struct lwp *l1, pid_t *pi
  	    (unsigned) ((char *)&p2->p_endzero - (char *)&p2->p_startzero));
  	memcpy(&p2->p_startcopy, &p1->p_startcopy,
  	    (unsigned) ((char *)&p2->p_endcopy - (char *)&p2->p_startcopy));
 -	p2->p_vmspace = proc0.p_vmspace;
 +
 +	/*
 +	 * Allocate an empty user vmspace for the new process now.
 +	 * The min/max and topdown parameters given here are just placeholders,
 +	 * the right values will be assigned in uvmspace_exec().
 +	 */
 +	p2->p_vmspace = uvmspace_alloc(exec_vm_minaddr(VM_MIN_ADDRESS),
 +				       VM_MAXUSER_ADDRESS, true);
  
  	TAILQ_INIT(&p2->p_sigpend.sp_info);
  
 Index: src/sys/uvm/uvm_extern.h
 ===================================================================
 RCS file: /home/chs/netbsd/cvs/src/sys/uvm/uvm_extern.h,v
 retrieving revision 1.233
 diff -u -p -r1.233 uvm_extern.h
 --- src/sys/uvm/uvm_extern.h	26 Feb 2023 07:27:14 -0000	1.233
 +++ src/sys/uvm/uvm_extern.h	18 Mar 2025 14:30:19 -0000
 @@ -731,7 +731,6 @@ struct vmspace		*uvmspace_alloc(vaddr_t,
  void			uvmspace_init(struct vmspace *, struct pmap *,
  			    vaddr_t, vaddr_t, bool);
  void			uvmspace_exec(struct lwp *, vaddr_t, vaddr_t, bool);
 -void			uvmspace_spawn(struct lwp *, vaddr_t, vaddr_t, bool);
  struct vmspace		*uvmspace_fork(struct vmspace *);
  void			uvmspace_addref(struct vmspace *);
  void			uvmspace_free(struct vmspace *);
 Index: src/sys/uvm/uvm_map.c
 ===================================================================
 RCS file: /home/chs/netbsd/cvs/src/sys/uvm/uvm_map.c,v
 retrieving revision 1.426
 diff -u -p -r1.426 uvm_map.c
 --- 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
 @@ -4257,28 +4257,6 @@ uvmspace_unshare(struct lwp *l)
  
  #endif
  
 -
 -/*
 - * uvmspace_spawn: a new process has been spawned and needs a vmspace
 - */
 -
 -void
 -uvmspace_spawn(struct lwp *l, vaddr_t start, vaddr_t end, bool topdown)
 -{
 -	struct proc *p = l->l_proc;
 -	struct vmspace *nvm;
 -
 -#ifdef __HAVE_CPU_VMSPACE_EXEC
 -	cpu_vmspace_exec(l, start, end);
 -#endif
 -
 -	nvm = uvmspace_alloc(start, end, topdown);
 -	kpreempt_disable();
 -	p->p_vmspace = nvm;
 -	pmap_activate(l);
 -	kpreempt_enable();
 -}
 -
  /*
   * uvmspace_exec: the process wants to exec a new program
   */
 @@ -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.
  	 */
  
 -	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
 @@ -4344,13 +4321,22 @@ uvmspace_exec(struct lwp *l, vaddr_t sta
  		pmap_update(map->pmap);
  		KASSERT(map->header.prev == &map->header);
  		KASSERT(map->nentries == 0);
 +	}
 +
 +	if (ovm->vm_refcnt == 1) {
  
  		/*
 -		 * resize the map
 +		 * The vmspace is not shared and is empty.
 +		 * Resize the map and set topdown as appropriate.
  		 */
  
  		vm_map_setmin(map, start);
  		vm_map_setmax(map, end);
 +		if (topdown) {
 +			map->flags |= VM_MAP_TOPDOWN;
 +		} else {
 +			map->flags &= ~VM_MAP_TOPDOWN;
 +		}
  	} else {
  
  		/*
 
 --Eqn6h8LjFSAmGTae--