From owner-freebsd-net@FreeBSD.ORG Fri Jun 19 12:37:17 2009 Return-Path: Delivered-To: freebsd-net@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8FA9F1065670 for ; Fri, 19 Jun 2009 12:37:17 +0000 (UTC) (envelope-from vanhu@zeninc.net) Received: from smtp.zeninc.net (smtp.zeninc.net [80.67.176.25]) by mx1.freebsd.org (Postfix) with ESMTP id 115328FC08 for ; Fri, 19 Jun 2009 12:37:17 +0000 (UTC) (envelope-from vanhu@zeninc.net) Received: from astro.zen.inc (astro.zen.inc [192.168.1.239]) by smtp.zeninc.net (smtpd) with ESMTP id 0AA0B2798B8 for ; Fri, 19 Jun 2009 14:37:15 +0200 (CEST) Received: by astro.zen.inc (Postfix, from userid 1000) id 2B57017024; Fri, 19 Jun 2009 15:00:41 +0200 (CEST) Date: Fri, 19 Jun 2009 15:00:41 +0200 From: VANHULLEBUS Yvan To: freebsd-net@FreeBSD.org Message-ID: <20090619130040.GA53996@zeninc.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="82I3+IH0IqGh5yIs" Content-Disposition: inline Cc: Subject: IPsec crash, patch for review X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Jun 2009 12:37:17 -0000 --82I3+IH0IqGh5yIs Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi all. We (NETASQ) had some IPsec related kernel crashes, and hunted them, here are some informations and a possible patch: First, problem only occurs when asynchronous crypto is done (hardware encryption such as hifn cards, or software patch to do encryption on a separate kthread when having multiple CPUs). More or less exploitable backtraces always shows a problem with an isr which seems to be used but already freed. While looking at the code, we noticed that there is probably a race condition when using asynchronous crypto: ip_output (well, ip_ipsec_output() now) releases refcnt to the SPD entry when IPsec processing needs to be done, but SPD's isr will still be used by [esp|ah|ipcomp]_output_cb(). The attached patch moves the KEY_FREESP() from ip_ipsec_output() to [esp|ah|ipcomp]_output_cb() when there will be IPsec processing (when ip_ipsec_output()returns -1). (note for George and Bjoern: this is a newer version of the patch, which also fixes a refcnt leak). This patches solves our problem (no more crashes for a week, when we had at least 2-3 crashes / day), and has also successfully passed our non regression testsuite. As it may still have some unexpected impacts on other parts of the code, I'd like to have some feedback on it before commiting it. Thanks, Yvan. --82I3+IH0IqGh5yIs Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=patch-xform_cb-freesp Index: netinet/ip_ipsec.c =================================================================== --- netinet/ip_ipsec.c (revision 193698) +++ netinet/ip_ipsec.c (working copy) @@ -405,8 +405,6 @@ done: KEY_FREESP(&sp); return 0; reinjected: - if (sp != NULL) - KEY_FREESP(&sp); return -1; bad: if (sp != NULL) Index: netipsec/xform_esp.c =================================================================== --- netipsec/xform_esp.c (revision 193698) +++ netipsec/xform_esp.c (working copy) @@ -979,11 +979,15 @@ esp_output_cb(struct cryptop *crp) err = ipsec_process_done(m, isr); KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + if(isr->sp != NULL) /* should NEVER be NULL !!! */ + KEY_FREESP(isr->sp); return err; bad: if (sav) KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + if(isr->sp != NULL) /* should NEVER be NULL !!! */ + KEY_FREESP(isr->sp); if (m) m_freem(m); free(tc, M_XDATA); Index: netipsec/xform_ipcomp.c =================================================================== --- netipsec/xform_ipcomp.c (revision 193698) +++ netipsec/xform_ipcomp.c (working copy) @@ -588,11 +588,15 @@ ipcomp_output_cb(struct cryptop *crp) error = ipsec_process_done(m, isr); KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + if(isr->sp != NULL) /* should NEVER be NULL !!! */ + KEY_FREESP(isr->sp); return error; bad: if (sav) KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + if(isr->sp != NULL) /* should NEVER be NULL !!! */ + KEY_FREESP(isr->sp); if (m) m_freem(m); free(tc, M_XDATA); Index: netipsec/xform_ah.c =================================================================== --- netipsec/xform_ah.c (revision 193698) +++ netipsec/xform_ah.c (working copy) @@ -1210,11 +1210,15 @@ ah_output_cb(struct cryptop *crp) err = ipsec_process_done(m, isr); KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + if(isr->sp != NULL) /* should NEVER be NULL !!! */ + KEY_FREESP(isr->sp); return err; bad: if (sav) KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + if(isr->sp != NULL) /* should NEVER be NULL !!! */ + KEY_FREESP(isr->sp); if (m) m_freem(m); free(tc, M_XDATA); --82I3+IH0IqGh5yIs Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=patch-xform_freespfix-3 Index: netinet/ip_ipsec.c =================================================================== --- netinet/ip_ipsec.c (revision 194337) +++ netinet/ip_ipsec.c (working copy) @@ -347,6 +347,11 @@ ip_ipsec_output(struct mbuf **m, struct /* NB: callee frees mbuf */ *error = ipsec4_process_packet(*m, sp->req, *flags, 0); + + /* Release SP now if an error occured (including acquire) */ + if(error) + KEY_FREESP(&sp); + if (*error == EJUSTRETURN) { /* * We had a SP with a level of 'use' and no SA. We @@ -358,6 +363,7 @@ ip_ipsec_output(struct mbuf **m, struct ip->ip_off = ntohs(ip->ip_off); goto done; } + /* * Preserve KAME behaviour: ENOENT can be returned * when an SA acquire is in progress. Don't propagate @@ -405,8 +411,6 @@ done: KEY_FREESP(&sp); return 0; reinjected: - if (sp != NULL) - KEY_FREESP(&sp); return -1; bad: if (sp != NULL) Index: netipsec/xform_esp.c =================================================================== --- netipsec/xform_esp.c (revision 194337) +++ netipsec/xform_esp.c (working copy) @@ -979,11 +979,15 @@ esp_output_cb(struct cryptop *crp) err = ipsec_process_done(m, isr); KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp")); + KEY_FREESP(&isr->sp); return err; bad: if (sav) KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp")); + KEY_FREESP(&isr->sp); if (m) m_freem(m); free(tc, M_XDATA); Index: netipsec/xform_ipcomp.c =================================================================== --- netipsec/xform_ipcomp.c (revision 194337) +++ netipsec/xform_ipcomp.c (working copy) @@ -588,11 +588,15 @@ ipcomp_output_cb(struct cryptop *crp) error = ipsec_process_done(m, isr); KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp")); + KEY_FREESP(&isr->sp); return error; bad: if (sav) KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp")); + KEY_FREESP(&isr->sp); if (m) m_freem(m); free(tc, M_XDATA); Index: netipsec/xform_ah.c =================================================================== --- netipsec/xform_ah.c (revision 194337) +++ netipsec/xform_ah.c (working copy) @@ -1210,11 +1210,15 @@ ah_output_cb(struct cryptop *crp) err = ipsec_process_done(m, isr); KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp")); + KEY_FREESP(&isr->sp); return err; bad: if (sav) KEY_FREESAV(&sav); IPSECREQUEST_UNLOCK(isr); + IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp")); + KEY_FREESP(&isr->sp); if (m) m_freem(m); free(tc, M_XDATA); --82I3+IH0IqGh5yIs--