> Date: Thu, 1 Oct 2020 14:10:56 +0200
> From: Martin Pieuchot <m...@openbsd.org>
>
> While studying a bug report from naddy@ in 2017 when testing guenther@'s
> amap/anon locking diff I figured out that we have been too optimistic in
> the !MAP_ANON case.
>
> The reported panic involves, I'd guess, a race between fd_getfile() and
> vref():
>
> panic: vref used where vget required
> db_enter() at db_enter+0x5
> panic() at panic+0x129
> vref(ffffff03b20d29e8) at vref+0x5d
> uvn_attach(1010000,ffffff03a5879dc0) at uvn_attach+0x11d
> uvm_mmapfile(7,ffffff03a5879dc0,2,1,13,100000012) at uvm_mmapfile+0x12c
> sys_mmap(c50,ffff8000225f82a0,1) at sys_mmap+0x604
> syscall() at syscall+0x279
> --- syscall (number 198) ---
> end of kernel
>
> Removing the KERNEL_LOCK() from file mapping was out of the scope of this
> previous work, so I'd like to go back to a single KERNEL_LOCK/UNLOCK dance
> in this code path to remove any false positive.
>
> Note that this code is currently always run under KERNEL_LOCK() so this
> will only have effect once the syscall will be unlocked.
>
> ok?
Hmm, I thought fd_getfile() was fully mpsafe.
But I suppose the kernel lock needs to be grabbed before we start
looking at the vnode?
Your diff makes the locking a bit convoluted, but I suppose adding a
KERNEL_UNLOCK() before every "goto out" is worse?
> Index: uvm/uvm_mmap.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
> retrieving revision 1.161
> diff -u -p -r1.161 uvm_mmap.c
> --- uvm/uvm_mmap.c 4 Mar 2020 21:15:39 -0000 1.161
> +++ uvm/uvm_mmap.c 28 Sep 2020 09:48:26 -0000
> @@ -288,8 +288,11 @@ sys_mmap(struct proc *p, void *v, regist
>
> /* check for file mappings (i.e. not anonymous) and verify file. */
> if ((flags & MAP_ANON) == 0) {
> - if ((fp = fd_getfile(fdp, fd)) == NULL)
> - return (EBADF);
> + KERNEL_LOCK();
> + if ((fp = fd_getfile(fdp, fd)) == NULL) {
> + error = EBADF;
> + goto out;
> + }
>
> if (fp->f_type != DTYPE_VNODE) {
> error = ENODEV; /* only mmap vnodes! */
> @@ -313,6 +316,7 @@ sys_mmap(struct proc *p, void *v, regist
> flags |= MAP_ANON;
> FRELE(fp, p);
> fp = NULL;
> + KERNEL_UNLOCK();
> goto is_anon;
> }
>
> @@ -362,9 +366,7 @@ sys_mmap(struct proc *p, void *v, regist
> * EPERM.
> */
> if (fp->f_flag & FWRITE) {
> - KERNEL_LOCK();
> error = VOP_GETATTR(vp, &va, p->p_ucred, p);
> - KERNEL_UNLOCK();
> if (error)
> goto out;
> if ((va.va_flags & (IMMUTABLE|APPEND)) == 0)
> @@ -390,9 +392,9 @@ sys_mmap(struct proc *p, void *v, regist
> goto out;
> }
> }
> - KERNEL_LOCK();
> error = uvm_mmapfile(&p->p_vmspace->vm_map, &addr, size, prot,
> maxprot, flags, vp, pos, lim_cur(RLIMIT_MEMLOCK), p);
> + FRELE(fp, p);
> KERNEL_UNLOCK();
> } else { /* MAP_ANON case */
> if (fd != -1)
> @@ -428,7 +430,10 @@ is_anon: /* label for SunOS style /dev/z
> /* remember to add offset */
> *retval = (register_t)(addr + pageoff);
>
> + return (error);
> +
> out:
> + KERNEL_UNLOCK();
> if (fp)
> FRELE(fp, p);
> return (error);
>
>
