Dealing with strange disk devices

[Martin Husemann <martin%duskware.de@localhost>]

Hi folks,

Constantine and I disagree on a small change he made recently. The issue
does not come up in any normal environment, and has apparently not happened
at all in the last years (or so). The bug is pretty old. Actually it is a
combination of several bugs that triggered it now:

 - when configuring VirtualBox wiht a NVMe controller, but no drive,
   strange things happen (not sure if that is a VirtualBox issue)
 - untill recently (this has been fixed) in that situation our kernel code
   fully attached a ld device to the non-functional NVMe.
 - when accessing a non-raw partition on such a bogus ld device (easy
   trigger: "dumpfs ld0a") a div by zero killed the kernel

Constantine fixed that with a simple and effective change:

--- subr_disk.c 22 May 2019 08:47:02 -0000      1.128
+++ subr_disk.c 30 Sep 2019 23:23:59 -0000      1.129
@@ -385,7 +385,7 @@ bounds_check_with_label(struct disk *dk,
        }
 
        /* Protect against division by zero. XXX: Should never happen?!?! */
-       if (lp->d_secpercyl == 0) {
+       if ((lp->d_secsize / DEV_BSIZE) == 0 || lp->d_secpercyl == 0) {
                bp->b_error = EINVAL;
                return -1;
        }


This makes bounds_check_with_label() fail early for all devices with small
sector size.

Instead I would like to fix it differently. The div by zero happend when
calculating the cylinder number for the buffer - which is not used a lot
typically (or not at all). This looks like:

Index: subr_disk.c
===================================================================
RCS file: /cvsroot/src/sys/kern/subr_disk.c,v
retrieving revision 1.119.2.1
diff -u -p -r1.119.2.1 subr_disk.c
--- subr_disk.c	5 Apr 2019 08:40:19 -0000	1.119.2.1
+++ subr_disk.c	27 Oct 2019 10:20:28 -0000
@@ -364,7 +364,7 @@ bounds_check_with_label(struct disk *dk,
 {
 	struct disklabel *lp = dk->dk_label;
 	struct partition *p = lp->d_partitions + DISKPART(bp->b_dev);
-	uint64_t p_size, p_offset, labelsector;
+	uint64_t p_size, p_offset, labelsector, blocks;
 	int64_t sz;
 
 	if (bp->b_blkno < 0) {
@@ -420,8 +420,13 @@ bounds_check_with_label(struct disk *dk,
 	}
 
 	/* calculate cylinder for disksort to order transfers with */
+	if (lp->d_secsize < DEV_BSIZE)
+		blocks = 1;
+	else
+		blocks = lp->d_secsize / DEV_BSIZE;
+		
 	bp->b_cylinder = (bp->b_blkno + p->p_offset) /
-	    (lp->d_secsize / DEV_BSIZE) / lp->d_secpercyl;
+	    blocks / lp->d_secpercyl;
 	return 1;
 }
 

The practical difference is likely zero, as such setup just do not happen
in real life (and the other error(s) needed to get here being fixed).
I prefer my version because it does not introduce artifical limits on the
sector size - but it is not a very strong technical argument.

What do you think?

Martin