Path: utzoo!utgpu!jarvis.csri.toronto.edu!mailrus!cs.utexas.edu!uunet!munnari.oz.au!csc!ccadfa!usage!basser!metro!extro!natmlab!ditsyda!evans
From: evans@ditsyda.oz (Bruce Evans)
Newsgroups: comp.os.minix
Subject: Re: bug causing panic in kernel (ST)
Keywords: minix-st kernel_panic umap
Message-ID: <2254@ditsyda.oz>
Date: 9 Oct 89 13:36:18 GMT
References: <280@nikhefh.nikhef.nl>
Reply-To: evans@ditsyda.mq.oz (Bruce Evans)
Organization: CSIRO DIT Sydney, Australia
Lines: 92

In article <280@nikhefh.nikhef.nl> n62@nikhefh.nikhef.nl (Klamer Schutte) writes:
>This is quite st specific because PC segments start at address 0.

Not at all. Your test program exposed a bug in my version of TTY. The test

	if (write( 2, 8000000L, 13 ) < 0)

generates an error as in lines 3927-3930 of the book. The error case is
allowed to fall through the general case and only return an error code
much later. I missed checking parts of this labyrinth and did not reply
to TTY for the error case (much like the old driver does not reply
properly after an XOFF suspends output, so FS remains waiting on TTY).

This was with a 32-bit 386 Minix so the 80000000L is sort of OK. Did you
really mean 0x80000000L? That may expose other bugs.

>The patch is not entirely clean: It is still possible to write in your own
>text segment. But repairing this will need a lot of changes, all through the

For PC-Minix common I&D programs, and I think all ST-Minix programs, the
text segment is empty from the umap's point of view (== data seg from
another). Protected mode PC-Minix programs have a read-only text segment.
There is no choice - it can even be execute-only, but the protection is
completely lost with common I&D programs since text seg == data seg
physically. So don't use common I&D on the PC!

Making the text segment read-only in system calls is hardly worthwhile if
crazy programs can still corrupt their text segments and elsewhere using
ordinary pointers.

>One more bug spotted: The value of errno is not consistent. It should in both
>cases (write & fstat) return EFAULT (== -14 on my system). Anyone voluntering

The failed writes return E_BAD_ADDR (-10) which is identical with some other
standard error code and very confusing when printed by perror :-(.

---

Here's how I think umap() should be done. I was thinking about how to make
it faster by avoiding conversions between clicks and bytes. It takes 100+
instructions on a PC and is called 3+ times for every block of i/o, so I
think it is the worst (concentrated) bottleneck in the kernel.

Everything becomes simpler, so the bug found be Klamer is easy to avoid.
Memory *allocation* must be done in clicks, but the rest of the code is
better without them.

Caveat. The code has not been tested.

/* The mem_map structure (or part of it in the kernel) needs 3 new fields:
 *
 *	adjust:		offset to convert virtual address to physical
 *			could be made optional when it is always 0
 *	base:		minimum physical address in segment
 *	limit:		maximum physical address in segment
 *
 * Having these makes the calculations cleaner.  Magic click conversions and
 * the (lack of) distinction between data and stack can be hidden in the
 * initialization code.
 *
 * Phys_bytes and vir_bytes ought to unsigned long instead of long.  Assume
 * that here.  Watch out for overflow, however.  See your local malloc for
 * how not to check for overflow :-).
 */

PUBLIC phys_bytes umap(rp, seg, vir_addr, bytes)
register struct proc *rp;	/* pointer to proc table entry for process */
int seg;			/* T, D, or S segment */
vir_bytes vir_addr;		/* virtual address in bytes within the seg */
vir_bytes bytes;		/* # of bytes to be copied */
{
/* Calculate the physical memory address for a given virtual address. */

  phys_bytes high_phys;
  phys_bytes low_phys;

  low_phys = rp->p_map[seg].adjust + vir_addr;	/* return at once if trusted?*/
  high_phys = low_phys + bytes;

  /* Check base <= low < high < limit.  Preposterous (zero, huge or negative)
   * bytes will not pass, though 0 probably should be allowed.
   */
  if (low_phys < rp->p_map[seg].base || low_phys >= high_phys ||
      high_phys >= rp->p_map[seg].limit)
	return(0);
  return(low_phys);
}

The whole routine could be written in 1 return statement with no local
variables, but the instructions on the Cereal box say not to do that :-).
-- 
Bruce Evans		evans@ditsyda.oz.au