From: Matthew Dillon Date: Thu, 22 Aug 2019 15:31:29 +0000 (-0700) Subject: kernel - Fix devfs bitmap races for pty and other devices X-Git-Tag: v5.8.0rc1~1105 X-Git-Url: https://gitweb.dragonflybsd.org/dragonfly.git/commitdiff_plain/1991e949fd5daddb34cbf77ff2b492cd33ac0570 kernel - Fix devfs bitmap races for pty and other devices * 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. --- diff --git a/sys/dev/disk/vn/vn.c b/sys/dev/disk/vn/vn.c index 32a7ab8c48..cd7e96da68 100644 --- a/sys/dev/disk/vn/vn.c +++ b/sys/dev/disk/vn/vn.c @@ -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; } diff --git a/sys/dev/misc/snp/snp.c b/sys/dev/misc/snp/snp.c index 38415e01de..05b011d4c0 100644 --- a/sys/dev/misc/snp/snp.c +++ b/sys/dev/misc/snp/snp.c @@ -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; diff --git a/sys/net/bpf.c b/sys/net/bpf.c index 76440c96bf..b9ca2e84a2 100644 --- a/sys/net/bpf.c +++ b/sys/net/bpf.c @@ -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); diff --git a/sys/sys/devfs.h b/sys/sys/devfs.h index 857fafd220..c9ca6473ee 100644 --- a/sys/sys/devfs.h +++ b/sys/sys/devfs.h @@ -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); diff --git a/sys/vfs/devfs/devfs_helper.c b/sys/vfs/devfs/devfs_helper.c index 0a1d137b58..92b10ac6e8 100644 --- a/sys/vfs/devfs/devfs_helper.c +++ b/sys/vfs/devfs/devfs_helper.c @@ -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 @@ -40,6 +40,16 @@ 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; }