Date: Fri, 3 Dec 2010 20:45:12 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Cran <brucec@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: 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... Message-ID: <20101203201705.O2228@besplex.bde.org> In-Reply-To: <201012022219.oB2MJUx5031472@svn.freebsd.org> References: <201012022219.oB2MJUx5031472@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101203201705.O2228>