On Sat, Nov 13, 2021 at 09:49:31PM +0000, Stuart Henderson wrote: > On 2021/11/13 18:04, Alexander Bluhm wrote: > > Hi, > > > > To make IPsec MP safe we need refcounting for the tdb. The diff > > below is part of something bigger we have at genua. Although it > > does not cover timeouts and the tdb reaper yet, I want to get this > > in as a frist step. > > > > It passes regress but there are setups that are not covered. Bridge > > and pfsync with IPsec and TCP signature need special care. > > > > When testing, please check for tdb leaks. The vmstat -m tdb in use > > culumn must be 0. It it is not, try ipsecctl -F. If ipsecctl -sa > > shows nothing, but vmstat -m shows a used tdb, then it is a bug. > > > > Name Size Requests Fail InUse Pgreq Pgrel Npage Hiwat Minpg Maxpg > > Idle > > tdb 904 56 0 0 7 7 0 7 0 8 > > 0 > > Tested with a box running isakmpd at home. Config is: > > isakmpd_flags=-Kv -D0=29 -D1=49 -D2=10 -D3=30 -D5=20 -D6=30 -D8=30 -D9=30 > -D10=20 > > ike passive esp \ > from xxx.xx.xxx.1 (xxx.xx.xxx.0/27) to 0.0.0.0/0 \ > local xxx.xx.xxx.1 \ > main auth hmac-sha2-256 enc aes group ecp256 \ > quick enc aes-128-gcm group ecp256 \ > srcid xxx > > isakmpd starts at boot, 3 devices connect in and tunnels come up, > FLOWS and SAD show in ipsecctl -sa. > > isakmpd crashes shortly after starting (as described in previous mails; > this is not new, it happens very often on the first start at boot), > FLOWS and SAD remain showing. At this point they still show InUse: > > Name Size Requests Fail InUse Pgreq Pgrel Npage Hiwat Minpg Maxpg > Idle > tdb 1088 9 0 6 1 0 1 1 0 8 > 0 > > After ipsecctl -F they are freed: > > tdb 1088 9 0 0 1 0 1 1 0 8 > 1 > > Restart isakmpd, ipsecctl -f /etc/ipsec.conf, devices connect back in: > > tdb 1088 18 0 6 2 1 1 1 0 8 > 0 > > (And at this point isakmpd is usually stable). > > I will leave a shell loop running vmstat -m every few minutes to check > if it grows. > > (Bad timing for testing TCPMD5 because over the last couple of days I have > just removed bgpd+ospfd from the machines that would have been good > choices to run test diffs..I might be able to get some simple setup > back up and running though) >
tdb_free() should be called with netlock held. With your diff tdb_unref() is the only called of tdb_free(). I marked the only place where you call tdb_free() without nelock held. > > @@ -2008,10 +2015,12 @@ pfkeyv2_send(struct socket *so, void *me > > (caddr_t)&ipo->ipo_mask, rnh, > > ipo->ipo_nodes, 0)) == NULL) { > > /* Remove from linked list of policies on TDB */ > > - if (ipo->ipo_tdb) > > - > > TAILQ_REMOVE(&ipo->ipo_tdb->tdb_policy_head, > > + if (ipo->ipo_tdb != NULL) { > > + TAILQ_REMOVE( > > + &ipo->ipo_tdb->tdb_policy_head, > > ipo, ipo_tdb_next); > > - > > + tdb_unref(ipo->ipo_tdb); > > + } > > if (ipo->ipo_ids) > > ipsp_ids_free(ipo->ipo_ids); > > pool_put(&ipsec_policy_pool, ipo); > > @@ -2127,6 +2136,7 @@ realret: > > free(message, M_PFKEY, len); > > > > free(sa1, M_PFKEY, sizeof(*sa1)); > > + tdb_unref(sa2); netlock missed here > > > > return (rval); > > } > > Index: netinet/ip_ipsp.c