On 11/01/22 09:57 +0100, Claudio Jeker wrote:
> On Tue, Jan 11, 2022 at 08:15:13AM +0000, Klemens Nanni wrote:
> > On Mon, Jan 10, 2022 at 12:06:44PM +0000, Klemens Nanni wrote:
> > > On Fri, Dec 31, 2021 at 07:54:53PM +0300, Vitaliy Makkoveev wrote:
> > > > The uvm_wxabort path within uvm_wxcheck() looks not MP-safe.
> > >
> > > Right, I did not pay enough attention to W^X handling.
> >
> > New diff, either alone or on top of the mmap unlocking.
> >
> > > I'm not entirely sure about the sigexit() path.
> >
> > We can't dump core without the kernel lock, assertions do make sure...
> >
> > > There's `ps_wxcounter' as u_int64_t which needs a lock or atomic
> > > operations.
> >
> > This could look like this. `ps_wxcounter' is not used anywhere else in
> > the tree; it has been like this since import in 2016.
> >
> > > The kernel lock could be pushed into uvm_wxabort() but there it'd still
> > > be grabbed for every mmap(2) call.
> >
> > This means we're only grabbing the kernel lock if `kern.wxabort=1' is
> > set *and* there's a W^X violation -- much better.
> >
> >
> > Index: sys/proc.h
> > ===================================================================
> > RCS file: /cvs/src/sys/sys/proc.h,v
> > retrieving revision 1.323
> > diff -u -p -r1.323 proc.h
> > --- sys/proc.h 10 Dec 2021 05:34:42 -0000 1.323
> > +++ sys/proc.h 9 Jan 2022 13:42:45 -0000
> > @@ -197,7 +197,7 @@ struct process {
> > time_t ps_nextxcpu; /* when to send next SIGXCPU, */
> > /* in seconds of process runtime */
> >
> > - u_int64_t ps_wxcounter;
> > + u_int64_t ps_wxcounter; /* [a] number of W^X violations */
>
> This can't be right. We don't have 64bit atomic instructions on all archs.
> Either this needs to be unsigned long or unsigned int.
> Since this is only used to limit the number of reports per process I would
> suggest to mover this to an unsigned int. You can't have that many threads
> to overflow that count.
> Or another option is to move the ps_wxcounter into the KERNEL_LOCK block
> which is also trivial and will fix this for as long as sigexit requires
> the KERNEL_LOCK.
I think moving the KERNEL_LOCK block is good enough.
