From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: Kernel protection fault in fill_kproc()
To: Gerhard Roth <gerhard_roth@genua.de>
Cc: "tech@openbsd.org" <tech@openbsd.org>, "mpi@openbsd.org" <mpi@openbsd.org>, Carsten Beckmann <carsten_beckmann@genua.de>
Date: Tue, 12 Aug 2025 07:22:29 +0200

On Mon, Aug 11, 2025 at 10:45:05AM +0000, Gerhard Roth wrote:
> About a year ago, the call to uvm_exit() was moved outside of the 
> KERNEL_LOCK() in the reaper() by mpi@. Now we observed a kernel
> protection fault that results from this change.
> 
> In fill_kproc() we read the vmspace pointer (vm) right at the very
> beginning of the function:
> 
>         struct vmspace *vm = pr->ps_vmspace;
> 
> Sometime later, we try to access it:
> 
> 	/* fixups that can only be done in the kernel */
> 	if ((pr->ps_flags & PS_ZOMBIE) == 0) {
> 		if ((pr->ps_flags & PS_EMBRYO) == 0 && vm != NULL)
> 			ki->p_vm_rssize = vm_resident_count(vm);
> 
> 
> In the meantime the process might have exited and the reaper() can free
> the vmspace by calling uvm_exit(). After that, the 'vm' pointer in
> fill_kproc() points to stale memory. Accessing it will yield a kernel
> protection fault.
> 
> BTW: only after freeing the vmspace of the process, the PS_ZOMBIE flag
> is set by the reaper().
> 
> I propose to put the reaper()'s call to uvm_exit() back under the
> kernel lock to avoid the fault.

In my opinion the fill_kproc() code is wrong and it should not look at
pr->ps_vmspace if the PS_EXITING flag is set for the process.

exit1() sets PS_EXITING flag early on and after that point the vm can be
purged so the vm_resident_count() is probably wrong anyway.

 
> Gerhard
> 
> 
> Index: sys/kern/kern_exit.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_exit.c,v
> diff -u -p -u -p -r1.252 kern_exit.c
> --- sys/kern/kern_exit.c	10 Aug 2025 15:17:57 -0000	1.252
> +++ sys/kern/kern_exit.c	11 Aug 2025 10:30:57 -0000
> @@ -498,10 +498,15 @@ reaper(void *arg)
>  		} else {
>  			struct process *pr = p->p_p;
>  
> -			/* Release the rest of the process's vmspace */
> +			/*
> +			 * Release the rest of the process's vmspace
> +			 * Use the kernel lock to avoid a race with fill_kproc()
> +			 * accessing the vmspace while the process isn't yet a
> +			 * zombie.
> +			 */
> +			KERNEL_LOCK();
>  			uvm_exit(pr);
>  
> -			KERNEL_LOCK();
>  			if ((pr->ps_flags & PS_NOZOMBIE) == 0) {
>  				/* Process is now a true zombie. */
>  				atomic_setbits_int(&pr->ps_flags, PS_ZOMBIE);
> 



-- 
:wq Claudio