Date: Mon, 10 Oct 2011 14:35:26 +0000 (UTC) From: Rick Macklem <rmacklem@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org Subject: svn commit: r226213 - stable/8/sys/rpc/rpcsec_gss Message-ID: <201110101435.p9AEZQoT064907@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: rmacklem Date: Mon Oct 10 14:35:26 2011 New Revision: 226213 URL: http://svn.freebsd.org/changeset/base/226213 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. Modified: stable/8/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c Directory Properties: stable/8/sys/ (props changed) stable/8/sys/amd64/include/xen/ (props changed) stable/8/sys/cddl/contrib/opensolaris/ (props changed) stable/8/sys/contrib/dev/acpica/ (props changed) stable/8/sys/contrib/pf/ (props changed) Modified: stable/8/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c ============================================================================== --- stable/8/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c Mon Oct 10 13:59:33 2011 (r226212) +++ stable/8/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c Mon Oct 10 14:35:26 2011 (r226213) @@ -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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201110101435.p9AEZQoT064907>