> Date: Sun, 5 Dec 2021 18:01:04 -0600
> From: Scott Cheloha <[email protected]>
>
> Two things in sys_kbind() need an explicit kernel lock. First,
> sigexit(). Second, uvm_map_extract(), because the following call
> chain panics without it:
>
> panic
> assert
> uvn_reference
> uvm_mapent_clone
> uum_map_extract
> sys_kbind
> syscall
> Xsyscall
>
> uvn_reference() does KERNEL_ASSERT_LOCKED().
>
> These are the other routines called from sys_kbind():
>
> copyin
> kcopy
> trunc_page
> vm_map_lock
> vm_map_unlock
> uvm_unmap_detach
> uvm_unmap_remove
>
> copyin/kcopy are safe, trunc_page is safe, vm_map_lock/vm_map_unlock
> are safe, uvm_unmap_detach takes the kernel lock as needed, and
> uvm_unmap_remove has callees that take the kernel lock as needed.
>
> With this committed we can unlock kbind(2).
>
> Thoughts? ok?
To be honest, I don't think this makes sense unless you can make the
"normal" code path lock free. You're replacing a single
KERNEL_LOCK/UNLOCK pair with (potentially) a bunch of them. That may
actually make things worse. So I think we need to make
uvm_map_extract() mpsafe first.
In case mpi@ disagrees, the unbalanced KERNEL_LOCK() before the
sigexit() call is unfortunate. I'd add a /* NOTREACHED */ after the
sigexit() call to signal that the unbalanced KERNEL_LOCK() is ok.
> Index: uvm_mmap.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
> retrieving revision 1.165
> diff -u -p -r1.165 uvm_mmap.c
> --- uvm_mmap.c 5 Dec 2021 22:00:42 -0000 1.165
> +++ uvm_mmap.c 5 Dec 2021 23:57:28 -0000
> @@ -1112,10 +1112,12 @@ sys_kbind(struct proc *p, void *v, regis
> if (pr->ps_kbind_addr == 0) {
> pr->ps_kbind_addr = pc;
> pr->ps_kbind_cookie = SCARG(uap, proc_cookie);
> - } else if (pc != pr->ps_kbind_addr || pc == BOGO_PC)
> - sigexit(p, SIGILL);
> - else if (pr->ps_kbind_cookie != SCARG(uap, proc_cookie))
> + } else if (pc != pr->ps_kbind_addr || pc == BOGO_PC ||
> + pr->ps_kbind_cookie != SCARG(uap, proc_cookie)) {
> + KERNEL_LOCK();
> sigexit(p, SIGILL);
> + }
> +
> if (psize < sizeof(struct __kbind) || psize > sizeof(param))
> return EINVAL;
> if ((error = copyin(paramp, ¶m, psize)))
> @@ -1169,8 +1171,11 @@ sys_kbind(struct proc *p, void *v, regis
> vm_map_unlock(kernel_map);
> kva = 0;
> }
> - if ((error = uvm_map_extract(&p->p_vmspace->vm_map,
> - baseva, PAGE_SIZE, &kva, UVM_EXTRACT_FIXPROT)))
> + KERNEL_LOCK();
> + error = uvm_map_extract(&p->p_vmspace->vm_map,
> + baseva, PAGE_SIZE, &kva, UVM_EXTRACT_FIXPROT);
> + KERNEL_UNLOCK();
> + if (error)
> break;
> last_baseva = baseva;
> }
>
>
