Bug 125139 - [patch] [ata] bugs in ATAPI CD tray control
Summary: [patch] [ata] bugs in ATAPI CD tray control
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 7.0-STABLE
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-01 11:00 UTC by Jaakko Heinonen
Modified: 2018-05-20 23:49 UTC (History)
1 user (show)

See Also:


Attachments
atapi-cd-tray-control-fixes.diff (1.93 KB, patch)
2008-07-01 11:00 UTC, Jaakko Heinonen
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaakko Heinonen 2008-07-01 11:00:11 UTC
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.)
Comment 1 Philip Paeps freebsd_committer 2008-10-17 14:44:34 UTC
Responsible Changed
From-To: freebsd-bugs->philip

I'll take it.
Comment 2 Brooks Davis freebsd_committer 2008-10-17 15:05:09 UTC
State Changed
From-To: open->analyzed

I have verified this bug.  The refcounting of opens appears correct, but 
seems to need locking.
Comment 3 Jaakko Heinonen 2008-10-20 10:44:31 UTC
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
Comment 4 Jaakko Heinonen 2008-11-24 19:52:57 UTC
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;
 };
%%%
Comment 5 Brooks Davis freebsd_committer 2010-03-12 07:31:43 UTC
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.
Comment 6 Brooks Davis freebsd_committer 2014-06-17 14:53:59 UTC
Return to pool.
Comment 7 Eitan Adler freebsd_committer freebsd_triage 2018-05-20 23:49:50 UTC
For bugs matching the following conditions:
- Status == In Progress
- Assignee == "bugs@FreeBSD.org"
- Last Modified Year <= 2017

Do
- Set Status to "Open"