svn commit: r226209 - stable/9/sys/rpc/rpcsec_gss

[Rick Macklem]

Author: rmacklem
Date: Mon Oct 10 13:51:21 2011
New Revision: 226209
URL: http://svn.freebsd.org/changeset/base/226209

Log:
  MFC: r226081, r226104
  A crash reported on freebsd-fs@ on Sep. 23, 2011 under the subject
  heading "kernel panics with RPCSEC_GSS" appears to be caused by a
  corrupted tailq list for the client structure. Looking at the code, calls
  to the function svc_rpc_gss_forget_client() were done in an SMP unsafe
  manner, with the svc_rpc_gss_lock only being acquired in the function
  and not before it. As such, when multiple threads called
  svc_rpc_gss_forget_client() concurrently, it could try and remove the
  same client structure from the tailq lists multiple times.
  The patch fixes this by moving the critical code into a separate
  function called svc_rpc_gss_forget_client_locked(), which must be
  called with the lock held. For the one case where the caller would
  have no interest in the lock, svc_rpc_gss_forget_client() was retained,
  but a loop was added to check that the client structure is still in
  the tailq lists before removing it, to make it safe for multiple
  concurrent calls.
  Also, remove an extraneous "already" from a comment introduced by r226081.
  
  Approved by:	re (kib)

Modified:
  stable/9/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
Directory Properties:
  stable/9/sys/   (props changed)
  stable/9/sys/amd64/include/xen/   (props changed)
  stable/9/sys/boot/   (props changed)
  stable/9/sys/boot/i386/efi/   (props changed)
  stable/9/sys/boot/ia64/efi/   (props changed)
  stable/9/sys/boot/ia64/ski/   (props changed)
  stable/9/sys/boot/powerpc/boot1.chrp/   (props changed)
  stable/9/sys/boot/powerpc/ofw/   (props changed)
  stable/9/sys/cddl/contrib/opensolaris/   (props changed)
  stable/9/sys/conf/   (props changed)
  stable/9/sys/contrib/dev/acpica/   (props changed)
  stable/9/sys/contrib/octeon-sdk/   (props changed)
  stable/9/sys/contrib/pf/   (props changed)
  stable/9/sys/contrib/x86emu/   (props changed)

Modified: stable/9/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
==============================================================================
--- stable/9/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c	Mon Oct 10 13:26:53 2011	(r226208)
+++ stable/9/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c	Mon Oct 10 13:51:21 2011	(r226209)
@@ -609,27 +609,52 @@ svc_rpc_gss_release_client(struct svc_rp
 }
 
 /*
- * Remove a client from our global lists and free it if we can.
+ * Remove a client from our global lists.
+ * Must be called with svc_rpc_gss_lock held.
  */
 static void
-svc_rpc_gss_forget_client(struct svc_rpc_gss_client *client)
+svc_rpc_gss_forget_client_locked(struct svc_rpc_gss_client *client)
 {
 	struct svc_rpc_gss_client_list *list;
 
+	sx_assert(&svc_rpc_gss_lock, SX_XLOCKED);
 	list = &svc_rpc_gss_client_hash[client->cl_id.ci_id % CLIENT_HASH_SIZE];
-	sx_xlock(&svc_rpc_gss_lock);
 	TAILQ_REMOVE(list, client, cl_link);
 	TAILQ_REMOVE(&svc_rpc_gss_clients, client, cl_alllink);
 	svc_rpc_gss_client_count--;
+}
+
+/*
+ * Remove a client from our global lists and free it if we can.
+ */
+static void
+svc_rpc_gss_forget_client(struct svc_rpc_gss_client *client)
+{
+	struct svc_rpc_gss_client_list *list;
+	struct svc_rpc_gss_client *tclient;
+
+	list = &svc_rpc_gss_client_hash[client->cl_id.ci_id % CLIENT_HASH_SIZE];
+	sx_xlock(&svc_rpc_gss_lock);
+	TAILQ_FOREACH(tclient, list, cl_link) {
+		/*
+		 * Make sure this client has not already been removed
+		 * from the lists by svc_rpc_gss_forget_client() or
+		 * svc_rpc_gss_forget_client_locked().
+		 */
+		if (client == tclient) {
+			svc_rpc_gss_forget_client_locked(client);
+			sx_xunlock(&svc_rpc_gss_lock);
+			svc_rpc_gss_release_client(client);
+			return;
+		}
+	}
 	sx_xunlock(&svc_rpc_gss_lock);
-	svc_rpc_gss_release_client(client);
 }
 
 static void
 svc_rpc_gss_timeout_clients(void)
 {
 	struct svc_rpc_gss_client *client;
-	struct svc_rpc_gss_client *nclient;
 	time_t now = time_uptime;
 
 	rpc_gss_log_debug("in svc_rpc_gss_timeout_clients()");
@@ -638,16 +663,29 @@ svc_rpc_gss_timeout_clients(void)
 	 * First enforce the max client limit. We keep
 	 * svc_rpc_gss_clients in LRU order.
 	 */
-	while (svc_rpc_gss_client_count > CLIENT_MAX)
-		svc_rpc_gss_forget_client(TAILQ_LAST(&svc_rpc_gss_clients,
-			    svc_rpc_gss_client_list));
-	TAILQ_FOREACH_SAFE(client, &svc_rpc_gss_clients, cl_alllink, nclient) {
+	sx_xlock(&svc_rpc_gss_lock);
+	client = TAILQ_LAST(&svc_rpc_gss_clients, svc_rpc_gss_client_list);
+	while (svc_rpc_gss_client_count > CLIENT_MAX && client != NULL) {
+		svc_rpc_gss_forget_client_locked(client);
+		sx_xunlock(&svc_rpc_gss_lock);
+		svc_rpc_gss_release_client(client);
+		sx_xlock(&svc_rpc_gss_lock);
+		client = TAILQ_LAST(&svc_rpc_gss_clients,
+		    svc_rpc_gss_client_list);
+	}
+again:
+	TAILQ_FOREACH(client, &svc_rpc_gss_clients, cl_alllink) {
 		if (client->cl_state == CLIENT_STALE
 		    || now > client->cl_expiration) {
+			svc_rpc_gss_forget_client_locked(client);
+			sx_xunlock(&svc_rpc_gss_lock);
 			rpc_gss_log_debug("expiring client %p", client);
-			svc_rpc_gss_forget_client(client);
+			svc_rpc_gss_release_client(client);
+			sx_xlock(&svc_rpc_gss_lock);
+			goto again;
 		}
 	}
+	sx_xunlock(&svc_rpc_gss_lock);
 }
 
 #ifdef DEBUG