svn commit: r279234 - head/sys/netipsec

[Andrey V. Elsukov]

Author: ae
Date: Tue Feb 24 10:35:07 2015
New Revision: 279234
URL: https://svnweb.freebsd.org/changeset/base/279234

Log:
  Fix possible memory leak and several races in the IPsec policy management
  code.
  
  Resurrect the state field in the struct secpolicy, it has
  IPSEC_SPSTATE_ALIVE value when security policy linked in the chain,
  and IPSEC_SPSTATE_DEAD value in all other cases. This field protects
  from trying to unlink one security policy several times from the different
  threads.
  
  Take additional reference in the key_flush_spd() to be sure that policy
  won't be freed from the different thread while we are sending SPDEXPIRE message.
  
  Add KEY_FREESP() call to the key_unlink() to release additional reference
  that we take when use key_getsp*() functions.
  
  Differential Revision:	https://reviews.freebsd.org/D1914
  Tested by:		Emeric POUPON <emeric.poupon at stormshield dot eu>
  Reviewed by:	hrs
  Sponsored by:	Yandex LLC

Modified:
  head/sys/netipsec/ipsec.h
  head/sys/netipsec/key.c

Modified: head/sys/netipsec/ipsec.h
==============================================================================
--- head/sys/netipsec/ipsec.h	Tue Feb 24 08:53:47 2015	(r279233)
+++ head/sys/netipsec/ipsec.h	Tue Feb 24 10:35:07 2015	(r279234)
@@ -89,6 +89,9 @@ struct secpolicy {
 				/* if policy == IPSEC else this value == NULL.*/
 	u_int refcnt;			/* reference count */
 	u_int policy;			/* policy_type per pfkeyv2.h */
+	u_int state;
+#define	IPSEC_SPSTATE_DEAD	0
+#define	IPSEC_SPSTATE_ALIVE	1
 	u_int32_t id;			/* It's unique number on the system. */
 	/*
 	 * lifetime handler.

Modified: head/sys/netipsec/key.c
==============================================================================
--- head/sys/netipsec/key.c	Tue Feb 24 08:53:47 2015	(r279233)
+++ head/sys/netipsec/key.c	Tue Feb 24 10:35:07 2015	(r279234)
@@ -1198,8 +1198,14 @@ key_unlink(struct secpolicy *sp)
 	SPTREE_UNLOCK_ASSERT();
 
 	SPTREE_WLOCK();
+	if (sp->state == IPSEC_SPSTATE_DEAD) {
+		SPTREE_WUNLOCK();
+		return;
+	}
+	sp->state = IPSEC_SPSTATE_DEAD;
 	TAILQ_REMOVE(&V_sptree[sp->spidx.dir], sp, chain);
 	SPTREE_WUNLOCK();
+	KEY_FREESP(&sp);
 }
 
 /*
@@ -1900,6 +1906,7 @@ key_spdadd(struct socket *so, struct mbu
 
 	SPTREE_WLOCK();
 	TAILQ_INSERT_TAIL(&V_sptree[newsp->spidx.dir], newsp, chain);
+	newsp->state = IPSEC_SPSTATE_ALIVE;
 	SPTREE_WUNLOCK();
 
 	/* delete the entry in spacqtree */
@@ -2335,6 +2342,12 @@ key_spdflush(struct socket *so, struct m
 	for (dir = 0; dir < IPSEC_DIR_MAX; dir++) {
 		TAILQ_CONCAT(&drainq, &V_sptree[dir], chain);
 	}
+	/*
+	 * We need to set state to DEAD for each policy to be sure,
+	 * that another thread won't try to unlink it.
+	 */
+	TAILQ_FOREACH(sp, &drainq, chain)
+		sp->state = IPSEC_SPSTATE_DEAD;
 	SPTREE_WUNLOCK();
 	sp = TAILQ_FIRST(&drainq);
 	while (sp != NULL) {
@@ -4209,9 +4222,10 @@ restart:
 			    now - sp->created > sp->lifetime) ||
 			    (sp->validtime &&
 			    now - sp->lastused > sp->validtime)) {
+				SP_ADDREF(sp);
 				SPTREE_RUNLOCK();
-				key_unlink(sp);
 				key_spdexpire(sp);
+				key_unlink(sp);
 				KEY_FREESP(&sp);
 				goto restart;
 			}