From: David van Moolenbroek <david@minix3.org>
Date: Mon, 18 May 2009 11:27:12 +0000 (+0000)
Subject: Various VFS and MFS fixes to improve correctness, consistency and
X-Git-Tag: v3.1.4~27
X-Git-Url: http://zhaoyanbai.com/repos/html/index.html?a=commitdiff_plain;h=f76d75a5ecf13d22494ca3a9ed13ba09785f440b;p=minix.git

Various VFS and MFS fixes to improve correctness, consistency and
POSIX compliance.

VFS changes:
* truncate() on a file system mounted read-only no longer panics MFS.
* ftruncate() and fcntl(F_FREESP) now check for write permission on
  the file descriptor instead of the file, write().
* utime(), chown() and fchown() now check for file system read-only
  status.

MFS changes:
* link() and rename() no longer return the internal EENTERMOUNT and
  ELEAVEMOUNT errors to the application as part of a check on the
  source path.
* rename() now treats EENTERMOUNT from the destination path check as
  an error, preventing file system corruption from renaming a normal
  directory to an existing mountpoint directory.
* mountpoints (mounted-on dirs) are hidden better during lookups:
  - if a lookup starts from a mountpoint, the first component has to
    be ".." (anything else being a VFS-FS protocol violation).
  - in that case, the permissions of the mountpoint are not checked.
  - in all other cases, visiting a mountpoint always results in
    EENTERMOUNT.
* a lookup on ".." from a mount root or chroot(2) root no longer
  succeeds if the caller does not have search permission on that
  directory.
* POSIX: getdents() now updates directory access times.
* POSIX: readlink() now returns partial results instead of ERANGE.

Miscellaneous changes:
* semaphore file handling bug (leading to hangs) fixed in test 32.

The VFS changes should now put the burden of checking for read-only
status of file systems entirely on VFS, and limit the access
permission checks that file systems have to perform, to checking
search permission on directories during lookups. From this point on,
any deviation from that spceification should be considered a bug.
Note that for legacy reasons, the root partition is assumed to be
mounted read-write.
---

diff --git a/servers/mfs/link.c b/servers/mfs/link.c
index c9edb16fe..7f58f8ad6 100644
--- a/servers/mfs/link.c
+++ b/servers/mfs/link.c
@@ -87,6 +87,7 @@ printf("MFS(%d) get_inode by fs_link() failed\n", SELF_E);
 	if ( (new_ip = advance_o(&ip, string)) == NIL_INODE) {
 		r = err_code;
 		if (r == ENOENT) r = OK;
+		else if (r == EENTERMOUNT || r == ELEAVEMOUNT) r = EEXIST;
 	} else {
 		put_inode(new_ip);
 		r = EEXIST;
@@ -169,6 +170,7 @@ printf("MFS(%d) get_inode by fs_link() failed\n", SELF_E);
 	if ( (new_ip = advance_nocheck(&ip, string)) == NIL_INODE) {
 		r = err_code;
 		if (r == ENOENT) r = OK;
+		else if (r == EENTERMOUNT || r == ELEAVEMOUNT) r = EEXIST;
 	} else {
 		put_inode(new_ip);
 		r = EEXIST;
@@ -385,13 +387,12 @@ PUBLIC int fs_rdlink_s()
 
   if (!S_ISLNK(rip->i_mode))
 	r = EACCES;
-  else if (copylen < rip->i_size) 
-	r = ERANGE;
   else if ((b = read_map(rip, (off_t) 0)) == NO_BLOCK)
 	r = EIO;
   else {
 	/* Passed all checks */
-	copylen = rip->i_size;
+	if (copylen > rip->i_size)
+		copylen = rip->i_size;
 	bp = get_block(rip->i_dev, b, NORMAL);
 	r = sys_safecopyto(FS_PROC_NR, fs_m_in.REQ_GRANT, 0, 
 	(vir_bytes) bp->b_data, (vir_bytes) copylen, D);
@@ -605,13 +606,23 @@ PUBLIC int fs_rename_o()
   if ( (old_dirp = get_inode(fs_dev, fs_m_in.REQ_OLD_DIR)) == NIL_INODE) 
         return(err_code);
 
-  if ( (old_ip = advance_o(&old_dirp, old_name)) == NIL_INODE) r = err_code;
+  if ( (old_ip = advance_o(&old_dirp, old_name)) == NIL_INODE) {
+	r = err_code;
+	if (r == EENTERMOUNT) r = EBUSY;	/* should this fail at all? */
+	else if (r == ELEAVEMOUNT) r = EINVAL;	/* rename on dot-dot */
+  }
 
   /* Get new dir inode */ 
   if ( (new_dirp = get_inode(fs_dev, fs_m_in.REQ_NEW_DIR)) == NIL_INODE) 
       r = err_code;
   new_ip = advance_o(&new_dirp, new_name);	/* not required to exist */
 
+  /* However, if the check failed because the file does exist, don't continue.
+   * Note that ELEAVEMOUNT is covered by the dot-dot check later.
+   */
+  if (new_ip == NIL_INODE && err_code == EENTERMOUNT)
+ 	r = EBUSY;
+
   if (old_ip != NIL_INODE)
 	odir = ((old_ip->i_mode & I_TYPE) == I_DIRECTORY);  /* TRUE iff dir */
 
@@ -799,14 +810,23 @@ PUBLIC int fs_rename_s()
   if ( (old_dirp = get_inode(fs_dev, fs_m_in.REQ_REN_OLD_DIR)) == NIL_INODE) 
         return(err_code);
 
-  if ( (old_ip = advance_nocheck(&old_dirp, old_name)) == NIL_INODE)
+  if ( (old_ip = advance_nocheck(&old_dirp, old_name)) == NIL_INODE) {
 	r = err_code;
+	if (r == EENTERMOUNT) r = EBUSY;	/* should this fail at all? */
+	else if (r == ELEAVEMOUNT) r = EINVAL;	/* rename on dot-dot */
+  }
 
   /* Get new dir inode */ 
   if ( (new_dirp = get_inode(fs_dev, fs_m_in.REQ_REN_NEW_DIR)) == NIL_INODE) 
       r = err_code;
   new_ip = advance_nocheck(&new_dirp, new_name); /* not required to exist */
 
+  /* However, if the check failed because the file does exist, don't continue.
+   * Note that ELEAVEMOUNT is covered by the dot-dot check later.
+   */
+  if (new_ip == NIL_INODE && err_code == EENTERMOUNT)
+ 	r = EBUSY;
+
   if (old_ip != NIL_INODE)
 	odir = ((old_ip->i_mode & I_TYPE) == I_DIRECTORY);  /* TRUE iff dir */
 
diff --git a/servers/mfs/path.c b/servers/mfs/path.c
index 9a33284cc..22bdb1b7a 100644
--- a/servers/mfs/path.c
+++ b/servers/mfs/path.c
@@ -432,7 +432,7 @@ int *symlinkp;
    * just look up the first (or only) component in path after skipping any
    * leading slashes. 
    */
-  int r;
+  int r, leaving_mount;
   struct inode *rip, *dir_ip;
   char *cp, *ncp;
   char string[NAME_MAX+1];
@@ -460,6 +460,12 @@ int *symlinkp;
 	return ENOENT;
   }
 
+  /* If the given start inode is a mountpoint, we must be here because the file
+   * system mounted on top returned an ELEAVEMOUNT error. In this case, we must
+   * only accept ".." as the first path component.
+   */
+  leaving_mount = rip->i_mountpoint;
+
   /* Scan the path component by component. */
   while (TRUE) {
 	if (cp[0] == '\0')
@@ -499,12 +505,19 @@ int *symlinkp;
 
 	/* Special code for '..'. A process is not allowed to leave a chrooted
 	 * environment. A lookup of '..' at the root of a mounted filesystem
-	 * has to return ELEAVEMOUNT.
+	 * has to return ELEAVEMOUNT. In both cases, the caller needs search
+	 * permission for the current inode, as it is used as directory.
 	 */
 	if (strcmp(string, "..") == 0)
 	{
 		if (rip->i_num == root_ino)
 		{
+			/* 'rip' is now accessed as directory */
+			if ((r = forbidden(rip, X_BIT)) != OK) {
+				put_inode(rip);
+				return r;
+			}
+
 			cp= ncp;
 			continue;	/* Just ignore the '..' at a process'
 					 * root.
@@ -512,27 +525,47 @@ int *symlinkp;
 		}
 		if (rip->i_num == ROOT_INODE && !rip->i_sp->s_is_root) {
 			/* Climbing up mountpoint */
+
+			/* 'rip' is now accessed as directory */
+			if ((r = forbidden(rip, X_BIT)) != OK) {
+				put_inode(rip);
+				return r;
+			}
+
 			put_inode(rip);
-			*res_inop= NULL;
 			*offsetp += cp-user_path;
 			return ELEAVEMOUNT;
 		}
 	}
 	else
 	{
-		/* Only check for a mount point if we are not looking for '..'.
-		 */
-		if (rip->i_mountpoint)
-		{
-			*res_inop= rip;
-			*offsetp += cp-user_path;
-			return EENTERMOUNT;
+		/* Check for misbehaving child file systems. */
+		if (leaving_mount) {
+        		printf("mfs:parse_path_s: first component after "
+        			"leaving mount is '%s'\n", string);
+
+			/* DO NOT pass back EENTERMOUNT. We supposedly got here
+			 * because the child file system treated the last path
+			 * component as "..". It is likely to do that again.
+			 */
+			put_inode(rip);
+			return EINVAL;
 		}
 	}
 
-	/* There is more path.  Keep parsing. */
+	/* Only check for a mount point if we are not coming from one. */
+	if (!leaving_mount && rip->i_mountpoint)
+	{
+		*res_inop= rip;
+		*offsetp += cp-user_path;
+		return EENTERMOUNT;
+	}
+
+	/* There is more path.  Keep parsing.
+	 * If we're leaving a mountpoint, skip directory permission checks.
+	 */
 	dir_ip = rip;
-	r = advance_s1(dir_ip, string, &rip);
+	r = advance_s1(dir_ip, leaving_mount ? dot2 : string, &rip);
 
 	if (r != OK)
 	{
@@ -540,6 +573,8 @@ int *symlinkp;
 		return r;
 	}
 	
+	leaving_mount = 0;
+
 	/* The call to advance() succeeded.  Fetch next component. */
 	if (S_ISLNK(rip->i_mode)) {
 
@@ -572,7 +607,6 @@ int *symlinkp;
 		{
                         put_inode(dir_ip);
                         put_inode(rip);
-			*res_inop= NULL;
 			*offsetp= 0;
                         return ESYMLINK;
 		}
diff --git a/servers/mfs/read.c b/servers/mfs/read.c
index cf948f289..5619134a2 100644
--- a/servers/mfs/read.c
+++ b/servers/mfs/read.c
@@ -1016,6 +1016,9 @@ printf("MFS(%d) get_inode by fs_getdents() failed\n", SELF_E);
 		fs_m_out.RES_GDE_POS_CHANGE= new_pos-pos;
 	else
 		fs_m_out.RES_GDE_POS_CHANGE= 0;
+
+	rip->i_update |= ATIME;
+	rip->i_dirt = DIRTY;
 	r= OK;
   }
 
diff --git a/servers/vfs/link.c b/servers/vfs/link.c
index ff85ab305..e2fb29610 100644
--- a/servers/vfs/link.c
+++ b/servers/vfs/link.c
@@ -246,7 +246,8 @@ PUBLIC int do_truncate()
   /* Request lookup */
   if ((r = lookup_vp(0 /*flags*/, 0 /*!use_realuid*/, &vp)) != OK) return r;
   
-  r= truncate_vn(vp, m_in.m2_l1);
+  if ((r = forbidden(vp, W_BIT, 0 /*!use_realuid*/)) == OK)
+	  r = truncate_vn(vp, m_in.m2_l1);
 
   put_vnode(vp);
 
@@ -266,8 +267,8 @@ PUBLIC int do_ftruncate()
   
   if ( (rfilp = get_filp(m_in.m2_i1)) == NIL_FILP)
         return err_code;
-  if ( (r = forbidden(rfilp->filp_vno, W_BIT, 0 /*!use_realuid*/)) != OK)
-	return r;
+  if (!(rfilp->filp_mode & W_BIT))
+  	return EBADF;
   return truncate_vn(rfilp->filp_vno, m_in.m2_l1);
 }
 
diff --git a/servers/vfs/misc.c b/servers/vfs/misc.c
index baaeb3756..e3fc41b0e 100644
--- a/servers/vfs/misc.c
+++ b/servers/vfs/misc.c
@@ -219,8 +219,8 @@ PUBLIC int do_fcntl()
 		return EINVAL;
 	}
 
-	if ( (r = forbidden(f->filp_vno, W_BIT, 0 /*!use_realuid*/)) != OK)
-	  return r;
+	if (!(f->filp_mode & W_BIT))
+		return EBADF;
 
 	/* Copy flock data from userspace. */
 	if((r = sys_datacopy(who_e, (vir_bytes) m_in.name1, 
diff --git a/servers/vfs/protect.c b/servers/vfs/protect.c
index 97082ce16..c579c9655 100644
--- a/servers/vfs/protect.c
+++ b/servers/vfs/protect.c
@@ -127,6 +127,10 @@ PUBLIC int do_chown()
 	if (gid != m_in.group)
 		r = EPERM;	/* only change to the current gid */
   }
+
+  if (r == OK)
+  	r = read_only(vp);
+
   if (r != OK) {
 	put_vnode(vp);
 	return r;
diff --git a/servers/vfs/time.c b/servers/vfs/time.c
index 5e34e395b..aa30a2861 100644
--- a/servers/vfs/time.c
+++ b/servers/vfs/time.c
@@ -59,6 +59,9 @@ PUBLIC int do_utime()
 	r = forbidden(vp, W_BIT, 0 /*!use_realuid*/);
   }
 
+  if (r == OK)
+  	r = read_only(vp);
+
   if (r != OK)
   {
 	put_vnode(vp);
diff --git a/test/test32.c b/test/test32.c
index 03105f44a..64d4d2cba 100644
--- a/test/test32.c
+++ b/test/test32.c
@@ -281,7 +281,7 @@ void test32c()
 		alarm(20);
 		if (chdir("newdir") != 0) e(15);
 		Creat("/tmp/sema.11a");
-		while (stat("/tmp/sema.11a", &st1) == -1) sleep(1);
+		while (stat("/tmp/sema.11a", &st1) == 0) sleep(1);
 		exit(0);
 	    default:
 		wait(&stat_loc);
@@ -291,7 +291,7 @@ void test32c()
 	/* Child B. */
 	if (chdir("olddir") != 0) e(17);
 	Creat("/tmp/sema.11b");
-	while (stat("/tmp/sema.11b", &st1) == -1) sleep(1);
+	while (stat("/tmp/sema.11b", &st1) == 0) sleep(1);
 	exit(0);
       default:
 	/* Wait for child A. It will keep ``newdir'' bussy. */