interrupts could be recursive since lapic_eoi() called before rti

so fast interrupts overflow the kernel stack
fix: cli() before lapic_eoi()

diff --git a/Notes b/Notes
index 1140f9ded5742d1411e431d8ac2c9145dbbfea9b..3efafb32f6fe49f73f649f61e1f50fe4a5a78331 100644 (file)
--- a/Notes
+++ b/Notes
@@ -279,3 +279,78 @@ BUT now userfs doesn't do the final cat README
 
 AND w/ cprintf("kbd overflow"), panic holding locks in scheduler
   maybe also simulataneous panic("interrupt while holding a lock")
+
+again (holding down x key):
+  kbd overflow
+  kbd oaaniicloowh
+  olding locks in scheduler
+  trap v 33 eip 100F5F c^CNext at t=32166285
+  (0) [0x0010033e] 0008:0010033e (unk. ctxt): jmp .+0xfffffffe (0x0010033e) ; ebfe
+  (1) [0x0010005c] 0008:0010005c (unk. ctxt): jmp .+0xfffffffe (0x0010005c) ; ebfe
+cpu0 paniced due to holding locks in scheduler
+cpu1 got panic("interrupt while holding a lock")
+  again in lapic_write.
+  while re-enabling an IRQ?
+
+again:
+cpu 0 panic("holding locks in scheduler")
+  but didn't trigger related panics earlier in scheduler or sched()
+  of course the panic is right after release() and thus sti()
+  so we may be seeing an interrupt that left locks held
+cpu 1 unknown panic
+why does it happen to both cpus at the same time?
+
+again:
+cpu 0 panic("holding locks in scheduler")
+  but trap() didn't see any held locks on return
+cpu 1 no apparent panic
+
+again:
+cpu 0 panic: holding too many locks in scheduler
+cpu 1 panic: kbd_intr returned while holding a lock
+
+again:
+cpu 0 panic: holding too man
+  la 10d70c lr 10027b
+  those don't seem to be locks...
+  only place non-constant lock is used is sleep()'s 2nd arg
+  maybe register not preserved across context switch?
+  it's in %esi...
+  sched() doesn't touch %esi
+  %esi is evidently callee-saved
+  something to do with interrupts? since ordinarily it works
+cpu 1 panic: kbd_int returned while holding a lock
+  la 107340 lr 107300
+  console_lock and kbd_lock
+
+maybe console_lock is often not released due to change
+  in use_console_lock (panic on other cpu)
+
+again:
+cpu 0: panic: h...
+  la 10D78C lr 102CA0
+cpu 1: panic: acquire FL_IF (later than cpu 0)
+
+but if sleep() were acquiring random locks, we'd see panics
+in release, after sleep() returned.
+actually when system is idle, maybe no-one sleeps at all.
+  just scheduler() and interrupts
+
+questions:
+  does userfs use pipes? or fork?
+    no
+  does anything bad happen if process 1 exits? eg exit() in cat.c
+    looks ok
+  are there really no processes left?
+  lock_init() so we can have a magic number?
+
+HMM maybe the variables at the end of struct cpu are being overwritten
+  nlocks, lastacquire, lastrelease
+  by cpu->stack?
+  adding junk buffers maybe causes crash to take longer...
+  when do we run on cpu stack?
+  just in scheduler()?
+    and interrupts from scheduler()
+ 
+OH! recursive interrupts will use up any amount of cpu[].stack!
+  underflow and wrecks *previous* cpu's struct

diff --git a/bio.c b/bio.c
index f847c2e2636888555ecf81a2972ddcd529d2f4a9..2b17a52167a0022d40694b2a37ac670271641877 100644 (file)
--- a/bio.c
+++ b/bio.c
@@ -8,7 +8,13 @@
 #include "buf.h"
 
 struct buf buf[NBUF];
-struct spinlock buf_table_lock = { "buf_table" };
+struct spinlock buf_table_lock;
+
+void
+binit(void)
+{
+  initlock(&buf_table_lock, "buf_table");
+}
 
 struct buf *
 getblk()

diff --git a/console.c b/console.c
index 726aa0a4020d62057b50e9df9fa5f0d734974059..5fc192038fac427918baa1ad5d00b964257cb508 100644 (file)
--- a/console.c
+++ b/console.c
@@ -3,11 +3,16 @@
 #include "defs.h"
 #include "spinlock.h"
 #include "dev.h"
+#include "param.h"
 
-struct spinlock console_lock = { "console" };
+struct spinlock console_lock;
 int panicked = 0;
 int use_console_lock = 0;
 
+// per-cpu copy of output to help panic/lock debugging
+char obuf[NCPU][1024];
+uint obufi[NCPU];
+
 /*
  * copy console output to parallel port, which you can tell
  * .bochsrc to copy to the stdout:
@@ -32,6 +37,10 @@ cons_putc(int c)
   ushort *crt = (ushort *) 0xB8000; // base of CGA memory
   int ind;
 
+  obuf[rcr4()][obufi[rcr4()]++] = c;
+  if(obufi[rcr4()] >= 1024)
+    obufi[rcr4()] = 0;
+
   if(panicked){
     cli();
     for(;;)
@@ -101,11 +110,13 @@ printint(int xx, int base, int sgn)
 void
 cprintf(char *fmt, ...)
 {
-  int i, state = 0, c;
+  int i, state = 0, c, locking = 0;
   uint *ap = (uint *)(void*)&fmt + 1;
 
-  if(use_console_lock)
+  if(use_console_lock){
+    locking = 1;
     acquire(&console_lock);
+  }
 
   for(i = 0; fmt[i]; i++){
     c = fmt[i] & 0xff;
@@ -140,7 +151,7 @@ cprintf(char *fmt, ...)
     }
   }
 
-  if(use_console_lock)
+  if(locking)
     release(&console_lock);
 }
 
@@ -293,7 +304,7 @@ static uchar *charcode[4] = {
 char kbd_buf[KBD_BUF];
 int kbd_r;
 int kbd_w;
-struct spinlock kbd_lock = { "kbd_lock" };
+struct spinlock kbd_lock;
 
 void
 kbd_intr()
@@ -303,20 +314,17 @@ kbd_intr()
 
   st = inb(KBSTATP);
   if ((st & KBS_DIB) == 0){
-    lapic_eoi();
     return;
   }
   data = inb(KBDATAP);
 
   if (data == 0xE0) {
     shift |= E0ESC;
-    lapic_eoi();
     return;
   } else if (data & 0x80) {
     // Key released
     data = (shift & E0ESC ? data : data & 0x7F);
     shift &= ~(shiftcode[data] | E0ESC);
-    lapic_eoi();
     return;
   } else if (shift & E0ESC) {
     // Last character was an E0 escape; or with 0x80
@@ -346,14 +354,17 @@ kbd_intr()
   }
 
   release(&kbd_lock);
-
-  lapic_eoi();
 }
 
 void
 console_init()
 {
+  initlock(&console_lock, "console");
+  initlock(&kbd_lock, "kbd");
+
   devsw[CONSOLE].d_write = console_write;
 
   ioapic_enable (IRQ_KBD, 1);
+
+  use_console_lock = 1;
 }

diff --git a/defs.h b/defs.h
index de114014cd0bd51db3f04cb5dc2bae056c4ae7fc..0b94488871fd13420bb2d2ca7866f588fab6323c 100644 (file)
--- a/defs.h
+++ b/defs.h
@@ -10,6 +10,7 @@ void panic(char *s);
 void kbd_intr(void);
 
 // proc.c
+void pinit(void);
 struct proc;
 struct jmpbuf;
 void setupsegs(struct proc *);
@@ -67,6 +68,7 @@ void ioapic_enable (int irq, int cpu);
 
 // spinlock.c
 struct spinlock;
+void initlock(struct spinlock *, char *);
 void acquire(struct spinlock*);
 void release(struct spinlock*);
 int holding(struct spinlock*);
@@ -83,6 +85,7 @@ int pipe_write(struct pipe *p, char *addr, int n);
 int pipe_read(struct pipe *p, char *addr, int n);
 
 // fd.c
+void fd_init(void);
 int fd_ualloc(void);
 struct fd * fd_alloc(void);
 void fd_close(struct fd *);
@@ -97,6 +100,7 @@ void* ide_start_rw(int diskno, uint secno, void *dst, uint nsecs, int read);
 int ide_finish(void *);
 
 // bio.c
+void binit(void);
 struct buf;
 struct buf *getblk(void);
 struct buf *bread(uint, uint);
@@ -104,6 +108,7 @@ void bwrite(uint, struct buf *, uint);
 void brelse(struct buf *);
 
 // fs.c
+void iinit(void);
 struct inode * iget(uint dev, uint inum);
 void ilock(struct inode *ip);
 void iunlock(struct inode *ip);

diff --git a/fd.c b/fd.c
index 983497fb7d34e8f0470143439edf50d9b0220b5d..d162813109ee7db21320504e0c3d06ce021b9816 100644 (file)
--- a/fd.c
+++ b/fd.c
@@ -13,6 +13,12 @@ struct devsw devsw[NDEV];
 
 struct fd fds[NFD];
 
+void
+fd_init(void)
+{
+  initlock(&fd_table_lock, "fd_table");
+}
+
 /*
  * allocate a file descriptor number for curproc.
  */

diff --git a/fs.c b/fs.c
index 126c18fe1f3a41f6dd2108ce2c391b2aa9827701..b298d7fc2145f5409d04d00b11078846e1e554b7 100644 (file)
--- a/fs.c
+++ b/fs.c
@@ -13,10 +13,16 @@
 // these are inodes currently in use
 // an entry is free if count == 0
 struct inode inode[NINODE];
-struct spinlock inode_table_lock = { "inode_table" };
+struct spinlock inode_table_lock;
 
 uint rootdev = 1;
 
+void
+iinit(void)
+{
+  initlock(&inode_table_lock, "inode_table");
+}
+
 static uint 
 balloc(uint dev) 
 {

diff --git a/ide.c b/ide.c
index 67fb613d91db8996b497e5fa1628d213b08f2414..353212115771c1d5e7aaa0c7a848c1bb67b41a91 100644 (file)
--- a/ide.c
+++ b/ide.c
@@ -26,7 +26,7 @@ struct ide_request {
 };
 struct ide_request request[NREQUEST];
 int head, tail;
-struct spinlock ide_lock = { "ide" };
+struct spinlock ide_lock;
 
 int disk_channel;
 
@@ -46,6 +46,7 @@ ide_wait_ready(int check_error)
 void
 ide_init(void)
 {
+  initlock(&ide_lock, "ide");
   if (ncpu < 2) {
     panic ("ide_init: disk interrupt is going to the second  cpu\n");
   }
@@ -61,7 +62,6 @@ ide_intr(void)
   //  cprintf("cpu%d: ide_intr\n", cpu());
   wakeup(&request[tail]);
   release(&ide_lock);
-  lapic_eoi();
 }
 
 int

diff --git a/kalloc.c b/kalloc.c
index c13a639dc360cf51f51c5af40b774b1471979629..989e3e8a9f97a02e25205647454593f663d2c770 100644 (file)
--- a/kalloc.c
+++ b/kalloc.c
@@ -15,7 +15,7 @@
 #include "proc.h"
 #include "spinlock.h"
 
-struct spinlock kalloc_lock = { "kalloc" };
+struct spinlock kalloc_lock;
 
 struct run {
   struct run *next;
@@ -37,6 +37,7 @@ kinit(void)
   uint mem;
   char *start;
   
+  initlock(&kalloc_lock, "kalloc");
   start = (char *) &end;
   start = (char *) (((uint)start + PAGE) & ~(PAGE-1));
   mem = 256; // XXX

diff --git a/main.c b/main.c
index b558e41ad9499afc12d6e20d0b28c2ca8d75ed14..a7209b61da5232a4599cda3157fce19dc948f5eb 100644 (file)
--- a/main.c
+++ b/main.c
@@ -15,8 +15,6 @@ extern uchar _binary_user1_start[], _binary_user1_size[];
 extern uchar _binary_usertests_start[], _binary_usertests_size[];
 extern uchar _binary_userfs_start[], _binary_userfs_size[];
 
-extern int use_console_lock;
-
 // CPU 0 starts running C code here.
 // This is called main0 not main so that it can have
 // a void return type.  Gcc can't handle functions named
@@ -27,28 +25,36 @@ main0(void)
   int i;
   struct proc *p;
 
+  lcr4(0); // xxx copy of cpu #
+
   // clear BSS
   memset(edata, 0, end - edata);
 
   // Make sure interrupts stay disabled on all processors
   // until each signals it is ready, by pretending to hold
   // an extra lock.
-  for(i=0; i<NCPU; i++)
+  // xxx maybe replace w/ acquire remembering if FL_IF
+  for(i=0; i<NCPU; i++){
     cpus[i].nlock++;
+    cpus[i].guard1 = 0xdeadbeef;
+    cpus[i].guard2 = 0xdeadbeef;
+  }
 
   mp_init(); // collect info about this machine
 
-  use_console_lock = 1;
-
   lapic_init(mp_bcpu());
 
   cprintf("\n\ncpu%d: booting xv6\n\n", cpu());
 
+  pinit();
+  binit();
   pic_init(); // initialize PIC
   ioapic_init();
   kinit(); // physical memory allocator
   tvinit(); // trap vectors
   idtinit(); // this CPU's idt register
+  fd_init();
+  iinit();
 
   // create a fake process per CPU
   // so each CPU always has a tss and a gdt
@@ -101,6 +107,8 @@ main0(void)
 void
 mpmain(void)
 {
+  lcr4(1); // xxx copy of cpu #
+
   cprintf("cpu%d: starting\n", cpu());
   idtinit(); // CPU's idt
   if(cpu() == 0)

diff --git a/mmu.h b/mmu.h
index 82fb89db54b5849873f95796739c3ef53a158621..0cd7944dc5a70b5807559097cd7fdeea9be9cf8f 100644 (file)
--- a/mmu.h
+++ b/mmu.h
@@ -178,6 +178,7 @@ struct gatedesc {
 
 // Set up a normal interrupt/trap gate descriptor.
 // - istrap: 1 for a trap (= exception) gate, 0 for an interrupt gate.
+//   interrupt gate clears FL_IF, trap gate leaves FL_IF alone
 // - sel: Code segment selector for interrupt/trap handler
 // - off: Offset in code segment for interrupt/trap handler
 // - dpl: Descriptor Privilege Level -

diff --git a/pipe.c b/pipe.c
index c8da428822aae306fe8039660a91c1b0f112d5b5..6f8081075f214f9afdb5b8b595d04b68ab93d376 100644 (file)
--- a/pipe.c
+++ b/pipe.c
@@ -34,7 +34,7 @@ pipe_alloc(struct fd **fd1, struct fd **fd2)
   p->writeopen = 1;
   p->writep = 0;
   p->readp = 0;
-  memset(&p->lock, 0, sizeof(p->lock));
+  initlock(&p->lock, "pipe");
   (*fd1)->type = FD_PIPE;
   (*fd1)->readable = 1;
   (*fd1)->writeable = 0;

diff --git a/proc.c b/proc.c
index 0254c2ec22f490dcf3034e0d9a354dd0ecc312f7..5f8769b930d1feb0444363a50982af84675caae5 100644 (file)
--- a/proc.c
+++ b/proc.c
@@ -7,7 +7,7 @@
 #include "defs.h"
 #include "spinlock.h"
 
-struct spinlock proc_table_lock = { "proc_table" };
+struct spinlock proc_table_lock;
 
 struct proc proc[NPROC];
 struct proc *curproc[NCPU];
@@ -15,6 +15,12 @@ int next_pid = NCPU;
 extern void forkret(void);
 extern void forkret1(struct trapframe*);
 
+void
+pinit(void)
+{
+  initlock(&proc_table_lock, "proc_table");
+}
+
 /*
  * set up a process's task state and segment descriptors
  * correctly, given its current size and address in memory.
@@ -146,6 +152,9 @@ scheduler(void)
     // Loop over process table looking for process to run.
     acquire(&proc_table_lock);
     for(i = 0; i < NPROC; i++){
+      if(cpus[cpu()].guard1 != 0xdeadbeef || 
+         cpus[cpu()].guard2 != 0xdeadbeef)
+        panic("cpu guard");
       p = &proc[i];
       if(p->state != RUNNABLE)
         continue;
@@ -194,6 +203,7 @@ scheduler(void)
 
       // XXX if not holding proc_table_lock panic.
     }
+
     release(&proc_table_lock);
     
     if(cpus[cpu()].nlock != 0)
@@ -212,7 +222,9 @@ scheduler(void)
 void
 sched(void)
 {
-  if(setjmp(&curproc[cpu()]->jmpbuf) == 0)
+  struct proc *p = curproc[cpu()];
+
+  if(setjmp(&p->jmpbuf) == 0)
     longjmp(&cpus[cpu()].jmpbuf);
 }
 

diff --git a/proc.h b/proc.h
index c6f5be67f983f352cfe90d909e15ba2d276cab8e..6ed2e78c12331fb648339fd0c2a85394fbf92a63 100644 (file)
--- a/proc.h
+++ b/proc.h
@@ -41,8 +41,6 @@ struct proc{
   uint sz; // total size of mem, including kernel stack
   char *kstack; // kernel stack, separate from mem so it doesn't move
   enum proc_state state;
-  enum proc_state newstate; // desired state after swtch()
-  struct spinlock *mtx; // mutex for condition variable
   int pid;
   int ppid;
   void *chan; // sleep
@@ -68,7 +66,9 @@ extern struct proc *curproc[NCPU];  // can be NULL if no proc running.
 struct cpu {
   uchar apicid;       // Local APIC ID
   struct jmpbuf jmpbuf;
+  int guard1;
   char mpstack[MPSTACK]; // per-cpu start-up stack
+  int guard2;
   volatile int booted;
   int nlock; // # of locks currently held
   struct spinlock *lastacquire; // xxx debug

diff --git a/spinlock.c b/spinlock.c
index b1b4079d19ec3a1c5f8f226c4a26e345a5a95649..663fe33ed16d724f5c08155887870e657bd3d092 100644 (file)
--- a/spinlock.c
+++ b/spinlock.c
@@ -10,8 +10,19 @@
 // because cprintf uses them itself.
 //#define cprintf dont_use_cprintf
 
+#define LOCKMAGIC 0x6673ffea
+
 extern int use_console_lock;
 
+void
+initlock(struct spinlock *lock, char *name)
+{
+  lock->magic = LOCKMAGIC;
+  lock->name = name;
+  lock->locked = 0;
+  lock->cpu = 0xffffffff;
+}
+
 void
 getcallerpcs(void *v, uint pcs[])
 {
@@ -27,6 +38,8 @@ getcallerpcs(void *v, uint pcs[])
 void
 acquire(struct spinlock * lock)
 {
+  if(lock->magic != LOCKMAGIC)
+    panic("weird lock magic");
   if(holding(lock))
     panic("acquire");
 
@@ -45,6 +58,9 @@ acquire(struct spinlock * lock)
 void
 release(struct spinlock * lock)
 {
+  if(lock->magic != LOCKMAGIC)
+    panic("weird lock magic");
+
        if(!holding(lock))
                panic("release");
 
@@ -55,8 +71,6 @@ release(struct spinlock * lock)
        lock->locked = 0;
        if(--cpus[cpu()].nlock == 0)
                sti();
-        // xxx we may have just turned interrupts on during
-        // an interrupt, is that ok?
 }
 
 int

diff --git a/spinlock.h b/spinlock.h
index f866b4c918b3cce1df06e26570ee1b23677b1c02..f124f15359424933d203b107e1910fa34a1d34a0 100644 (file)
--- a/spinlock.h
+++ b/spinlock.h
@@ -1,6 +1,7 @@
 struct spinlock {
+  uint magic;
   char *name;
   uint locked;
-  uint pcs[10];
   int cpu;
+  uint pcs[10];
 };

diff --git a/trap.c b/trap.c
index b172a771a9441d44aec927976439c3fd199569f3..99aaa70ef7b28d960a38d32f7b95f138f3ea4e93 100644 (file)
--- a/trap.c
+++ b/trap.c
@@ -41,6 +41,17 @@ trap(struct trapframe *tf)
     panic("interrupt while holding a lock");
   }
 
+  if(cpu() == 1 && curproc[cpu()] == 0){
+    if(&tf < cpus[cpu()].mpstack || &tf > cpus[cpu()].mpstack + 512){
+      cprintf("&tf %x mpstack %x\n", &tf, cpus[cpu()].mpstack);
+      panic("trap cpu stack");
+    }
+  } else if(curproc[cpu()]){
+    if(&tf < curproc[cpu()]->kstack){
+      panic("trap kstack");
+    }
+  }
+
   if(v == T_SYSCALL){
     struct proc *cp = curproc[cpu()];
     int num = cp->tf->eax;
@@ -97,11 +108,20 @@ trap(struct trapframe *tf)
 
   if(v == (IRQ_OFFSET + IRQ_IDE)){
     ide_intr();
+    if(cpus[cpu()].nlock)
+      panic("ide_intr returned while holding a lock");
+    cli(); // prevent a waiting interrupt from overflowing stack
+    lapic_eoi();
     return;
   }
 
   if(v == (IRQ_OFFSET + IRQ_KBD)){
     kbd_intr();
+    if(cpus[cpu()].nlock){
+      panic("kbd_intr returned while holding a lock");
+    }
+    cli(); // prevent a waiting interrupt from overflowing stack
+    lapic_eoi();
     return;
   }