From jaakko@saunalahti.fi Tue Jul 1 09:55:14 2008 Return-Path: Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5707710656BB for ; Tue, 1 Jul 2008 09:55:14 +0000 (UTC) (envelope-from jaakko@saunalahti.fi) Received: from gw03.mail.saunalahti.fi (gw03.mail.saunalahti.fi [195.197.172.111]) by mx1.freebsd.org (Postfix) with ESMTP id DC4AB8FC16 for ; Tue, 1 Jul 2008 09:55:13 +0000 (UTC) (envelope-from jaakko@saunalahti.fi) Received: from ws64.jh.dy.fi (a91-153-120-204.elisa-laajakaista.fi [91.153.120.204]) by gw03.mail.saunalahti.fi (Postfix) with ESMTP id B2743216B42 for ; Tue, 1 Jul 2008 12:55:11 +0300 (EEST) Received: from ws64.jh.dy.fi (localhost [127.0.0.1]) by ws64.jh.dy.fi (8.14.2/8.14.2) with ESMTP id m619tBd8036258 for ; Tue, 1 Jul 2008 12:55:11 +0300 (EEST) (envelope-from jaakko@ws64.jh.dy.fi) Received: (from jaakko@localhost) by ws64.jh.dy.fi (8.14.2/8.14.2/Submit) id m619tBYc036257; Tue, 1 Jul 2008 12:55:11 +0300 (EEST) (envelope-from jaakko) Message-Id: <200807010955.m619tBYc036257@ws64.jh.dy.fi> Date: Tue, 1 Jul 2008 12:55:11 +0300 (EEST) From: Jaakko Heinonen To: FreeBSD-gnats-submit@freebsd.org Cc: Subject: [patch] [ata] bugs in ATAPI CD tray control X-Send-Pr-Version: 3.113 X-GNATS-Notify: >Number: 125139 >Category: kern >Synopsis: [patch] [ata] bugs in ATAPI CD tray control >Confidential: no >Severity: non-critical >Priority: low >Responsible: brooks >State: analyzed >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Tue Jul 01 10:00:11 UTC 2008 >Closed-Date: >Last-Modified: Fri Mar 12 07:33:48 UTC 2010 >Originator: Jaakko Heinonen >Release: FreeBSD 7.0-STABLE / 8.0-CURRENT >Organization: >Environment: System: FreeBSD x 7.0-STABLE FreeBSD 7.0-STABLE #4: Mon Jun 30 19:10:47 EEST 2008 x:X amd64 >Description: 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. >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.) >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). --- atapi-cd-tray-control-fixes.diff begins here --- Index: sys/dev/ata/atapi-cd.c =================================================================== --- sys/dev/ata/atapi-cd.c (revision 180023) +++ sys/dev/ata/atapi-cd.c (working copy) @@ -246,11 +246,19 @@ acd_geom_ioctl(struct g_provider *pp, u_ break; case CDIOCALLOW: + if (cdp->gnp != 1 || pp->acr != 1) { + error = EBUSY; + break; + } error = acd_prevent_allow(dev, 0); cdp->flags &= ~F_LOCKED; break; case CDIOCPREVENT: + if (cdp->gnp != 1 || pp->acr != 1) { + error = EBUSY; + break; + } error = acd_prevent_allow(dev, 1); cdp->flags |= F_LOCKED; break; @@ -266,7 +274,7 @@ acd_geom_ioctl(struct g_provider *pp, u_ break; case CDIOCEJECT: - if (pp->acr != 1) { + if (cdp->gnp != 1 || pp->acr != 1) { error = EBUSY; break; } @@ -274,7 +282,7 @@ acd_geom_ioctl(struct g_provider *pp, u_ break; case CDIOCCLOSE: - if (pp->acr != 1) + if (cdp->gnp != 1 || pp->acr != 1) break; error = acd_tray(dev, 1); break; @@ -713,12 +721,16 @@ acd_geom_access(struct g_provider *pp, i ata_free_request(request); if (pp->acr == 0) { + cdp->gnp++; acd_prevent_allow(dev, 1); cdp->flags |= F_LOCKED; acd_read_toc(dev); } - if (dr + pp->acr == 0) { + if (dr + pp->acr == 0) + cdp->gnp--; + + if (cdp->gnp == 0) { acd_prevent_allow(dev, 0); cdp->flags &= ~F_LOCKED; } Index: sys/dev/ata/atapi-cd.h =================================================================== --- sys/dev/ata/atapi-cd.h (revision 180023) +++ sys/dev/ata/atapi-cd.h (working copy) @@ -312,4 +312,6 @@ 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 gnp; /* number of geom providers + attached to the device */ }; --- atapi-cd-tray-control-fixes.diff ends here --- >Release-Note: >Audit-Trail: Responsible-Changed-From-To: freebsd-bugs->philip Responsible-Changed-By: philip Responsible-Changed-When: Fri Oct 17 13:44:34 UTC 2008 Responsible-Changed-Why: I'll take it. http://www.freebsd.org/cgi/query-pr.cgi?pr=125139 State-Changed-From-To: open->analyzed State-Changed-By: brooks State-Changed-When: Fri Oct 17 14:05:09 UTC 2008 State-Changed-Why: I have verified this bug. The refcounting of opens appears correct, but seems to need locking. http://www.freebsd.org/cgi/query-pr.cgi?pr=125139 From: Jaakko Heinonen To: brooks@FreeBSD.org Cc: philip@FreeBSD.org, bug-followup@FreeBSD.org Subject: Re: kern/125139: [patch] [ata] bugs in ATAPI CD tray control Date: Mon, 20 Oct 2008 12:44:31 +0300 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 From: Jaakko Heinonen To: brooks@FreeBSD.org Cc: philip@FreeBSD.org, bug-followup@FreeBSD.org Subject: Re: kern/125139: [patch] [ata] bugs in ATAPI CD tray control Date: Mon, 24 Nov 2008 21:52:57 +0200 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 Responsible-Changed-By: brooks Responsible-Changed-When: Fri Mar 12 07:31:43 UTC 2010 Responsible-Changed-Why: 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. http://www.freebsd.org/cgi/query-pr.cgi?pr=125139 >Unformatted: