svn commit: r216134 - in head: share/man/man9 sys/amd64/include sys/arm/include sys/i386/include sys/ia64/include sys/mips/include sys/pc98/include sys/powerpc/include sys/sparc64/include sys/sun4v...

[Bruce Evans]

On Thu, 2 Dec 2010, Bruce Cran wrote:

> Log:
>  Disallow passing in a count of zero bytes to the bus_space(9) functions.
>
>  Passing a count of zero on i386 and amd64 for [I386|AMD64]_BUS_SPACE_MEM
>  causes a crash/hang since the 'loop' instruction decrements the counter
>  before checking if it's zero.
>
>  PR:	kern/80980
>  Discussed with:	jhb
> ...
> Modified: head/sys/amd64/include/bus.h
> ==============================================================================
> --- head/sys/amd64/include/bus.h	Thu Dec  2 22:00:57 2010	(r216133)
> +++ head/sys/amd64/include/bus.h	Thu Dec  2 22:19:30 2010	(r216134)
> @@ -104,6 +104,9 @@
> #ifndef _AMD64_BUS_H_
> #define _AMD64_BUS_H_
>
> +#include <sys/param.h>
> +#include <sys/systm.h>
> +

This is massive namespace pollution.

Most  kernel .c files should include these first, and most already do.
(Ones that try to be smart and only include <sys/types.h> instead of
<sys/param.h>, or <sys/param.h> without <sys/systm.h>, or include
<sys/systm.h> after other headers, may already be broken, since
KASSERT() is declared in <sys/systm.h>, but it may be used in other
header (like this one now).  KASSERT() should probably be declared in
<sys/param.h> or even in <sys/cdefs.h>.  That gives more pollution there
but less overall.)

> #include <machine/_bus.h>
> #include <machine/cpufunc.h>

Including <machine/_bus.h> is correct (_bus.h exist to avoid namespace
pollution that is about 1000 times smaller than now here), but including
<machine/cpufunc.h> is older namespace pollution/historical mislayering
(we only need i/o functions from cpufunc.h, and they should be declared
here directly).  Now it has no effect, since <machine/cpufunc.h> is
standard namespace pollution in <sys/systm.h>.

>
> @@ -268,7 +271,7 @@ static __inline void
> bus_space_read_multi_1(bus_space_tag_t tag, bus_space_handle_t bsh,
> 		       bus_size_t offset, u_int8_t *addr, size_t count)
> {
> -
> +	KASSERT(count != 0, ("%s: count == 0", __func__));
> 	if (tag == AMD64_BUS_SPACE_IO)
> 		insb(bsh + offset, addr, count);
> 	else {

KASSERT() in little inline functions gives a lot of bloat for such an
unlikely error.  Stupid callers can still pass any garbage count except 0.

The function name of a leaf function is not very interesting.  In some
of the other bus.h's, the caller's name is available since the interface
is a macro, but there __func__ (which should only be used in macros)
is not used, apparently since it would give a name that is useful but
inconsistent with arches that don't use a macro.

Bruce