Merge pull request #141 from hangpark/iss/140

[#140] Avoid accessing frame by other proc during loading frame
hangpark · May 21, 2017 · ba07264 · ba07264
2 parents f44afcd + b924e68
commit ba07264
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 36 deletions.
diff --git a/src/vm/frame.c b/src/vm/frame.c
@@ -24,14 +24,6 @@ static struct list frame_table;
 static struct list_elem *frame_table_pos;
 #endif
 
-/* Frame table element. */
-struct frame
-  {
-    void *kpage;                  /* Kernel page maps to the frame. */
-    struct suppl_pte *suppl_pte;  /* Supplemental page table entry. */
-    struct list_elem elem;        /* List element. */
-  };
-
 static struct frame *frame_evict_and_get (void);
 #ifdef VM_CLOCK
 static struct frame *frame_to_evict_clock (void);
@@ -53,7 +45,7 @@ frame_table_init (void)
 /* Allocates a new user frame with given pte and flags.
    Note that this method is used only for that user frame flag
    is set. */
-void *
+struct frame *
 frame_alloc (struct suppl_pte *pte, enum palloc_flags flags)
 {
   ASSERT (pte != NULL);
@@ -73,9 +65,11 @@ frame_alloc (struct suppl_pte *pte, enum palloc_flags flags)
         }
       f->suppl_pte = pte;
 
+      list_remove (&f->elem);
+
       lock_release (&frame_table_lock);
 
-      return f->kpage;
+      return f;
     }
 
   f = malloc (sizeof (struct frame));
@@ -88,11 +82,17 @@ frame_alloc (struct suppl_pte *pte, enum palloc_flags flags)
 
   f->kpage = kpage;
   f->suppl_pte = pte;
-  list_push_back (&frame_table, &f->elem);
 
   lock_release (&frame_table_lock);
 
-  return kpage;
+  return f;
+}
+
+/* Appends the frame entry into the frame table. */
+void
+frame_append (struct frame *frame)
+{
+  list_push_back (&frame_table, &frame->elem);
 }
 
 /* Returns frame associated KPAGE. Returns NULL if failed. */
@@ -120,21 +120,17 @@ frame_search (void *kpage)
 /* Frees the given frame at kpage.
    If such frame exists in the frame table, remove it. */
 void
-frame_free (void *kpage)
+frame_free (struct frame *frame)
 {
   lock_acquire (&frame_table_lock);
 
-  struct frame *f = frame_search (kpage);
-  if (f != NULL)
-    {
 #ifdef VM_CLOCK
-      if (&f->elem == frame_table_pos)
-        frame_table_pos = list_next (frame_table_pos);
+  if (&frame->elem == frame_table_pos)
+    frame_table_pos = list_next (frame_table_pos);
 #endif
-      list_remove (&f->elem);
-      free (f);
-    }
-  palloc_free_page (kpage);
+  list_remove (&frame->elem);
+  free (frame);
+  palloc_free_page (frame->kpage);
 
   lock_release (&frame_table_lock);
 }

diff --git a/src/vm/frame.h b/src/vm/frame.h
@@ -5,9 +5,18 @@
 #include "threads/palloc.h"
 #include "vm/page.h"
 
+/* Frame table element. */
+struct frame
+  {
+    void *kpage;                  /* Kernel page maps to the frame. */
+    struct suppl_pte *suppl_pte;  /* Supplemental page table entry. */
+    struct list_elem elem;        /* List element. */
+  };
+
 void frame_table_init (void);
-void *frame_alloc (struct suppl_pte *, enum palloc_flags);
-void frame_free (void *kpage);
+struct frame *frame_alloc (struct suppl_pte *, enum palloc_flags);
+void frame_free (struct frame *);
 void frame_remove (void *kpage);
+void frame_append (struct frame *);
 
 #endif /* vm/frame.h */
diff --git a/src/vm/page.c b/src/vm/page.c
@@ -99,8 +99,8 @@ suppl_pt_load_page (void *upage)
     return false;
 
   /* Obtain a new frame. */
-  void *kpage = frame_alloc (pte, PAL_USER);
-  if (kpage == NULL)
+  struct frame *f = frame_alloc (pte, PAL_USER);
+  if (f == NULL)
     return false;
 
   /* Load page content for each page type. */
@@ -109,27 +109,27 @@ suppl_pt_load_page (void *upage)
     {
     /* Page filled with zeros. */
     case PAGE_ZERO:
-      memset (kpage, 0, PGSIZE);
+      memset (f->kpage, 0, PGSIZE);
       break;
 
     /* Page content from the file system. */
     case PAGE_FILE:
       file_seek (pte->file, pte->ofs);
-      if (file_read (pte->file, kpage, pte->read_bytes)
+      if (file_read (pte->file, f->kpage, pte->read_bytes)
           != (int) pte->read_bytes)
         {
-          frame_free (kpage);
+          frame_free (f);
           return false;
         }
-      memset (kpage + pte->read_bytes, 0, pte->zero_bytes);
+      memset (f->kpage + pte->read_bytes, 0, pte->zero_bytes);
       writable = pte->writable;
       break;
 
     /* Page content from the swap disk. */
     case PAGE_SWAP:
-      if (!swap_in (kpage, pte->swap_index))
+      if (!swap_in (f->kpage, pte->swap_index))
         {
-          frame_free (kpage);
+          frame_free (f);
           return false;
         };
       break;
@@ -140,17 +140,20 @@ suppl_pt_load_page (void *upage)
     }
 
   /* Install upage to kpage. */
-  if (!pagedir_set_page (pte->pagedir, upage, kpage, writable))
+  if (!pagedir_set_page (pte->pagedir, upage, f->kpage, writable))
     {
-      frame_free (kpage);
+      frame_free (f);
       return false;
     }
 
   /* Set dirty value of kernel page to false. */
-  pagedir_set_dirty (pte->pagedir, kpage, false);
+  pagedir_set_dirty (pte->pagedir, f->kpage, false);
 
   /* Append result to supplemental page table. */
-  pte->kpage = kpage;
+  pte->kpage = f->kpage;
+
+  /* Append frame entry to the frame table. */
+  frame_append (f);
 
   return true;
 }