>Number:         33636
>Category:       port-mac68k
>Synopsis:       mac68k bus_space_* problems with DIAGNOSTIC
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    port-mac68k-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Jun 02 20:30:00 +0000 2006
>Originator:     Dave Huang
>Release:        NetBSD 3.99.20
>Organization:
Name: Dave Huang         |  Mammal, mammal / their names are called /
INet: khym@azeotrope.org |  they raise a paw / the bat, the cat /
FurryMUCK: Dahan         |  dolphin and dog / koala bear and hog -- TMBG
Dahan: Hani G Y+C 30 Y++ L+++ W- C++ T++ A+ E+ S++ V++ F- Q+++ P+ B+ PA+ PL++
>Environment:
	
	
System: NetBSD yerfable.azeotrope.org 3.99.20 NetBSD 3.99.20 (YERFABLE) #219: Fri Jun 2 12:56:04 CDT 2006 khym@yerfable.azeotrope.org:/usr2/obj.alpha/sys/arch/alpha/compile/YERFABLE alpha
Architecture: alpha
Machine: alpha
>Description:
	It's documented that bus_space(9) routines that take a count
argument have undefined results if count == 0. The mac68k
implementation checks for that case and does nothing if count == 0 for
non-DIAGNOSTIC kernels, and panics on DIAGNOSTIC kernels. Ironically,
due to insufficient parentheses, the DIAGNOSTIC check can sometimes
miss a zero count, and instead of giving a useful panic message, it
causes a kernel bus error trap as the count of 0 is interpreted as
2^32.

E.g.
#define bus_space_set_region_2(t, h, o, val, c) do {                     \
         if (!c) panic("bus_space_set_region_2 called with zero count."); \
         (h).bssr2(t,&(h),o,val,c); } while (0)

Called as
        bus_space_set_region_2(sc->sc_buft, sc->sc_bufh, buf, 0,
             (ETHER_MIN_LEN - ETHER_CRC_LEN - totlen) >> 1);
 
So, that "if" line expands to:
	if (!(ETHER_MIN_LEN - ETHER_CRC_LEN - totlen) >> 1) panic([...]);
 
Assume ETHER_MIN_LEN - ETHER_CRC_LEN - totlen == 1:
	if (!(1) >> 1) panic();
or
	if (0 >> 1) panic(); 	

No panic, even though (ETHER_MIN_LEN - ETHER_CRC_LEN - totlen) >> 1 is 0.

>How-To-Repeat:
	Call bus_space_set_region_2 in a DIAGNOSTIC kernel with a
count argument of "(1) >> 1". Instead of getting a
"bus_space_set_region_2 called with zero count." panic, you get a
kernel trap.

>Fix:
Add parens, and follow KNF's recommendation of, "Don't use `!' for
tests unless it's a boolean."

Index: bus.h
===================================================================
RCS file: /cvsroot/src/sys/arch/mac68k/include/bus.h,v
retrieving revision 1.24
diff -u -r1.24 bus.h
--- bus.h	16 Feb 2006 20:17:14 -0000	1.24
+++ bus.h	2 Jun 2006 18:07:31 -0000
@@ -310,22 +310,22 @@
 
 #if defined(DIAGNOSTIC)
 #define	bus_space_read_multi_1(t, h, o, a, c) do {			 \
-	if (!c) panic("bus_space_read_multi_1 called with zero count."); \
+	if ((c) == 0) panic("bus_space_read_multi_1 called with zero count."); \
 	(h).bsrm1(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_multi_2(t, h, o, a, c) do {			 \
-	if (!c) panic("bus_space_read_multi_2 called with zero count."); \
+	if ((c) == 0) panic("bus_space_read_multi_2 called with zero count."); \
 	(h).bsrm2(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_multi_4(t, h, o, a, c) do {			 \
-	if (!c) panic("bus_space_read_multi_4 called with zero count."); \
+	if ((c) == 0) panic("bus_space_read_multi_4 called with zero count."); \
 	(h).bsrm4(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_multi_stream_1(t, h, o, a, c) do {		 \
-	if (!c) panic("bus_space_read_multi_stream_1 called with count=0."); \
+	if ((c) == 0) panic("bus_space_read_multi_stream_1 called with count=0."); \
 	(h).bsrms1(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_multi_stream_2(t, h, o, a, c) do {		 \
-	if (!c) panic("bus_space_read_multi_stream_2 called with count=0."); \
+	if ((c) == 0) panic("bus_space_read_multi_stream_2 called with count=0."); \
 	(h).bsrms2(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_multi_stream_4(t, h, o, a, c) do {		 \
-	if (!c) panic("bus_space_read_multi_stream_4 called with count=0."); \
+	if ((c) == 0) panic("bus_space_read_multi_stream_4 called with count=0."); \
 	(h).bsrms4(t,&(h),o,a,c); } while (0)
 #else
 #define	bus_space_read_multi_1(t, h, o, a, c) \
@@ -381,22 +381,22 @@
 
 #if defined(DIAGNOSTIC)
 #define	bus_space_read_region_1(t, h, o, a, c) do {			  \
-	if (!c) panic("bus_space_read_region_1 called with zero count."); \
+	if ((c) == 0) panic("bus_space_read_region_1 called with zero count."); \
 	(h).bsrr1(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_region_2(t, h, o, a, c) do {			  \
-	if (!c) panic("bus_space_read_region_2 called with zero count."); \
+	if ((c) == 0) panic("bus_space_read_region_2 called with zero count."); \
 	(h).bsrr2(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_region_4(t, h, o, a, c) do {			  \
-	if (!c) panic("bus_space_read_region_4 called with zero count."); \
+	if ((c) == 0) panic("bus_space_read_region_4 called with zero count."); \
 	(h).bsrr4(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_region_stream_1(t, h, o, a, c) do {		  \
-	if (!c) panic("bus_space_read_region_stream_1 called with count=0."); \
+	if ((c) == 0) panic("bus_space_read_region_stream_1 called with count=0."); \
 	(h).bsrrs1(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_region_stream_2(t, h, o, a, c) do {		  \
-	if (!c) panic("bus_space_read_region_stream_2 called with count=0."); \
+	if ((c) == 0) panic("bus_space_read_region_stream_2 called with count=0."); \
 	(h).bsrrs2(t,&(h),o,a,c); } while (0)
 #define	bus_space_read_region_stream_4(t, h, o, a, c) do {		  \
-	if (!c) panic("bus_space_read_region_stream_4 called with count=0."); \
+	if ((c) == 0) panic("bus_space_read_region_stream_4 called with count=0."); \
 	(h).bsrrs4(t,&(h),o,a,c); } while (0)
 #else
 #define	bus_space_read_region_1(t, h, o, a, c) \
@@ -490,22 +490,22 @@
 
 #if defined(DIAGNOSTIC)
 #define	bus_space_write_multi_1(t, h, o, a, c) do {			  \
-	if (!c) panic("bus_space_write_multi_1 called with zero count."); \
+	if ((c) == 0) panic("bus_space_write_multi_1 called with zero count."); \
 	(h).bswm1(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_multi_2(t, h, o, a, c) do {			  \
-	if (!c) panic("bus_space_write_multi_2 called with zero count."); \
+	if ((c) == 0) panic("bus_space_write_multi_2 called with zero count."); \
 	(h).bswm2(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_multi_4(t, h, o, a, c) do {			  \
-	if (!c) panic("bus_space_write_multi_4 called with zero count."); \
+	if ((c) == 0) panic("bus_space_write_multi_4 called with zero count."); \
 	(h).bswm4(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_multi_stream_1(t, h, o, a, c) do {		  \
-	if (!c) panic("bus_space_write_multi_stream_1 called with count=0."); \
+	if ((c) == 0) panic("bus_space_write_multi_stream_1 called with count=0."); \
 	(h).bswms1(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_multi_stream_2(t, h, o, a, c) do {		  \
-	if (!c) panic("bus_space_write_multi_stream_2 called with count=0."); \
+	if ((c) == 0) panic("bus_space_write_multi_stream_2 called with count=0."); \
 	(h).bswms2(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_multi_stream_4(t, h, o, a, c) do {		  \
-	if (!c) panic("bus_space_write_multi_stream_4 called with count=0."); \
+	if ((c) == 0) panic("bus_space_write_multi_stream_4 called with count=0."); \
 	(h).bswms4(t,&(h),o,a,c); } while (0)
 #else
 #define	bus_space_write_multi_1(t, h, o, a, c) \
@@ -561,22 +561,22 @@
 
 #if defined(DIAGNOSTIC)
 #define	bus_space_write_region_1(t, h, o, a, c) do {			   \
-	if (!c) panic("bus_space_write_region_1 called with zero count."); \
+	if ((c) == 0) panic("bus_space_write_region_1 called with zero count."); \
 	(h).bswr1(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_region_2(t, h, o, a, c) do {			   \
-	if (!c) panic("bus_space_write_region_2 called with zero count."); \
+	if ((c) == 0) panic("bus_space_write_region_2 called with zero count."); \
 	(h).bswr2(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_region_4(t, h, o, a, c) do {			   \
-	if (!c) panic("bus_space_write_region_4 called with zero count."); \
+	if ((c) == 0) panic("bus_space_write_region_4 called with zero count."); \
 	(h).bswr4(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_region_stream_1(t, h, o, a, c) do {		   \
-	if (!c) panic("bus_space_write_region_stream_1 called with count=0."); \
+	if ((c) == 0) panic("bus_space_write_region_stream_1 called with count=0."); \
 	(h).bswrs1(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_region_stream_2(t, h, o, a, c) do {		   \
-	if (!c) panic("bus_space_write_region_stream_2 called with count=0."); \
+	if ((c) == 0) panic("bus_space_write_region_stream_2 called with count=0."); \
 	(h).bswrs2(t,&(h),o,a,c); } while (0)
 #define	bus_space_write_region_stream_4(t, h, o, a, c) do {		   \
-	if (!c) panic("bus_space_write_region_stream_4 called with count=0."); \
+	if ((c) == 0) panic("bus_space_write_region_stream_4 called with count=0."); \
 	(h).bswrs4(t,&(h),o,a,c); } while (0)
 #else
 #define	bus_space_write_region_1(t, h, o, a, c) \
@@ -628,13 +628,13 @@
 
 #if defined(DIAGNOSTIC)
 #define	bus_space_set_multi_1(t, h, o, val, c) do {			\
-	if (!c) panic("bus_space_set_multi_1 called with zero count."); \
+	if ((c) == 0) panic("bus_space_set_multi_1 called with zero count."); \
 	(h).bssm1(t,&(h),o,val,c); } while (0)
 #define	bus_space_set_multi_2(t, h, o, val, c) do {			\
-	if (!c) panic("bus_space_set_multi_2 called with zero count."); \
+	if ((c) == 0) panic("bus_space_set_multi_2 called with zero count."); \
 	(h).bssm2(t,&(h),o,val,c); } while (0)
 #define	bus_space_set_multi_4(t, h, o, val, c) do {			\
-	if (!c) panic("bus_space_set_multi_4 called with zero count."); \
+	if ((c) == 0) panic("bus_space_set_multi_4 called with zero count."); \
 	(h).bssm4(t,&(h),o,val,c); } while (0)
 #else
 #define	bus_space_set_multi_1(t, h, o, val, c) \
@@ -678,13 +678,13 @@
 
 #if defined(DIAGNOSTIC)
 #define	bus_space_set_region_1(t, h, o, val, c) do {			 \
-	if (!c) panic("bus_space_set_region_1 called with zero count."); \
+	if ((c) == 0) panic("bus_space_set_region_1 called with zero count."); \
 	(h).bssr1(t,&(h),o,val,c); } while (0)
 #define	bus_space_set_region_2(t, h, o, val, c) do {			 \
-	if (!c) panic("bus_space_set_region_2 called with zero count."); \
+	if ((c) == 0) panic("bus_space_set_region_2 called with zero count."); \
 	(h).bssr2(t,&(h),o,val,c); } while (0)
 #define	bus_space_set_region_4(t, h, o, val, c) do {			 \
-	if (!c) panic("bus_space_set_region_4 called with zero count."); \
+	if ((c) == 0) panic("bus_space_set_region_4 called with zero count."); \
 	(h).bssr4(t,&(h),o,val,c); } while (0)
 #else
 #define	bus_space_set_region_1(t, h, o, val, c) \

>Unformatted: