xv6 lab copy-on-write lab: test copyinstr2 failed

Question

when i work on the xv6 copy-on-write lab, i find that the copyinstr2 test is failed,here is the message and code: Message: $ usertests copyinstr2 usertests starting test copyinstr2: exec(echo, BIG) succeeded, should have failed FAILED SOME TESTS FAILED Code:

Add:

#referance count struct
struct {
  struct spinlock lock;
  uint count[38000];
} kreferance;

#referance functions
uint
krefget(uint64 pa)
{
  int i;
  uint ct;

  if(pa >= MAXVA)
    return -1;
  i = (PGROUNDDOWN(pa) - (uint64)end) / PGSIZE;
  acquire(&kreferance.lock);
  ct = kreferance.count[i];
  release(&kreferance.lock);

  return ct;
}
void
krefset(uint64 pa, uint ct)
{
  int i;

  if(pa >= MAXVA)
    panic("krefset");
  i = (PGROUNDDOWN(pa) - (uint64)end) / PGSIZE;
  acquire(&kreferance.lock);
  kreferance.count[i] = ct;
  release(&kreferance.lock);
}

#new pte flag
#define PTE_C (1L << 54) //copy on write flag
#define PTE_N (1L << 55) //no write flag

#copyonwrite()
int
copyonwrite(pagetable_t pagetable, uint64 va, pte_t *pte)
{
  uint64 pa;
  uint ct;

  pa= PTE2PA(*pte);

  if((ct = krefget(pa)) > 1){
    uint64 flags;
    void *mem;

    //alloc page
    if((mem = kalloc()) == 0)
      return -1;
    //copy content
    memmove(mem, (void *)pa, PGSIZE);
    //get flags
    flags = PTE_FLAGS(*pte);
    if((*pte & PTE_N) == 0){
      flags |= PTE_W;
    }
    //map new page
    *pte &= ~PTE_V;
    if(mappages(pagetable, va, PGSIZE, (uint64)mem, flags) != 0){
      kfree(mem);
      return -1;
    }
    //sub referance count
    krefset(pa, ct - 1);
  } else {
    //rm PTE_C flag
    *pte &= ~PTE_C;
    //recover PTE_W flag
    if((*pte & PTE_N) == 0){
      *pte |= PTE_W;
    } else {
      *pte &= ~PTE_N;
    }
  }
  return 0;
}

Change:
#change mappages() def for high bits pte flags
#PTE2PA()
#define PTE2PA(pte) ((((pte) & ~(0x7FL << 54)) >> 10) << 12)
#kinit()
void
kinit()
{
  initlock(&kmem.lock, "kmem");
+  initlock(&kreferance.lock, "kreferance");
+  memset((void *)kreferance.count, 0, 38000 * 4);
  freerange(end, (void*)PHYSTOP);
}

#kalloc()
void *
kalloc(void)
{
  struct run *r;

  acquire(&kmem.lock);
  r = kmem.freelist;
  if(r)
    kmem.freelist = r->next;
  release(&kmem.lock);
  
+  if(r)
+   krefset((uint64)r, 1);
  if(r)
    memset((char*)r, 5, PGSIZE); // fill with junk
  return (void*)r;
}
#kfree()
void
kfree(void *pa)
{
  struct run *r;
  uint ct;

  if(((uint64)pa % PGSIZE) != 0 || (char*)pa < end || (uint64)pa >= PHYSTOP)
    panic("kfree");

+  ct = krefget((uint64)pa);
+  if(ct <= 1){
+   // Fill with junk to catch dangling refs.
+    memset(pa, 1, PGSIZE);
+
+    r = (struct run*)pa;
+
+    acquire(&kmem.lock);
+    r->next = kmem.freelist;
+    kmem.freelist = r;
+    release(&kmem.lock);  
+  }else{
+    krefset((uint64)pa, ct - 1);
+  }
}
#uvmcopy()
int
uvmcopy(pagetable_t old, pagetable_t new, uint64 sz)
{
  pte_t *pte;
  uint64 pa, i, flags;

  for(i = 0; i < sz; i += PGSIZE){
    if((pte = walk(old, i, 0)) == 0)
      panic("uvmcopy: pte should exist");
    if((*pte & PTE_V) == 0)
      panic("uvmcopy: page not present");
    //get physical address
+    pa = PTE2PA(*pte);
+    //get flags
+    flags = PTE_FLAGS(*pte);
+    //add flags like PTE_C, PTE_N
+    if(*pte & PTE_W){
+      flags &= ~PTE_W;
+      flags |= PTE_C;
+    } else {
+      flags |= PTE_N;
+      flags |= PTE_C;
+    }
+    //map page
+    if(mappages(new, i, PGSIZE, pa, flags) != 0)
+      goto err;
+    //change parent pagetable entry
+    if(*pte & PTE_W){
+      *pte &= ~PTE_W;
+      *pte |= PTE_C;
+    } else {
+      *pte |= PTE_N;
+      *pte |= PTE_C;
+    }
+    //add referance count for this page
+    krefset(pa, krefget(pa) + 1);
  }
  return 0;

 err:
  uvmunmap(new, 0, i / PGSIZE, 1);
  return -1;
}
#copyout()
int
copyout(pagetable_t pagetable, uint64 dstva, char *src, uint64 len)
{
  uint64 n, va0, pa0;
  pte_t *pte;

  while(len > 0){
    va0 = PGROUNDDOWN(dstva);
    pa0 = walkaddr(pagetable, va0);
    if(pa0 == 0)
      return -1;
    //get pagetable entry
+    if((pte = walk(pagetable, va0, 0)) == 0)
+      return -1;
+    //if is a copy on write page then alloc new page
+    if(*pte & PTE_C){
+      if(copyonwrite(pagetable, va0, pte) != 0)
+        return -1;
+      //get new physical address
+      pa0 = walkaddr(pagetable, va0);
+    }
    n = PGSIZE - (dstva - va0);
    if(n > len)
      n = len;
    memmove((void *)(pa0 + (dstva - va0)), src, n);

    len -= n;
    src += n;
    dstva = va0 + PGSIZE;
  }
  return 0;
}
#usertrap()
void
usertrap(void)
{
  int which_dev = 0;

  if((r_sstatus() & SSTATUS_SPP) != 0)
    panic("usertrap: not from user mode");

  // send interrupts and exceptions to kerneltrap(),
  // since we're now in the kernel.
  w_stvec((uint64)kernelvec);

  struct proc *p = myproc();
  
  // save user program counter.
  p->trapframe->epc = r_sepc();
  
  if(r_scause() == 8){
    // system call

    if(killed(p))
      exit(-1);

    // sepc points to the ecall instruction,
    // but we want to return to the next instruction.
    p->trapframe->epc += 4;

    // an interrupt will change sepc, scause, and sstatus,
    // so enable only now that we're done with those registers.
    intr_on();

    syscall();
  } else if((which_dev = devintr()) != 0){
    // ok
  //if is pagefault then deal with it
+  } else if(r_scause() == 0xc || r_scause() == 0xd || r_scause() == 0xf){
+    uint64 va;
+    pte_t *pte;
+   
+    //get virtual address
+    va = r_stval();
+    va = PGROUNDDOWN(va);
+    //get pagetable entry
+    if((pte = walk(p->pagetable, va, 0)) == 0)
+      p->killed = 1;
+    //if is copy on write then alloc new page
+    if(*pte & PTE_C){
+      if(copyonwrite(p->pagetable, va, pte) != 0)
+        p->killed = 1;
+    //if is other reason then kill proc
+    } else {
+      p->killed = 1;
+    }
  } else {
    printf("usertrap(): unexpected scause %p pid=%d\n", r_scause(), p->pid);
    printf("            sepc=%p stval=%p\n", r_sepc(), r_stval());
    setkilled(p);
  }

  if(killed(p))
    exit(-1);

  // give up the CPU if this is a timer interrupt.
  if(which_dev == 2)
    yield();

  usertrapret();
}

after debugging i find that the fetchstr() in sys_exec() not retrun -1 and then it call exec() and run echo so that child return not 747 but -1, here is the wrong part of copyinstr2 test's code:

  if(pid == 0){
    static char big[PGSIZE+1];
    for(int i = 0; i < PGSIZE; i++)
      big[i] = 'x';
    big[PGSIZE] = '\0';
    char *args2[] = { big, big, big, 0 };
    ret = exec("echo", args2);
    if(ret != -1){
      printf("exec(echo, BIG) returned %d, not -1\n", fd);
      exit(1);
    }
    exit(747); // OK
  }

  int st = 0;
  wait(&st);
  if(st != 747){
    printf("exec(echo, BIG) succeeded, should have failed\n");
    exit(1);
  }

how can i fix it?

2023-10-16: i find that a page has PTE_W flag is copied as a no PTE_W flag page ,because the pte of parent is also copied from other and does not be copied to new page so it does not have PTE_W flag but PTE_C flag, i fix it by add new condition in uvmcopy, but is lose some free pages after usertests

    //add flags like PTE_C, PTE_N
if((*pte & PTE_W) == PTE_W){
  flags &= ~PTE_W;
  flags |= PTE_C;
} else if ((*pte & PTE_C) == 0 
    || (*pte & PTE_N) == PTE_N){
  flags |= PTE_N;
  flags |= PTE_C;
} else {
  flags |= PTE_C;
}
//map page
if(mappages(new, i, PGSIZE, pa, flags) != 0)
  goto err;
//change parent pagetable entry
if((*pte & PTE_C) == 0){
  if((*pte & PTE_W) == PTE_W){
    *pte &= ~PTE_W;
    *pte |= PTE_C;
  } else {
    *pte |= PTE_N;
    *pte |= PTE_C;
  }
}