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?

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);

Reply via email to