Subject: kern/20975: default cdrom disklabels (partition tables) are incorrect
To: None <gnats-bugs@gnats.netbsd.org>
From: Yorick Hardy <yh@metroweb.co.za>
List: netbsd-bugs
Date: 04/01/2003 21:08:27
>Number:         20975
>Category:       kern
>Synopsis:       default cdrom disklabels (partition tables) are incorrect
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Apr 01 11:10:01 PST 2003
>Closed-Date:
>Last-Modified:
>Originator:     Yorick Hardy
>Release:        NetBSD 1.6_STABLE
>Organization:
Yorick Hardy
>Environment:
System: NetBSD Yorick 1.6_STABLE NetBSD 1.6_STABLE (YORICK) #8: Fri Mar 28 16:02:09 SAST 2003 yorick@Yorick:/usr/src/sys/arch/i386/compile/YORICK i386
Architecture: i386
Machine: i386
>Description:
The default disklabel uses the cdrom sector size, but uses DEV_BSIZE sized
sectors when creating the default partition table. It seems that this
approach was neccessary for the driver to work correctly with the
disklabel function bounds_check_with_label.

(As a physicist would say - check your units :-) )

>How-To-Repeat:
Note "total sectors" and the partition table in the following

$ disklabel /dev/cd0d
# /dev/cd0d:
type: ATAPI
disk: Hewlett-Packard 
label: fictitious
flags: removable
bytes/sector: 2048
sectors/track: 100
tracks/cylinder: 1
sectors/cylinder: 100
cylinders: 1007
total sectors: 100679
rpm: 300
interleave: 1
trackskew: 0
cylinderskew: 0
headswitch: 0		# microseconds
track-to-track seek: 0	# microseconds
drivedata: 0 

4 partitions:
#        size    offset     fstype  [fsize bsize cpg/sgs]
 a:    402716         0    ISO9660                      # (Cyl.    0 - 4027*)
 d:    402716         0    ISO9660                      # (Cyl.    0 - 4027*)
disklabel: boot block size 0
disklabel: super block size 0
disklabel: partition a: partition extends past end of unit
disklabel: partition d: partition extends past end of unit

>Fix:
I submit two patches below,
 1) Quick fix, simply works around the underlying problems
 2) Fix which also corrects some aspects in i386 disklabel functions.

Apologies for not patching against current, but the patches are
still mostly relevant.

First the quick fix, use DEV_BSIZE sized sectors instead of the
underlying cdrom sector size. Patched against rev 1.162 of cd.c.
The second patch follows ...

--- sys/dev/scsipi/cd.c.orig	Tue Apr  1 10:00:46 2003
+++ sys/dev/scsipi/cd.c	Tue Apr  1 11:48:19 2003
@@ -1417,9 +1417,9 @@
 
 	memset(lp, 0, sizeof(struct disklabel));
 
-	lp->d_secsize = cd->params.blksize;
+	lp->d_secsize = DEV_BSIZE;
 	lp->d_ntracks = 1;
-	lp->d_nsectors = 100;
+	lp->d_nsectors = 100 * (cd->params.blksize / DEV_BSIZE);
 	lp->d_ncylinders = (cd->params.disksize / 100) + 1;
 	lp->d_secpercyl = lp->d_ntracks * lp->d_nsectors;
 
@@ -1437,18 +1437,17 @@
 	}
 	strncpy(lp->d_typename, cd->name, 16);
 	strncpy(lp->d_packname, "fictitious", 16);
-	lp->d_secperunit = cd->params.disksize;
+	lp->d_secperunit =
+	    cd->params.disksize * (cd->params.blksize / DEV_BSIZE);
 	lp->d_rpm = 300;
 	lp->d_interleave = 1;
 	lp->d_flags = D_REMOVABLE;
 
 	lp->d_partitions[0].p_offset = 0;
-	lp->d_partitions[0].p_size =
-	    lp->d_secperunit * (lp->d_secsize / DEV_BSIZE);
+	lp->d_partitions[0].p_size = lp->d_secperunit;
 	lp->d_partitions[0].p_fstype = FS_ISO9660;
 	lp->d_partitions[RAW_PART].p_offset = 0;
-	lp->d_partitions[RAW_PART].p_size =
-	    lp->d_secperunit * (lp->d_secsize / DEV_BSIZE);
+	lp->d_partitions[RAW_PART].p_size = lp->d_secperunit;
 	lp->d_partitions[RAW_PART].p_fstype = FS_ISO9660;
 	lp->d_npartitions = RAW_PART + 1;
 

-----------
Now the second (more correct ?) fix. Use the underlying cdrom sector
size. But then bounds_check_with_label in i386 disksubr.c is incorrect
(I have not checked the other platforms), it calculates in terms
of sectors of size DEV_BSIZE.

Please double check this patch, I do not have enough experience to
judge the consequences of these changes.

Patched against rev 1.162 of cd.c, and rev 1.45 of disksubr.c.
-----------

--- sys/dev/scsipi/cd.c.orig	Tue Apr  1 10:00:46 2003
+++ sys/dev/scsipi/cd.c	Tue Apr  1 11:27:15 2003
@@ -1443,12 +1443,10 @@
 	lp->d_flags = D_REMOVABLE;
 
 	lp->d_partitions[0].p_offset = 0;
-	lp->d_partitions[0].p_size =
-	    lp->d_secperunit * (lp->d_secsize / DEV_BSIZE);
+	lp->d_partitions[0].p_size = lp->d_secperunit;
 	lp->d_partitions[0].p_fstype = FS_ISO9660;
 	lp->d_partitions[RAW_PART].p_offset = 0;
-	lp->d_partitions[RAW_PART].p_size =
-	    lp->d_secperunit * (lp->d_secsize / DEV_BSIZE);
+	lp->d_partitions[RAW_PART].p_size = lp->d_secperunit;
 	lp->d_partitions[RAW_PART].p_fstype = FS_ISO9660;
 	lp->d_npartitions = RAW_PART + 1;
 
--- sys/arch/i386/i386/disksubr.c.orig	Tue Apr  1 11:29:21 2003
+++ sys/arch/i386/i386/disksubr.c	Tue Apr  1 11:37:30 2003
@@ -477,11 +477,13 @@
 	struct partition *p = lp->d_partitions + DISKPART(bp->b_dev);
 	int labelsector = lp->d_partitions[2].p_offset + LABELSECTOR;
 	int sz;
+	int blkno;
 
+	blkno = bp->b_blkno / (lp->d_secsize / DEV_BSIZE);
 	sz = howmany(bp->b_bcount, lp->d_secsize);
 
-	if (bp->b_blkno + sz > p->p_size) {
-		sz = p->p_size - bp->b_blkno;
+	if (blkno + sz > p->p_size) {
+		sz = p->p_size - blkno;
 		if (sz == 0) {
 			/* If exactly at end of disk, return EOF. */
 			bp->b_resid = bp->b_bcount;
@@ -497,9 +499,9 @@
 	}
 
 	/* Overwriting disk label? */
-	if (bp->b_blkno + p->p_offset <= labelsector &&
+	if (blkno + p->p_offset <= labelsector &&
 #if LABELSECTOR != 0
-	    bp->b_blkno + p->p_offset + sz > labelsector &&
+	    blkno + p->p_offset + sz > labelsector &&
 #endif
 	    (bp->b_flags & B_READ) == 0 && !wlabel) {
 		bp->b_error = EROFS;
@@ -507,8 +509,7 @@
 	}
 
 	/* calculate cylinder for disksort to order transfers with */
-	bp->b_cylinder = (bp->b_blkno + p->p_offset) /
-	    (lp->d_secsize / DEV_BSIZE) / lp->d_secpercyl;
+	bp->b_cylinder = (blkno + p->p_offset) / lp->d_secpercyl;
 	return (1);
 
 bad:
>Release-Note:
>Audit-Trail:
>Unformatted: