There are several bugs in ATAPI driver concerning CD drive tray control: 1) The ATAPI driver locks the tray when a device is opened and unlocks it when a device is closed. If there are several device files associated with a drive the tray unlocks when one of the files closes even if there are other open device files. 2) CDIOCALLOW and CDIOCPREVENT ioctls are allowed if a device is open (for example when disc is mounted) but CDIOCEJECT is not allowed. cdcontrol(1) eject command does first CDIOCALLOW ioctl followed by CDIOCEJECT. With mounted disc this unlocks drive but doesn't eject it. This could be worked around in cdcontrol(1) but possibly there should be consistency when CDIOCALLOW, CDIOCPREVENT and CDIOCEJECT ioctls are allowed. The problem is also described in the PR kern/118779 and in this message: http://lists.freebsd.org/pipermail/freebsd-stable/2007-December/039162.html . 3) CDIOCEJECT is only disallowed if the same device file is in use. However there may be other device files associated with the drive. Fix: This patch adds a counter of attached geom providers to ATAPI CDROM device structure. Using that counter we can correctly detect if the drive is in use. The patch also adds the same busy check to CDIOCALLOW and CDIOCPREVENT ioctls that CDIOCEJECT ioctl has. I am not 100% sure if this is the right way but I think that those three ioctls should have consistent checks. The SCSI CD-ROM driver doesn't have any checks for those ioctls. The patch should fix all bugs described above (including kern/118779). How-To-Repeat: (Data CD in the drive) Case 1: # mount -t cd9660 /dev/acd0 /cdrom # # head -c1 /dev/acd0t01 # (Now you can eject the tray from button.) Case 2: # mount -t cd9660 /dev/acd0 /cdrom # cdcontrol eject # (Tray doesn't open but you can eject it from button now.) Case 3: (CD is not mounted) # less -f /dev/acd0t01 (leave /dev/acd0t01 open) # cdcontrol eject # (Tray opens even /dev/acd0t01 is kept open.)
Responsible Changed From-To: freebsd-bugs->philip I'll take it.
State Changed From-To: open->analyzed I have verified this bug. The refcounting of opens appears correct, but seems to need locking.
On 2008-10-17, brooks@FreeBSD.org wrote: > I have verified this bug. The refcounting of opens appears correct, > but seems to need locking. I assumed that the GEOM topology lock is held during acd_geom_ioctl() and acd_geom_access() calls but indeed it's not true for acd_geom_ioctl(). Looks like acd_geom_ioctl() misses locking completely. -- Jaakko
On 2008-10-17, brooks@FreeBSD.org wrote: > I have verified this bug. The refcounting of opens appears correct, but > seems to need locking. Here is an updated patch which uses a per device sx lock for softc locking. There are many sleepable code paths which makes using a mutex hard. (The patch contains also an unrelated fix to check g_modevent() return value in acd_modevent(). Also I added a check for NULL cdp to acd_geom_access(). cdp could be NULL if the device is detaching.) %%% Index: sys/dev/ata/atapi-cd.c =================================================================== --- sys/dev/ata/atapi-cd.c (revision 185028) +++ sys/dev/ata/atapi-cd.c (working copy) @@ -124,6 +124,7 @@ acd_attach(device_t dev) device_printf(dev, "out of memory\n"); return ENOMEM; } + sx_init(&cdp->lock, "ATAPI CD softc lock"); cdp->block_size = 2048; device_set_ivars(dev, cdp); ATA_SETMODE(device_get_parent(dev), dev); @@ -140,7 +141,7 @@ static int acd_detach(device_t dev) { g_waitfor_event(acd_geom_detach, dev, M_WAITOK, NULL); - return 0; + return (device_get_ivars(dev) == NULL) ? 0 : EBUSY; } static void @@ -191,6 +192,13 @@ acd_geom_detach(void *arg, int flag) { struct acd_softc *cdp = device_get_ivars(arg); + g_topology_assert(); + sx_slock(&cdp->lock); + if (cdp->grefcnt != 0) { + sx_sunlock(&cdp->lock); + return; + } + /* signal geom so we dont get any further requests */ g_wither_geom(cdp->gp, ENXIO); @@ -199,6 +207,8 @@ acd_geom_detach(void *arg, int flag) /* dont leave anything behind */ device_set_ivars(arg, NULL); + sx_sunlock(&cdp->lock); + sx_destroy(&cdp->lock); free(cdp, M_ACD); } @@ -213,6 +223,8 @@ acd_geom_ioctl(struct g_provider *pp, u_ if (!cdp) return ENXIO; + sx_xlock(&cdp->lock); + if (atadev->flags & ATA_D_MEDIA_CHANGED) { switch (cmd) { case CDIOCRESET: @@ -246,11 +258,19 @@ acd_geom_ioctl(struct g_provider *pp, u_ break; case CDIOCALLOW: + if (cdp->grefcnt != 1 || pp->acr != 1) { + error = EBUSY; + break; + } error = acd_prevent_allow(dev, 0); cdp->flags &= ~F_LOCKED; break; case CDIOCPREVENT: + if (cdp->grefcnt != 1 || pp->acr != 1) { + error = EBUSY; + break; + } error = acd_prevent_allow(dev, 1); cdp->flags |= F_LOCKED; break; @@ -266,7 +286,7 @@ acd_geom_ioctl(struct g_provider *pp, u_ break; case CDIOCEJECT: - if (pp->acr != 1) { + if (cdp->grefcnt != 1 || pp->acr != 1) { error = EBUSY; break; } @@ -274,8 +294,10 @@ acd_geom_ioctl(struct g_provider *pp, u_ break; case CDIOCCLOSE: - if (pp->acr != 1) + if (cdp->grefcnt != 1 || pp->acr != 1) { + error = EBUSY; break; + } error = acd_tray(dev, 1); break; @@ -678,6 +700,9 @@ acd_geom_ioctl(struct g_provider *pp, u_ default: error = ata_device_ioctl(dev, cmd, addr); } + + sx_xunlock(&cdp->lock); + return error; } @@ -691,6 +716,9 @@ acd_geom_access(struct g_provider *pp, i 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; int timeout = 60, track; + if (!cdp) + return ENXIO; + if (!(request = ata_alloc_request())) return ENOMEM; @@ -712,13 +740,19 @@ acd_geom_access(struct g_provider *pp, i } ata_free_request(request); + sx_xlock(&cdp->lock); + if (pp->acr == 0) { + cdp->grefcnt++; acd_prevent_allow(dev, 1); cdp->flags |= F_LOCKED; acd_read_toc(dev); } - if (dr + pp->acr == 0) { + if (dr + pp->acr == 0) + cdp->grefcnt--; + + if (cdp->grefcnt == 0) { acd_prevent_allow(dev, 0); cdp->flags &= ~F_LOCKED; } @@ -734,6 +768,8 @@ acd_geom_access(struct g_provider *pp, i } pp->mediasize *= pp->sectorsize; + sx_xunlock(&cdp->lock); + return 0; } @@ -748,7 +784,10 @@ acd_geom_start(struct bio *bp) return; } + sx_slock(&cdp->lock); + if (bp->bio_cmd == BIO_READ && cdp->disk_size == -1) { + sx_sunlock(&cdp->lock); g_io_deliver(bp, EIO); return; } @@ -782,6 +821,7 @@ acd_geom_start(struct bio *bp) bp2 = bp3; } } + sx_sunlock(&cdp->lock); } static void @@ -1906,8 +1946,7 @@ static devclass_t acd_devclass; static int acd_modevent(module_t mod, int what, void *arg) { - g_modevent(0, what, &acd_class); - return 0; + return g_modevent(0, what, &acd_class); } DRIVER_MODULE(acd, ata, acd_driver, acd_devclass, acd_modevent, NULL); Index: sys/dev/ata/atapi-cd.h =================================================================== --- sys/dev/ata/atapi-cd.h (revision 185028) +++ sys/dev/ata/atapi-cd.h (working copy) @@ -312,4 +312,7 @@ struct acd_softc { u_int32_t iomax; /* Max I/O request (bytes) */ struct g_geom *gp; /* geom instance */ struct g_provider *pp[MAXTRK+1]; /* providers */ + u_int grefcnt; /* number of open geom + providers */ + struct sx lock; }; %%%
Responsible Changed From-To: philip->brooks Steal this one from philip. I don't have the hardware to test this patch with me, but I'll do it once I get home.
Return to pool.
For bugs matching the following conditions: - Status == In Progress - Assignee == "bugs@FreeBSD.org" - Last Modified Year <= 2017 Do - Set Status to "Open"
Keyword: patch or patch-ready – in lieu of summary line prefix: [patch] * bulk change for the keyword * summary lines may be edited manually (not in bulk). Keyword descriptions and search interface: <https://bugs.freebsd.org/bugzilla/describekeywords.cgi>