Date: Mon, 29 Sep 2003 15:08:05 -0700 (PDT) From: Sam Leffler <sam@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 38806 for review Message-ID: <200309292208.h8TM859f009219@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=38806 Change 38806 by sam@sam_ebb on 2003/09/29 15:07:09 general cleanups and reorg to deal with recursive locking problems: o use LIST_FOREACH_SAFE instead of handrolled code o change key_flush_spd to drop the sptree lock before purging an entry to avoid lock recursion and to avoid holding the lock over a long-running operation o misc cleanups of tangled and twisty code There is still much to do here but for now things look to be working again. Affected files ... .. //depot/projects/netperf/sys/netipsec/key.c#10 edit Differences ... ==== //depot/projects/netperf/sys/netipsec/key.c#10 (text+ko) ==== @@ -289,10 +289,6 @@ SYSCTL_INT(_net_key, KEYCTL_PREFERED_OLDSA, prefered_oldsa, CTLFLAG_RW,\ &key_prefered_oldsa, 0, ""); -#ifndef LIST_FOREACH -#define LIST_FOREACH(elm, head, field) \ - for (elm = LIST_FIRST(head); elm; elm = LIST_NEXT(elm, field)) -#endif #define __LIST_CHAINED(elm) \ (!((elm)->chain.le_next == NULL && (elm)->chain.le_prev == NULL)) #define LIST_INSERT_TAIL(head, elm, type, field) \ @@ -1168,6 +1164,7 @@ IPSEC_ASSERT(sav != NULL, ("null sav")); + /* XXX unguarded? */ SA_DELREF(sav); KEYDEBUG(KEYDEBUG_IPSEC_STAMP, @@ -2298,9 +2295,10 @@ return key_senderror(so, m, EINVAL); for (dir = 0; dir < IPSEC_DIR_MAX; dir++) { - LIST_FOREACH(sp, &sptree[dir], chain) { + SPTREE_LOCK(); + LIST_FOREACH(sp, &sptree[dir], chain) sp->state = IPSEC_SPSTATE_DEAD; - } + SPTREE_UNLOCK(); } if (sizeof(struct sadb_msg) > m->m_len + M_TRAILINGSPACE(m)) { @@ -2610,7 +2608,7 @@ struct secashead *sah; { struct secasvar *sav, *nextsav; - u_int stateidx, state; + u_int stateidx; int zombie = 0; IPSEC_ASSERT(sah != NULL, ("NULL sah")); @@ -2620,14 +2618,8 @@ for (stateidx = 0; stateidx < _ARRAYLEN(saorder_state_any); stateidx++) { - - state = saorder_state_any[stateidx]; - for (sav = (struct secasvar *)LIST_FIRST(&sah->savtree[state]); - sav != NULL; - sav = nextsav) { - - nextsav = LIST_NEXT(sav, chain); - + u_int state = saorder_state_any[stateidx]; + LIST_FOREACH_SAFE(sav, &sah->savtree[state], chain, nextsav) { if (sav->refcnt == 0) { /* sanity check */ KEY_CHKSASTATE(state, sav->state, __func__); @@ -2638,20 +2630,16 @@ } } } - /* remove from tree of SA index */ - if (!zombie && __LIST_CHAINED(sah)) - LIST_REMOVE(sah, chain); - - /* don't delete sah only if there are savs. */ - if (zombie) - return; - - if (sah->sa_route.ro_rt) { - RTFREE(sah->sa_route.ro_rt); - sah->sa_route.ro_rt = (struct rtentry *)NULL; + if (!zombie) { /* delete only if there are savs */ + /* remove from tree of SA index */ + if (__LIST_CHAINED(sah)) + LIST_REMOVE(sah, chain); + if (sah->sa_route.ro_rt) { + RTFREE(sah->sa_route.ro_rt); + sah->sa_route.ro_rt = (struct rtentry *)NULL; + } + free(sah, M_IPSEC_SAH); } - - free(sah, M_IPSEC_SAH); } /* @@ -3973,32 +3961,34 @@ static void key_flush_spd(time_t now) { - struct secpolicy *sp, *nextsp; + static u_int16_t sptree_scangen = 0; + u_int16_t gen = sptree_scangen++; + struct secpolicy *sp; u_int dir; /* SPD */ for (dir = 0; dir < IPSEC_DIR_MAX; dir++) { +restart: SPTREE_LOCK(); - for (sp = LIST_FIRST(&sptree[dir]); - sp != NULL; - sp = nextsp) { - - nextsp = LIST_NEXT(sp, chain); - + LIST_FOREACH(sp, &sptree[dir], chain) { + if (sp->scangen == gen) /* previously handled */ + continue; + sp->scangen = gen; if (sp->state == IPSEC_SPSTATE_DEAD) { + /* NB: clean entries created by key_spdflush */ + SPTREE_UNLOCK(); KEY_FREESP(&sp); - continue; + goto restart; } - if (sp->lifetime == 0 && sp->validtime == 0) continue; - - /* the deletion will occur next time */ if ((sp->lifetime && now - sp->created > sp->lifetime) || (sp->validtime && now - sp->lastused > sp->validtime)) { sp->state = IPSEC_SPSTATE_DEAD; + SPTREE_UNLOCK(); key_spdexpire(sp); - continue; + KEY_FREESP(&sp); + goto restart; } } SPTREE_UNLOCK(); @@ -4013,12 +4003,7 @@ /* SAD */ SAHTREE_LOCK(); - for (sah = LIST_FIRST(&sahtree); - sah != NULL; - sah = nextsah) { - - nextsah = LIST_NEXT(sah, chain); - + LIST_FOREACH_SAFE(sah, &sahtree, chain, nextsah) { /* if sah has been dead, then delete it and process next sah. */ if (sah->state == SADB_SASTATE_DEAD) { key_delsah(sah); @@ -4026,27 +4011,16 @@ } /* if LARVAL entry doesn't become MATURE, delete it. */ - for (sav = LIST_FIRST(&sah->savtree[SADB_SASTATE_LARVAL]); - sav != NULL; - sav = nextsav) { - - nextsav = LIST_NEXT(sav, chain); - - if (now - sav->created > key_larval_lifetime) { + LIST_FOREACH_SAFE(sav, &sah->savtree[SADB_SASTATE_LARVAL], chain, nextsav) { + if (now - sav->created > key_larval_lifetime) KEY_FREESAV(&sav); - } } /* * check MATURE entry to start to send expire message * whether or not. */ - for (sav = LIST_FIRST(&sah->savtree[SADB_SASTATE_MATURE]); - sav != NULL; - sav = nextsav) { - - nextsav = LIST_NEXT(sav, chain); - + LIST_FOREACH_SAFE(sav, &sah->savtree[SADB_SASTATE_MATURE], chain, nextsav) { /* we don't need to check. */ if (sav->lft_s == NULL) continue; @@ -4059,8 +4033,8 @@ } /* check SOFT lifetime */ - if (sav->lft_s->sadb_lifetime_addtime != 0 - && now - sav->created > sav->lft_s->sadb_lifetime_addtime) { + if (sav->lft_s->sadb_lifetime_addtime != 0 && + now - sav->created > sav->lft_s->sadb_lifetime_addtime) { /* * check SA to be used whether or not. * when SA hasn't been used, delete it. @@ -4084,8 +4058,8 @@ * when new SA is installed. Caution when it's * installed too big lifetime by time. */ - else if (sav->lft_s->sadb_lifetime_bytes != 0 - && sav->lft_s->sadb_lifetime_bytes < sav->lft_c->sadb_lifetime_bytes) { + else if (sav->lft_s->sadb_lifetime_bytes != 0 && + sav->lft_s->sadb_lifetime_bytes < sav->lft_c->sadb_lifetime_bytes) { key_sa_chgstate(sav, SADB_SASTATE_DYING); /* @@ -4098,12 +4072,7 @@ } /* check DYING entry to change status to DEAD. */ - for (sav = LIST_FIRST(&sah->savtree[SADB_SASTATE_DYING]); - sav != NULL; - sav = nextsav) { - - nextsav = LIST_NEXT(sav, chain); - + LIST_FOREACH_SAFE(sav, &sah->savtree[SADB_SASTATE_DYING], chain, nextsav) { /* we don't need to check. */ if (sav->lft_h == NULL) continue; @@ -4115,8 +4084,8 @@ continue; } - if (sav->lft_h->sadb_lifetime_addtime != 0 - && now - sav->created > sav->lft_h->sadb_lifetime_addtime) { + if (sav->lft_h->sadb_lifetime_addtime != 0 && + now - sav->created > sav->lft_h->sadb_lifetime_addtime) { key_sa_chgstate(sav, SADB_SASTATE_DEAD); KEY_FREESAV(&sav); } @@ -4137,20 +4106,15 @@ } #endif /* check HARD lifetime by bytes */ - else if (sav->lft_h->sadb_lifetime_bytes != 0 - && sav->lft_h->sadb_lifetime_bytes < sav->lft_c->sadb_lifetime_bytes) { + else if (sav->lft_h->sadb_lifetime_bytes != 0 && + sav->lft_h->sadb_lifetime_bytes < sav->lft_c->sadb_lifetime_bytes) { key_sa_chgstate(sav, SADB_SASTATE_DEAD); KEY_FREESAV(&sav); } } /* delete entry in DEAD */ - for (sav = LIST_FIRST(&sah->savtree[SADB_SASTATE_DEAD]); - sav != NULL; - sav = nextsav) { - - nextsav = LIST_NEXT(sav, chain); - + LIST_FOREACH_SAFE(sav, &sah->savtree[SADB_SASTATE_DEAD], chain, nextsav) { /* sanity check */ if (sav->state != SADB_SASTATE_DEAD) { ipseclog((LOG_DEBUG, "%s: invalid sav->state " @@ -4158,7 +4122,6 @@ __func__, SADB_SASTATE_DEAD, sav->state)); } - /* * do not call key_freesav() here. * sav should already be freed, and sav->refcnt
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200309292208.h8TM859f009219>