From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: Kernel protection fault in fill_kproc()
To: Philip Guenther <guenther@gmail.com>
Cc: 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 13:29:55 +0200

On Tue, Aug 12, 2025 at 02:02:45AM -0700, Philip Guenther wrote:
> Nope. Hiding all zombie processes from ps is going to be both confusing and
> misleading.

I'm not talking about hiding them. I was more thinking of skipping
vm_resident_count() for processes that have PS_EXITING set.
The rss value returned for a process that is in exit1() or in the reaper()
is most probably incorrect anyway.  Maybe a few more bits need similar
treatment.
 
> Philip Guenther
> 
> On Monday, August 11, 2025, Claudio Jeker <cjeker@diehard.n-r-g.com> 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.
> >
> >
> > > 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
> >
> >

-- 
:wq Claudio