> Date: Tue, 11 Jan 2022 08:15:13 +0000
> From: Klemens Nanni <k...@openbsd.org>
>
> 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.
That diff is not right: u_int64_t is long long on 32-bit architectures
so that cast you're doing isn't right. I also don't think it is safe
to call log(9) without holding the kernel lock yet.
Now this is clearly a "slow" path. I don't think there is any reason
not to put all the code in that if (uvw_wxabort) block under the
kernel lock. For now I think making access to ps_wxcounter atomic is
just too fine grained.
> 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);
> /* Send uncatchable SIGABRT for coredump */
> + KERNEL_LOCK();
> sigexit(p, SIGABRT);
> + KERNEL_UNLOCK();
> }
>
> return ENOTSUP;
>
>
