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.
> 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;
>
--
:wq Claudio
