From: Thomas Veerman <thomas@minix3.org>
Date: Wed, 14 Nov 2012 13:12:37 +0000 (+0000)
Subject: VFS: fix deadlock when out of worker threads
X-Git-Tag: v3.2.1~231
X-Git-Url: http://zhaoyanbai.com/repos/%22http:/www.isc.org/icons/man.dnssec-dsfromkey.html?a=commitdiff_plain;h=badec36b331e6422762876434086f6c897a38188;p=minix.git

VFS: fix deadlock when out of worker threads

There is a deadlock vulnerability when there are no worker threads
available and all of them blocked on a worker thread that's waiting for a
reply from a driver or a reply from an FS that needs to make a back call. In
these cases the deadlock resolver thread should kick in, but didn't in all
cases. Moreover, POSIX calls from File Servers weren't handled properly
anymore, which also could lead to deadlocks.
---

diff --git a/servers/vfs/dmap.c b/servers/vfs/dmap.c
index deccc8d36..887c67291 100644
--- a/servers/vfs/dmap.c
+++ b/servers/vfs/dmap.c
@@ -79,6 +79,7 @@ int do_mapdriver()
   vir_bytes label_vir;
   size_t label_len;
   char label[LABEL_MAX];
+  struct fproc *rfp;
 
   /* Only RS can map drivers. */
   if (who_e != RS_PROC_NR) return(EPERM);
@@ -108,6 +109,10 @@ int do_mapdriver()
 	return(EINVAL);
   }
 
+  /* Process is a service */
+  rfp = &fproc[_ENDPOINT_P(endpoint)];
+  rfp->fp_flags |= FP_SRV_PROC;
+
   /* Try to update device mapping. */
   return map_driver(label, major, endpoint, style, flags);
 }
@@ -129,7 +134,6 @@ int flags;			/* device flags */
   int slot, s;
   size_t len;
   struct dmap *dp;
-  struct fproc *rfp;
 
   /* Get pointer to device entry in the dmap table. */
   if (major < 0 || major >= NR_DEVICES) return(ENODEV);
@@ -159,9 +163,6 @@ int flags;			/* device flags */
 	/* This is not a problem only when we force this driver mapping */
 	if (! (flags & DRV_FORCED))
 		return(EINVAL);
-  } else {
-	rfp = &fproc[slot];
-	rfp->fp_flags |= FP_SYS_PROC;	/* Process is a driver */
   }
 
   if (label != NULL) {
@@ -228,16 +229,21 @@ void dmap_unmap_by_endpt(endpoint_t proc_e)
 }
 
 /*===========================================================================*
- *		               map_service                                   *
+ *		               map_service				     *
  *===========================================================================*/
 int map_service(struct rprocpub *rpub)
 {
 /* Map a new service by storing its device driver properties. */
   int r;
   struct dmap *fdp, *sdp;
+  struct fproc *rfp;
+
+  /* Process is a service */
+  rfp = &fproc[_ENDPOINT_P(rpub->endpoint)];
+  rfp->fp_flags |= FP_SRV_PROC;
 
   /* Not a driver, nothing more to do. */
-  if(rpub->dev_nr == NO_DEV) return(OK);
+  if (rpub->dev_nr == NO_DEV) return(OK);
 
   /* Map driver. */
   r = map_driver(rpub->label, rpub->dev_nr, rpub->endpoint, rpub->dev_style,
diff --git a/servers/vfs/fproc.h b/servers/vfs/fproc.h
index 49fa2f890..9ef7acbd0 100644
--- a/servers/vfs/fproc.h
+++ b/servers/vfs/fproc.h
@@ -62,7 +62,7 @@ EXTERN struct fproc {
 #define FP_PENDING	 0010	/* Set if process has pending work */
 #define FP_EXITING	 0020	/* Set if process is exiting */
 #define FP_PM_PENDING	 0040	/* Set if process has pending PM request */
-#define FP_SYS_PROC	 0100	/* Set if process is a driver or FS */
+#define FP_SRV_PROC	 0100	/* Set if process is a service */
 #define FP_DROP_WORK	 0200	/* Set if process won't accept new work */
 
 /* Field values. */
diff --git a/servers/vfs/main.c b/servers/vfs/main.c
index 3e4eb3dad..adece2af8 100644
--- a/servers/vfs/main.c
+++ b/servers/vfs/main.c
@@ -42,6 +42,7 @@ EXTERN unsigned long calls_stats[NCALLS];
 /* Thread related prototypes */
 static void *do_async_dev_result(void *arg);
 static void *do_control_msgs(void *arg);
+static void *do_dev_event(void *arg);
 static void *do_fs_reply(struct job *job);
 static void *do_work(void *arg);
 static void *do_pm(void *arg);
@@ -110,7 +111,9 @@ int main(void)
 	} else if (is_notify(call_nr)) {
 		/* A task notify()ed us */
 		if (who_e == DS_PROC_NR)
-			worker_start(ds_event);
+			handle_work(ds_event);
+		else if (fp != NULL && (fp->fp_flags & FP_SRV_PROC))
+			handle_work(do_dev_event);
 		else
 			sys_worker_start(do_control_msgs);
 		continue;
@@ -170,37 +173,37 @@ static void handle_work(void *(*func)(void *arg))
 
   proc_e = m_in.m_source;
 
-  if (fp->fp_flags & FP_SYS_PROC) {
+  if (fp->fp_flags & FP_SRV_PROC) {
+	vmp = find_vmnt(proc_e);
+	if (vmp != NULL) {
+		/* A call back or dev result from an FS
+		 * endpoint. Set call back flag. Can do only
+		 * one call back at a time.
+		 */
+		if (vmp->m_flags & VMNT_CALLBACK) {
+			reply(proc_e, EAGAIN);
+			return;
+		}
+		vmp->m_flags |= VMNT_CALLBACK;
+		if (vmp->m_flags & VMNT_MOUNTING) {
+			vmp->m_flags |= VMNT_FORCEROOTBSF;
+		}
+	}
+
 	if (worker_available() == 0) {
 		if (!deadlock_resolving) {
-			if ((vmp = find_vmnt(proc_e)) != NULL) {
-				/* A call back or dev result from an FS
-				 * endpoint. Set call back flag. Can do only
-				 * one call back at a time.
-				 */
-				if (vmp->m_flags & VMNT_CALLBACK) {
-					reply(proc_e, EAGAIN);
-					return;
-				}
-				vmp->m_flags |= VMNT_CALLBACK;
-
-				/* When an FS endpoint has to make a call back
-				 * in order to mount, force its device to a
-				 * "none device" so block reads/writes will be
-				 * handled by ROOT_FS_E.
-				 */
-				if (vmp->m_flags & VMNT_MOUNTING)
-					vmp->m_flags |= VMNT_FORCEROOTBSF;
-			}
 			deadlock_resolving = 1;
 			dl_worker_start(func);
 			return;
 		}
-		/* Already trying to resolve a deadlock, can't
-		 * handle more, sorry */
 
-		reply(proc_e, EAGAIN);
-		return;
+		if (vmp != NULL) {
+			/* Already trying to resolve a deadlock, can't
+			 * handle more, sorry */
+
+			reply(proc_e, EAGAIN);
+			return;
+		}
 	}
   }
 
@@ -243,19 +246,7 @@ static void *do_async_dev_result(void *arg)
 	select_reply2(job_m_in.m_source, job_m_in.DEV_MINOR,
 		      job_m_in.DEV_SEL_OPS);
 
-  if (deadlock_resolving) {
-	if (fp != NULL && fp->fp_wtid == dl_worker.w_tid)
-		deadlock_resolving = 0;
-  }
-
-  if (fp != NULL && (fp->fp_flags & FP_SYS_PROC)) {
-	struct vmnt *vmp;
-
-	if ((vmp = find_vmnt(fp->fp_endpoint)) != NULL)
-		vmp->m_flags &= ~VMNT_CALLBACK;
-  }
-
-  thread_cleanup(NULL);
+  thread_cleanup(fp);
   return(NULL);
 }
 
@@ -273,15 +264,29 @@ static void *do_control_msgs(void *arg)
   if (job_m_in.m_source == CLOCK) {
 	/* Alarm timer expired. Used only for select(). Check it. */
 	expire_timers(job_m_in.NOTIFY_TIMESTAMP);
-  } else {
-	/* Device notifies us of an event. */
-	dev_status(job_m_in.m_source);
   }
 
   thread_cleanup(NULL);
   return(NULL);
 }
 
+/*===========================================================================*
+ *			       do_dev_event				     *
+ *===========================================================================*/
+static void *do_dev_event(void *arg)
+{
+/* Device notifies us of an event. */
+  struct job my_job;
+
+  my_job = *((struct job *) arg);
+  fp = my_job.j_fp;
+
+  dev_status(job_m_in.m_source);
+
+  thread_cleanup(fp);
+  return(NULL);
+}
+
 /*===========================================================================*
  *			       do_fs_reply				     *
  *===========================================================================*/
@@ -384,8 +389,8 @@ static void *do_pending_pipe(void *arg)
 	reply(fp->fp_endpoint, r);
 
   unlock_filp(f);
-
   thread_cleanup(fp);
+  unlock_proc(fp);
   return(NULL);
 }
 
@@ -402,6 +407,7 @@ void *do_dummy(void *arg)
 
   if ((r = mutex_trylock(&fp->fp_lock)) == 0) {
 	thread_cleanup(fp);
+	unlock_proc(fp);
   } else {
 	/* Proc is busy, let that worker thread carry out the work */
 	thread_cleanup(NULL);
@@ -456,24 +462,10 @@ static void *do_work(void *arg)
   }
 
   /* Copy the results back to the user and send reply. */
-  if (error != SUSPEND) {
-
-	if ((fp->fp_flags & FP_SYS_PROC)) {
-		struct vmnt *vmp;
-
-		if ((vmp = find_vmnt(fp->fp_endpoint)) != NULL)
-			vmp->m_flags &= ~VMNT_CALLBACK;
-	}
-
-	if (deadlock_resolving) {
-		if (fp->fp_wtid == dl_worker.w_tid)
-			deadlock_resolving = 0;
-	}
-
-	reply(fp->fp_endpoint, error);
-  }
+  if (error != SUSPEND) reply(fp->fp_endpoint, error);
 
   thread_cleanup(fp);
+  unlock_proc(fp);
   return(NULL);
 }
 
@@ -635,6 +627,7 @@ static void *do_init_root(void *arg)
 
   unlock_pm();
   thread_cleanup(fp);
+  unlock_proc(fp);
   return(NULL);
 }
 
@@ -710,7 +703,18 @@ void thread_cleanup(struct fproc *rfp)
 
   if (rfp != NULL) {
 	rfp->fp_flags &= ~FP_DROP_WORK;
-	unlock_proc(rfp);
+	if (rfp->fp_flags & FP_SRV_PROC) {
+		struct vmnt *vmp;
+
+		if ((vmp = find_vmnt(rfp->fp_endpoint)) != NULL) {
+			vmp->m_flags &= ~VMNT_CALLBACK;
+		}
+	}
+  }
+
+  if (deadlock_resolving) {
+	if (self->w_tid == dl_worker.w_tid)
+		deadlock_resolving = 0;
   }
 }
 
diff --git a/servers/vfs/mount.c b/servers/vfs/mount.c
index 217dba891..7956c24fb 100644
--- a/servers/vfs/mount.c
+++ b/servers/vfs/mount.c
@@ -266,7 +266,7 @@ char mount_label[LABEL_MAX] )
 	return(EINVAL);
   }
   rfp = &fproc[slot];
-  rfp->fp_flags |= FP_SYS_PROC;	/* Process is an FS */
+  rfp->fp_flags |= FP_SRV_PROC;	/* File Servers are also services */
 
   /* Store some essential vmnt data first */
   new_vmp->m_fs_e = fs_e;
@@ -407,7 +407,6 @@ void mount_pfs(void)
   strlcpy(vmp->m_label, "pfs", LABEL_MAX);
 
   rfp = &fproc[_ENDPOINT_P(PFS_PROC_NR)];
-  rfp->fp_flags |= FP_SYS_PROC;	/* PFS is a driver and an FS */
 }
 
 /*===========================================================================*