End two weeks of on and off debugging. Fix the crash on the Nth

insertion of a CF card, for random values of N > 1.  With these fixes,
I've been able to do 100 insert/remove of the cards w/o a crash with
lots of system activity going on that in the past would help trigger
the crash.

The problem:

FreeBSD creates dev_t's on the fly as they are needed and never
destroys them.  These dev_t's point to a struct disk that is used for
housekeeping on the disk.  When a device goes away, the struct disk
pointer becomes a dangling pointer.  Sometimes when the device comes
back, the pointer will point to the new struct disk (in which case the
insertion will work).  Other times it won't (especially if any length
of time has passed, since it is dependent on memory returned from
malloc).

The Fix:

There is one of these dev_t's that is always correct.  The
device for the WHOLE_DISK_SLICE is always right.  It gets set at
create_disk() time.  So, the fix is to spend a little CPU time and
lookup the WHOLE_DISK_SLICE dev_t and use the si_disk from that in
preference to the one that's in the device asking to do the I/O.  In
addition, we change the test of si_disk == NULL meaning that the dev
needed to inherit properties from the pdev to dev->si_disk !=
pdev->si_disk.  This test is a little stronger than the previous test,
but can sometimes be fooled into not inheriting.  However, the results
of this fooling are that the old values will be used, which will
generally always be the same as before.  si_drv[12] are the only
values that are copied that might pose a problem.  They tend to change
as the si_disk field would change, so it is a hole, but it is a small
hole.

One could correctly argue that one should replace much of this code
with something much much better.  I would be on the pro side of that
argument.

Reviewed by: phk (who also ported the original patch to current)
Sponsored by: Timing Solutions

1 files changed, 27 insertions, 24 deletions

diff --git a/sys/kern/subr_disk.c b/sys/kern/subr_disk.c
index 2a11923..8bb0c6d 100644
--- a/sys/kern/subr_disk.c
+++ b/sys/kern/subr_disk.c
@@ -31,6 +31,17 @@ static d_psize_t diskpsize;
 
 static LIST_HEAD(, disk) disklist = LIST_HEAD_INITIALIZER(&disklist);
  
+static void
+inherit_raw(dev_t pdev, dev_t dev)
+{
+	dev->si_disk = pdev->si_disk;
+	dev->si_drv1 = pdev->si_drv1;
+	dev->si_drv2 = pdev->si_drv2;
+	dev->si_iosize_max = pdev->si_iosize_max;
+	dev->si_bsize_phys = pdev->si_bsize_phys;
+	dev->si_bsize_best = pdev->si_bsize_best;
+}
+
 dev_t
 disk_create(int unit, struct disk *dp, int flags, struct cdevsw *cdevsw, struct cdevsw *proto)
 {
@@ -176,12 +187,7 @@ diskopen(dev_t dev, int oflags, int devtype, struct proc *p)
 	}
 
 	/* Inherit properties from the whole/raw dev_t */
-	dev->si_disk = pdev->si_disk;
-	dev->si_drv1 = pdev->si_drv1;
-	dev->si_drv2 = pdev->si_drv2;
-	dev->si_iosize_max = pdev->si_iosize_max;
-	dev->si_bsize_phys = pdev->si_bsize_phys;
-	dev->si_bsize_best = pdev->si_bsize_best;
+	inherit_raw(pdev, dev);
 
 	if (error)
 		goto out;
@@ -205,9 +211,11 @@ diskclose(dev_t dev, int fflag, int devtype, struct proc *p)
 {
 	struct disk *dp;
 	int error;
+	dev_t pdev;
 
 	error = 0;
-	dp = dev->si_disk;
+	pdev = dkmodpart(dkmodslice(dev, WHOLE_DISK_SLICE), RAW_PART);
+	dp = pdev->si_disk;
 	dsclose(dev, devtype, dp->d_slice);
 	if (!dsisopen(dp->d_slice)) {
 		error = dp->d_devsw->d_close(dp->d_dev, fflag, devtype, p);
@@ -221,16 +229,10 @@ diskstrategy(struct bio *bp)
 	dev_t pdev;
 	struct disk *dp;
 
-	dp = bp->bio_dev->si_disk;
-	if (!dp) {
-		pdev = dkmodpart(dkmodslice(bp->bio_dev, WHOLE_DISK_SLICE), RAW_PART);
-		dp = bp->bio_dev->si_disk = pdev->si_disk;
-		bp->bio_dev->si_drv1 = pdev->si_drv1;
-		bp->bio_dev->si_drv2 = pdev->si_drv2;
-		bp->bio_dev->si_iosize_max = pdev->si_iosize_max;
-		bp->bio_dev->si_bsize_phys = pdev->si_bsize_phys;
-		bp->bio_dev->si_bsize_best = pdev->si_bsize_best;
-	}
+	pdev = dkmodpart(dkmodslice(bp->bio_dev, WHOLE_DISK_SLICE), RAW_PART);
+	dp = pdev->si_disk;
+	if (dp != bp->bio_dev->si_disk)
+		inherit_raw(pdev, bp->bio_dev);
 
 	if (!dp) {
 		bp->bio_error = ENXIO;
@@ -256,8 +258,10 @@ diskioctl(dev_t dev, u_long cmd, caddr_t data, int fflag, struct proc *p)
 {
 	struct disk *dp;
 	int error;
+	dev_t pdev;
 
-	dp = dev->si_disk;
+	pdev = dkmodpart(dkmodslice(dev, WHOLE_DISK_SLICE), RAW_PART);
+	dp = pdev->si_disk;
 	error = dsioctl(dev, cmd, data, fflag, &dp->d_slice);
 	if (error == ENOIOCTL)
 		error = dp->d_devsw->d_ioctl(dev, cmd, data, fflag, p);
@@ -270,12 +274,11 @@ diskpsize(dev_t dev)
 	struct disk *dp;
 	dev_t pdev;
 
-	dp = dev->si_disk;
-	if (!dp) {
-		pdev = dkmodpart(dkmodslice(dev, WHOLE_DISK_SLICE), RAW_PART);
-		dp = pdev->si_disk;
-		if (!dp)
-			return (-1);
+	pdev = dkmodpart(dkmodslice(dev, WHOLE_DISK_SLICE), RAW_PART);
+	dp = pdev->si_disk;
+	if (!dp)
+		return (-1);
+	if (dp != dev->si_disk) {
 		dev->si_drv1 = pdev->si_drv1;
 		dev->si_drv2 = pdev->si_drv2;
 		/* XXX: don't set bp->b_dev->si_disk (?) */