Bug 15956

Summary: Off-by-1 error in diskstrategy()
Product: Base System Reporter: peter.jeremy <peter.jeremy>
Component: kernAssignee: freebsd-bugs (Nobody) <bugs>
Status: Closed FIXED    
Severity: Affects Only Me    
Priority: Normal    
Version: 4.0-CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
file.diff
none
file.diff none

Description peter.jeremy 2000-01-06 23:00:01 UTC 
	The I/O request range check in /sys/kern/subr_disk.c does not
	correctly handle an EOF return from dscheck() - instead of
	terminating the I/O (returning EOF to the caller), it passes a
	zero-length request to the underlying d_strategy() - which
	at least for the ATA device can't handle it.

Fix: 1) Don't pass zero-length requests to the underlying device:
2) Add belt-and-braces checks to ATA device (this code compiles
	   and links, but I haven't booted from the resultant kernel):
How-To-Repeat: 
	# dd if=/dev/rad0c of=/dev/null bs=128k
	will report an error at the end of the slice (even if the slice
	is a multiple of 128k).  The problem does not occur with the
	old wd driver (and it shouldn't appear with the wfd, ida or vn
	drivers), but appears to affect all other disk devices.
Comment 1 Poul-Henning Kamp 2000-01-07 11:57:35 UTC 
In message <00Jan7.095115est.40327@border.alcanet.com.au>, peter.jeremy@alcatel
.com.au writes:

>Index: subr_disk.c
>===================================================================
>RCS file: /home/CVSROOT/src/sys/kern/subr_disk.c,v
>retrieving revision 1.16
>diff -u -r1.16 subr_disk.c
>--- subr_disk.c	1999/12/19 12:36:41	1.16
>+++ subr_disk.c	2000/01/06 21:39:34
>@@ -193,7 +193,7 @@
> 		return;
> 	}
> 
>-	if (dscheck(bp, dp->d_slice) < 0) {
>+	if (dscheck(bp, dp->d_slice) <= 0) {
> 		biodone(bp);
> 		return;
> 	}
>
>	2) Add belt-and-braces checks to ATA device (this code compiles
>	   and links, but I haven't booted from the resultant kernel):

This was actually done that way deliberately, but not for any
specific reason.

Input welcome...

--
Poul-Henning Kamp             FreeBSD coreteam member
phk@FreeBSD.ORG               "Real hackers run -current on their laptop."
FreeBSD -- It will take a long time before progress goes too far!
Comment 2 peter.jeremy 2000-01-09 23:39:40 UTC 
On 2000-Jan-07 22:57:11 +1100, Poul-Henning Kamp <phk@critter.freebsd.dk> wrote:
>>-	if (dscheck(bp, dp->d_slice) < 0) {
>>+	if (dscheck(bp, dp->d_slice) <= 0) {
...
>This was actually done that way deliberately, but not for any
>specific reason.

I wasn't sure about the reasons.  What I did notice was that the other
references to dscheck() all do a <= check:

/sys/dev/ida/ida_disk.c:205:        if (dscheck(bp, drv->slices) <= 0)
/sys/dev/vn/vn.c:305:               if (vn->sc_slices != NULL && dscheck(bp, vn->sc_slices) <= 0) {
/sys/i386/isa/wd.c:581:     if (dscheck(bp, du->dk_slices) <= 0)
/sys/i386/isa/wfd.c:419:    if (dscheck(bp, t->dk_slices) <= 0) {
/sys/pc98/pc98/wd.c:675:    if (dscheck(bp, du->dk_slices) <= 0)

Also, I can't see any point in passing zero-length I/O requests into
the low level drivers - bouncing them back out as quick as possible
will (if anything) improve performance.

I notice Soren has since added a (correct, unlike mine) patch to
the ata drivers to catch zero-length requests.

Peter
Comment 3 Poul-Henning Kamp freebsd_committer freebsd_triage 2000-01-10 12:21:47 UTC 
State Changed
From-To: open->closed