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. > >
This is true, sigexit() should be serialized with kernel lock. Ant this path will be followed only with non-null kern.wxabort. > 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 */ > > struct unveil *ps_uvpaths; /* unveil vnodes and names */ > ssize_t ps_uvvcount; /* count of unveil vnodes held */ > Index: uvm/uvm_mmap.c > =================================================================== > RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v > retrieving revision 1.168 > diff -u -p -r1.168 uvm_mmap.c > --- uvm/uvm_mmap.c 5 Jan 2022 17:53:44 -0000 1.168 > +++ uvm/uvm_mmap.c 10 Jan 2022 15:48:03 -0000 > @@ -184,11 +184,13 @@ uvm_wxcheck(struct proc *p, char *call) > > if (uvm_wxabort) { > /* Report W^X failures */ > - if (pr->ps_wxcounter++ == 0) > + if (atomic_inc_long_nv((unsigned long *)&pr->ps_wxcounter) == 1) > log(LOG_NOTICE, "%s(%d): %s W^X violation\n", > pr->ps_comm, pr->ps_pid, call); `ps_wxcounter' is u_int64_t variable. I doubt this cast to unsigned long would work as you expect on x32 architectures. > /* Send uncatchable SIGABRT for coredump */ > + KERNEL_LOCK(); > sigexit(p, SIGABRT); > + KERNEL_UNLOCK(); > } > > return ENOTSUP; >