From 2c56547272e43b483d560a61692f1e24926a82fb Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Mon, 4 Aug 2014 13:06:48 -0400
Subject: [PATCH] every iput() and namei() must be inside a transaction

---
 exec.c      |  10 ++++-
 fs.c        |   3 ++
 log.c       |   6 +--
 proc.c      |   2 +
 sysfile.c   |  35 ++++++++++++-----
 usertests.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 145 insertions(+), 17 deletions(-)

diff --git a/exec.c b/exec.c
index a85e203..7eaef5b 100644
--- a/exec.c
+++ b/exec.c
@@ -18,8 +18,11 @@ exec(char *path, char **argv)
   struct proghdr ph;
   pde_t *pgdir, *oldpgdir;
 
-  if((ip = namei(path)) == 0)
+  begin_trans();
+  if((ip = namei(path)) == 0){
+    commit_trans();
     return -1;
+  }
   ilock(ip);
   pgdir = 0;
 
@@ -47,6 +50,7 @@ exec(char *path, char **argv)
       goto bad;
   }
   iunlockput(ip);
+  commit_trans();
   ip = 0;
 
   // Allocate two pages at the next page boundary.
@@ -95,7 +99,9 @@ exec(char *path, char **argv)
  bad:
   if(pgdir)
     freevm(pgdir);
-  if(ip)
+  if(ip){
     iunlockput(ip);
+    commit_trans();
+  }
   return -1;
 }
diff --git a/fs.c b/fs.c
index 9d6dfd1..7621d08 100644
--- a/fs.c
+++ b/fs.c
@@ -314,6 +314,8 @@ iunlock(struct inode *ip)
 // be recycled.
 // If that was the last reference and the inode has no links
 // to it, free the inode (and its content) on disk.
+// All calls to iput() must be inside a transaction in
+// case it has to free the inode.
 void
 iput(struct inode *ip)
 {
@@ -601,6 +603,7 @@ skipelem(char *path, char *name)
 // Look up and return the inode for a path name.
 // If parent != 0, return the inode for the parent and copy the final
 // path element into name, which must have room for DIRSIZ bytes.
+// Must be called inside a transaction since it calls iput().
 static struct inode*
 namex(char *path, int nameiparent, char *name)
 {
diff --git a/log.c b/log.c
index 1814edc..95cc4d5 100644
--- a/log.c
+++ b/log.c
@@ -5,7 +5,7 @@
 #include "fs.h"
 #include "buf.h"
 
-// Simple logging. Each system call that might write the file system
+// Simple logging. Each file system system call
 // should be surrounded with begin_trans() and commit_trans() calls.
 //
 // The log holds at most one transaction at a time. Commit forces
@@ -18,10 +18,6 @@
 // one transaction reading a block that another one has modified,
 // for example an i-node block.
 //
-// Read-only system calls don't need to use transactions, though
-// this means that they may observe uncommitted data. I-node and
-// buffer locks prevent read-only calls from seeing inconsistent data.
-//
 // The log is a physical re-do log containing disk blocks.
 // The on-disk log format:
 //   header block, containing sector #s for block A, B, C, ...
diff --git a/proc.c b/proc.c
index e19539c..db0e9c7 100644
--- a/proc.c
+++ b/proc.c
@@ -186,7 +186,9 @@ exit(void)
     }
   }
 
+  begin_trans();
   iput(proc->cwd);
+  commit_trans();
   proc->cwd = 0;
 
   acquire(&ptable.lock);
diff --git a/sysfile.c b/sysfile.c
index 64f2b33..095fca7 100644
--- a/sysfile.c
+++ b/sysfile.c
@@ -120,10 +120,12 @@ sys_link(void)
 
   if(argstr(0, &old) < 0 || argstr(1, &new) < 0)
     return -1;
-  if((ip = namei(old)) == 0)
-    return -1;
 
   begin_trans();
+  if((ip = namei(old)) == 0){
+    commit_trans();
+    return -1;
+  }
 
   ilock(ip);
   if(ip->type == T_DIR){
@@ -186,10 +188,12 @@ sys_unlink(void)
 
   if(argstr(0, &path) < 0)
     return -1;
-  if((dp = nameiparent(path, name)) == 0)
-    return -1;
 
   begin_trans();
+  if((dp = nameiparent(path, name)) == 0){
+    commit_trans();
+    return -1;
+  }
 
   ilock(dp);
 
@@ -286,18 +290,24 @@ sys_open(void)
 
   if(argstr(0, &path) < 0 || argint(1, &omode) < 0)
     return -1;
+
+  begin_trans();
+
   if(omode & O_CREATE){
-    begin_trans();
     ip = create(path, T_FILE, 0, 0);
-    commit_trans();
-    if(ip == 0)
+    if(ip == 0){
+      commit_trans();
       return -1;
+    }
   } else {
-    if((ip = namei(path)) == 0)
+    if((ip = namei(path)) == 0){
+      commit_trans();
       return -1;
+    }
     ilock(ip);
     if(ip->type == T_DIR && omode != O_RDONLY){
       iunlockput(ip);
+      commit_trans();
       return -1;
     }
   }
@@ -306,9 +316,11 @@ sys_open(void)
     if(f)
       fileclose(f);
     iunlockput(ip);
+    commit_trans();
     return -1;
   }
   iunlock(ip);
+  commit_trans();
 
   f->type = FD_INODE;
   f->ip = ip;
@@ -361,15 +373,20 @@ sys_chdir(void)
   char *path;
   struct inode *ip;
 
-  if(argstr(0, &path) < 0 || (ip = namei(path)) == 0)
+  begin_trans();
+  if(argstr(0, &path) < 0 || (ip = namei(path)) == 0){
+    commit_trans();
     return -1;
+  }
   ilock(ip);
   if(ip->type != T_DIR){
     iunlockput(ip);
+    commit_trans();
     return -1;
   }
   iunlock(ip);
   iput(proc->cwd);
+  commit_trans();
   proc->cwd = ip;
   return 0;
 }
diff --git a/usertests.c b/usertests.c
index 92b9136..5a78c7c 100644
--- a/usertests.c
+++ b/usertests.c
@@ -13,6 +13,106 @@ char name[3];
 char *echoargv[] = { "echo", "ALL", "TESTS", "PASSED", 0 };
 int stdout = 1;
 
+// does chdir() call iput(p->cwd) in a transaction?
+void
+iputtest(void)
+{
+  printf(stdout, "iput test\n");
+
+  if(mkdir("iputdir") < 0){
+    printf(stdout, "mkdir failed\n");
+    exit();
+  }
+  if(chdir("iputdir") < 0){
+    printf(stdout, "chdir iputdir failed\n");
+    exit();
+  }
+  if(unlink("../iputdir") < 0){
+    printf(stdout, "unlink ../iputdir failed\n");
+    exit();
+  }
+  if(chdir("/") < 0){
+    printf(stdout, "chdir / failed\n");
+    exit();
+  }
+  printf(stdout, "iput test ok\n");
+}
+
+// does exit() call iput(p->cwd) in a transaction?
+void
+exitiputtest(void)
+{
+  int pid;
+
+  printf(stdout, "exitiput test\n");
+
+  pid = fork();
+  if(pid < 0){
+    printf(stdout, "fork failed\n");
+    exit();
+  }
+  if(pid == 0){
+    if(mkdir("iputdir") < 0){
+      printf(stdout, "mkdir failed\n");
+      exit();
+    }
+    if(chdir("iputdir") < 0){
+      printf(stdout, "child chdir failed\n");
+      exit();
+    }
+    if(unlink("../iputdir") < 0){
+      printf(stdout, "unlink ../iputdir failed\n");
+      exit();
+    }
+    exit();
+  }
+  wait();
+  printf(stdout, "exitiput test ok\n");
+}
+
+// does the error path in open() for attempt to write a
+// directory call iput() in a transaction?
+// needs a hacked kernel that pauses just after the namei()
+// call in sys_open():
+//    if((ip = namei(path)) == 0)
+//      return -1;
+//    {
+//      int i;
+//      for(i = 0; i < 10000; i++)
+//        yield();
+//    }
+void
+openiputtest(void)
+{
+  int pid;
+
+  printf(stdout, "openiput test\n");
+  if(mkdir("oidir") < 0){
+    printf(stdout, "mkdir oidir failed\n");
+    exit();
+  }
+  pid = fork();
+  if(pid < 0){
+    printf(stdout, "fork failed\n");
+    exit();
+  }
+  if(pid == 0){
+    int fd = open("oidir", O_RDWR);
+    if(fd >= 0){
+      printf(stdout, "open directory for write succeeded\n");
+      exit();
+    }
+    exit();
+  }
+  sleep(1);
+  if(unlink("oidir") != 0){
+    printf(stdout, "unlink failed\n");
+    exit();
+  }
+  wait();
+  printf(stdout, "openiput test ok\n");
+}
+
 // simple file system tests
 
 void
@@ -187,7 +287,7 @@ void dirtest(void)
     printf(stdout, "unlink dir0 failed\n");
     exit();
   }
-  printf(stdout, "mkdir test\n");
+  printf(stdout, "mkdir test ok\n");
 }
 
 void
@@ -1628,6 +1728,10 @@ main(int argc, char *argv[])
   writetest1();
   createtest();
 
+  openiputtest();
+  exitiputtest();
+  iputtest();
+
   mem();
   pipe1();
   preempt();