Date: Thu, 9 Aug 2001 01:39:06 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Mitsuru IWASAKI <iwasaki@jp.FreeBSD.org> Cc: <arch@FreeBSD.ORG>, <audit@FreeBSD.ORG> Subject: Re: CFR: Some bug fixes in i386/i386/machdep.c Message-ID: <20010809005326.Y8258-100000@besplex.bde.org> In-Reply-To: <20010808030258E.iwasaki@jp.FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 8 Aug 2001, Mitsuru IWASAKI wrote:
> Hi, I've noticed that there are some bugs in i386/i386/machdep.c.
> With following patches on printing, I got strange result from dmesg.
It sure is dusty in there. In FreeBSD_1.x, the segment limit computations
were all written as `i386_btop(addr) - 1'. This worked in all cases because
`addr' was always a multiple of PAGE_SIZE. Cloning this code for small
segments gave the bugs that you observed.
I think `atop(addr - 1)' is correct. Reasons:
- the segment limit is a maximum page number, not a number of pages, so
rounding down is correct.
- we mostly have addresses, not byte counts, so using i386_btop()
(bytes to pages) is bogus and using atop() (address to pages) is correct.
Using atop() to convert byte counts to pages is bogus, but I suspect
that it is often used. Anyway, we have too many functions to convert
between bytes and pages.
> Also I've found too early warning printing before calling cninit().
> This would never warn to users.
Does it get put into the message buffer that early?
> Following is bug fixes for above problems. Please review it.
> I'll commit this weekend if no objections.
> Index: machdep.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/i386/i386/machdep.c,v
> retrieving revision 1.465
> diff -u -r1.465 machdep.c
> --- machdep.c 2001/07/26 23:06:44 1.465
> +++ machdep.c 2001/08/07 17:41:22
> @@ -1782,6 +1782,7 @@
> struct region_descriptor r_gdt, r_idt;
> #endif
> int off;
> + int metadata_missing = 0;
Some style bugs here. The declarations of ints should be:
int gsel_tss, metadata_missing, off, x;
(except some of these variables shouldn't be ints), and the initializer
should be elsewhere.
> @@ -1808,18 +1809,18 @@
> * XXX text protection is temporarily (?) disabled. The limit was
> * i386_btop(round_page(etext)) - 1.
> */
> - gdt_segs[GCODE_SEL].ssd_limit = i386_btop(0) - 1;
> - gdt_segs[GDATA_SEL].ssd_limit = i386_btop(0) - 1;
> + gdt_segs[GCODE_SEL].ssd_limit = i386_btop(0 - 1);
> + gdt_segs[GDATA_SEL].ssd_limit = i386_btop(0 - 1);
OK.
> #ifdef SMP
> gdt_segs[GPRIV_SEL].ssd_limit =
> - i386_btop(sizeof(struct privatespace)) - 1;
> + i386_btop(sizeof(struct privatespace) + PAGE_SIZE - 1);
I think the " + PAGE_SIZE" term gives an off by one error. Moving the
" - 1" term into the macro arg is sufficient.
> ....
> - i386_btop(sizeof(struct globaldata)) - 1;
> + i386_btop(sizeof(struct globaldata) + PAGE_SIZE - 1);
Similarly.
> @@ -1920,6 +1921,10 @@
> */
> cninit();
>
> + if (metadata_missing) {
> + printf("WARNING: loader(8) metadata is missing!\n");
> + }
> +
Style bug: too many braces.
Grepping for i386_btop in machdep.c shows some related bugs:
/*
* The code segment limit has to cover the user area until we move
* the signal trampoline out of the user area. This is safe because
* the code segment cannot be written to directly.
*/
#define VM_END_USER_R_ADDRESS (VM_END_USER_RW_ADDRESS + UPAGES * PAGE_SIZE)
ldt_segs[LUCODE_SEL].ssd_limit = i386_btop(VM_END_USER_R_ADDRESS) - 1;
ldt_segs[LUDATA_SEL].ssd_limit = i386_btop(VM_END_USER_RW_ADDRESS) - 1;
The " - 1" term should be moved into the macro arg here too. The comment
and the UPAGES term rotted long ago. Peter Wemm seems to be collecting
UPAGES garbage now (he just committed removal of signal trampoline garbage
iin libkern/mcount.c). Perhaps he has already fixed this.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010809005326.Y8258-100000>