From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: Kernel protection fault in fill_kproc()
To: Gerhard Roth <gerhard_roth@genua.de>, "tech@openbsd.org" <tech@openbsd.org>, "mpi@openbsd.org" <mpi@openbsd.org>, Carsten Beckmann <carsten_beckmann@genua.de>
Date: Tue, 12 Aug 2025 11:56:10 +0300

On Tue, Aug 12, 2025 at 07:22:29AM +0200, Claudio Jeker wrote:
> 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.
> 

The only fill_kproc() is the sysctl_doproc() which does the check in the
beginning of the allprocess loop:

        for (; pr != NULL; pr = LIST_NEXT(pr, ps_list)) {
                /* XXX skip processes in the middle of being zapped */
                if (pr->ps_pgrp == NULL)
                        continue;


According your suggestion the check should be like in this diff:

Index: sys/kern/kern_sysctl.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
diff -u -p -r1.481 kern_sysctl.c
--- sys/kern/kern_sysctl.c	24 Jul 2025 19:42:41 -0000	1.481
+++ sys/kern/kern_sysctl.c	12 Aug 2025 08:54:07 -0000
@@ -1922,14 +1922,10 @@ sysctl_doproc(int *name, u_int namelen, 
 	doingzomb = 0;
 again:
 	for (; pr != NULL; pr = LIST_NEXT(pr, ps_list)) {
-		/* XXX skip processes in the middle of being zapped */
-		if (pr->ps_pgrp == NULL)
-			continue;
-
 		/*
-		 * Skip embryonic processes.
+		 * Skip embryonic processes or the exiting processes.
 		 */
-		if (pr->ps_flags & PS_EMBRYO)
+		if (pr->ps_flags & (PS_EXITING | PS_EMBRYO))
 			continue;
 
 		/*