kernel - Fix devfs bitmap races for pty and other devices
authorMatthew Dillon <dillon@apollo.backplane.com>
Thu, 22 Aug 2019 15:31:29 +0000 (08:31 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Thu, 22 Aug 2019 15:31:29 +0000 (08:31 -0700)
* Use an internal lock to protect the integrity of bitmap
  operations for devfs_clone_bitmap_*() functions, allowing
  devices to use the functions without having to lock themselves.

* Devices which use devfs_clone_bitmap_chk() + devfs_clone_bitmap_set()
  sequences have to be more careful, either having their own covering
  lock or checking the return code from devfs_clone_bitmap_set() and
  looping.

* This fixes serious /dev/ptmx pty allocation races which become obvious
  when pty's are allocated concurrently at a high rate, such as by the
  dsynth code.

sys/dev/disk/vn/vn.c
sys/dev/misc/snp/snp.c
sys/net/bpf.c
sys/sys/devfs.h
sys/vfs/devfs/devfs_helper.c

index 32a7ab8..cd7e96d 100644 (file)
@@ -159,8 +159,10 @@ vnclone(struct dev_clone_args *ap)
 {
        int unit;
 
-       unit = devfs_clone_bitmap_get(&DEVFS_CLONE_BITMAP(vn), 0);
-       ap->a_dev = vn_create(unit, &DEVFS_CLONE_BITMAP(vn), 1);
+       do {
+               unit = devfs_clone_bitmap_get(&DEVFS_CLONE_BITMAP(vn), 0);
+               ap->a_dev = vn_create(unit, &DEVFS_CLONE_BITMAP(vn), 1);
+       } while (ap->a_dev == NULL);
 
        return 0;
 }
@@ -842,6 +844,9 @@ vnsize(struct dev_psize_args *ap)
        return(0);
 }
 
+/*
+ * Returns NULL only if the specified unit cannot be allocated.
+ */
 static cdev_t
 vn_create(int unit, struct devfs_bitmap *bitmap, int clone)
 {
@@ -849,6 +854,12 @@ vn_create(int unit, struct devfs_bitmap *bitmap, int clone)
        struct disk_info info;
        cdev_t dev, ret_dev;
 
+       /*
+        * Returns NULL onlyif the unit is already set in the bitmap
+        */
+       if (devfs_clone_bitmap_set(bitmap, unit) < 0)
+               return NULL;
+
        vn = vncreatevn();
        if (clone) {
                /*
@@ -872,9 +883,6 @@ vn_create(int unit, struct devfs_bitmap *bitmap, int clone)
        info.d_ncylinders = 0;
        disk_setdiskinfo_sync(&vn->sc_disk, &info);
 
-       if (bitmap != NULL)
-               devfs_clone_bitmap_set(bitmap, unit);
-
        return ret_dev;
 }
 
index 38415e0..05b011d 100644 (file)
@@ -485,6 +485,7 @@ snpclose(struct dev_close_args *ap)
 {
        cdev_t dev = ap->a_head.a_dev;
        struct snoop *snp;
+       int unit;
        int ret;
 
        lwkt_gettoken(&snp_token);
@@ -494,12 +495,14 @@ snpclose(struct dev_close_args *ap)
        kfree(snp->snp_buf, M_SNP);
        snp->snp_flags &= ~SNOOP_OPEN;
        dev->si_drv1 = NULL;
-       if (dev->si_uminor >= SNP_PREALLOCATED_UNITS) {
-               devfs_clone_bitmap_put(&DEVFS_CLONE_BITMAP(snp), dev->si_uminor);
+       unit = dev->si_uminor;
+       if (unit >= SNP_PREALLOCATED_UNITS) {
                destroy_dev(dev);
+               devfs_clone_bitmap_put(&DEVFS_CLONE_BITMAP(snp), unit);
        }
        ret = snp_detach(snp);
        lwkt_reltoken(&snp_token);
+
        return ret;
 }
 
@@ -719,7 +722,8 @@ snp_modevent(module_t mod, int type, void *data)
                        snpclone, UID_ROOT, GID_WHEEL, 0600, "snp");
 
                for (i = 0; i < SNP_PREALLOCATED_UNITS; i++) {
-                       make_dev(&snp_ops, i, UID_ROOT, GID_WHEEL, 0600, "snp%d", i);
+                       make_dev(&snp_ops, i, UID_ROOT, GID_WHEEL, 0600,
+                                "snp%d", i);
                        devfs_clone_bitmap_set(&DEVFS_CLONE_BITMAP(snp), i);
                }
                break;
index 76440c9..b9ca2e8 100644 (file)
@@ -409,6 +409,7 @@ bpfclose(struct dev_close_args *ap)
 {
        cdev_t dev = ap->a_head.a_dev;
        struct bpf_d *d = dev->si_drv1;
+       int unit;
 
        lwkt_gettoken(&bpf_token);
        funsetown(&d->bd_sigio);
@@ -419,9 +420,11 @@ bpfclose(struct dev_close_args *ap)
                bpf_detachd(d);
        bpf_freed(d);
        dev->si_drv1 = NULL;
-       if (dev->si_uminor >= BPF_PREALLOCATED_UNITS) {
-               devfs_clone_bitmap_put(&DEVFS_CLONE_BITMAP(bpf), dev->si_uminor);
+
+       unit = dev->si_uminor;
+       if (unit >= BPF_PREALLOCATED_UNITS) {
                destroy_dev(dev);
+               devfs_clone_bitmap_put(&DEVFS_CLONE_BITMAP(bpf), unit);
        }
        kfree(d, M_BPF);
        lwkt_reltoken(&bpf_token);
index 857fafd..c9ca647 100644 (file)
@@ -336,7 +336,7 @@ struct devfs_unit_hash {
 
 void devfs_clone_bitmap_init(struct devfs_bitmap *);
 void devfs_clone_bitmap_uninit(struct devfs_bitmap *);
-void devfs_clone_bitmap_set(struct devfs_bitmap *, int);
+int devfs_clone_bitmap_set(struct devfs_bitmap *, int);
 int devfs_clone_bitmap_get(struct devfs_bitmap *, int);
 int devfs_clone_bitmap_chk(struct devfs_bitmap *, int);
 void devfs_clone_bitmap_put(struct devfs_bitmap *, int);
index 0a1d137..92b10ac 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009 The DragonFly Project.  All rights reserved.
+ * Copyright (c) 2009-2019 The DragonFly Project.  All rights reserved.
  *
  * This code is derived from software contributed to The DragonFly Project
  * by Alex Hornung <ahornung@gmail.com>
 
 MALLOC_DECLARE(M_DEVFS);
 
+/*
+ * Locallized lock to ensure the integrity of the bitmap/cloning
+ * code.  Callers using chk/set sequences must still check for races
+ * or have their own locks.
+ *
+ * We use a recursive lock only to allow *_get() to call *_set().
+ */
+static struct lock devfs_bitmap_lock =
+       LOCK_INITIALIZER("dbmap", 0, LK_CANRECURSE);
+
 /*
  * DEVFS clone bitmap functions
  */
@@ -115,7 +125,9 @@ devfs_clone_bitmap_fff(struct devfs_bitmap *bitmap)
  * Caller wants to know if the specified unit has been allocated
  * or not.  Return 0, indicating that it has not been allocated.
  *
- * Caller must hold a lock to prevent chk-to-set races.
+ * Caller must hold a lock to prevent chk-to-set races.  Devfs also
+ * obtains a temporary lock, juse in case, to ensure structural
+ * integrity.
  *
  * (the bitmap implements 0=allocated, 1=not-allocated but the
  * return value is inverted).
@@ -124,35 +136,57 @@ int
 devfs_clone_bitmap_chk(struct devfs_bitmap *bitmap, int unit)
 {
        int chunk;
+       int res;
 
        chunk = unit / (sizeof(u_long) * 8);
        unit -= chunk * (sizeof(u_long) * 8);
 
-       if (chunk >= bitmap->chunks)
+       lockmgr(&devfs_bitmap_lock, LK_EXCLUSIVE);
+       if (chunk >= bitmap->chunks) {
+               lockmgr(&devfs_bitmap_lock, LK_RELEASE);
                return 0;               /* device does not exist */
+       }
+       res = !((bitmap->bitmap[chunk]) & (1UL << unit));
+       lockmgr(&devfs_bitmap_lock, LK_RELEASE);
 
-       return !((bitmap->bitmap[chunk]) & (1UL << unit));
+       return res;
 }
 
 /*
  * Unconditionally mark the specified unit as allocated in the bitmap.
  *
- * Caller must hold a lock to prevent chk-to-set races.  If the unit had
+ * Caller must hold a lock to prevent chk-to-set races, or otherwise
+ * check for a return value < 0 from this routine.  If the unit had
  * never been allocated in the past, a token lock might not be sufficient
  * to avoid races (if this function must extend the bitmap it could block
  * temporary in kmalloc()).
+ *
+ * devfs acquires a temporary lock to ensure structural integrity.
+ *
+ * If the bit is already clear (indicating that the unit is already
+ * allocated), return -1.  Otherwise return 0.
  */
-void
+int
 devfs_clone_bitmap_set(struct devfs_bitmap *bitmap, int unit)
 {
        int chunk;
+       int res;
 
        chunk = unit / (sizeof(u_long) * 8);
        unit -= chunk * (sizeof(u_long) * 8);
 
+       lockmgr(&devfs_bitmap_lock, LK_EXCLUSIVE);
        if (chunk >= bitmap->chunks)
                devfs_clone_bitmap_extend(bitmap, chunk);
-       bitmap->bitmap[chunk] &= ~(1UL << unit);
+       if (bitmap->bitmap[chunk] & (1UL << unit)) {
+               bitmap->bitmap[chunk] &= ~(1UL << unit);
+               res = 0;
+       } else {
+               res = -1;
+       }
+       lockmgr(&devfs_bitmap_lock, LK_RELEASE);
+
+       return res;
 }
 
 /*
@@ -170,9 +204,10 @@ devfs_clone_bitmap_put(struct devfs_bitmap *bitmap, int unit)
        chunk = unit / (sizeof(u_long) * 8);
        unit -= chunk * (sizeof(u_long) * 8);
 
-       if (chunk >= bitmap->chunks)
-               return;
-       bitmap->bitmap[chunk] |= (1UL << unit);
+       lockmgr(&devfs_bitmap_lock, LK_EXCLUSIVE);
+       if (chunk < bitmap->chunks)
+               bitmap->bitmap[chunk] |= (1UL << unit);
+       lockmgr(&devfs_bitmap_lock, LK_RELEASE);
 }
 
 /*
@@ -187,10 +222,14 @@ devfs_clone_bitmap_get(struct devfs_bitmap *bitmap, int limit)
 {
        int unit;
 
+       lockmgr(&devfs_bitmap_lock, LK_EXCLUSIVE);
        unit = devfs_clone_bitmap_fff(bitmap);
-       if ((limit > 0) && (unit > limit))
-               return -1;
-       devfs_clone_bitmap_set(bitmap, unit);
+       if (limit > 0 && unit > limit) {
+               unit = -1;
+       } else {
+               devfs_clone_bitmap_set(bitmap, unit);
+       }
+       lockmgr(&devfs_bitmap_lock, LK_RELEASE);
 
        return unit;
 }