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
