svn commit: r339554 - head/sys/net

[Kristof Provost]

On 22 Oct 2018, at 17:51, Kristof Provost wrote:
> On 21 Oct 2018, at 11:24, Andrey V. Elsukov wrote:
>> Author: ae
>> Date: Sun Oct 21 18:24:20 2018
>> New Revision: 339554
>> URL: https://svnweb.freebsd.org/changeset/base/339554
>>
>> Log:
>>   Rework if_ipsec(4) to use epoch(9) instead of rmlock.
>>
>>   * use CK_LIST and FNV hash to keep chains of softc;
>>   * read access to softc is protected by epoch();
>>   * write access is protected by ipsec_ioctl_sx. Changing of softc 
>> fields
>>     is allowed only when softc is unlinked from CK_LIST chains.
>>   * linking/unlinking of softc is allowed only when ipsec_ioctl_sx is
>>     exclusive locked.
>>   * the plain LIST of all softc is replaced by hash table that uses 
>> ingress
>>     address of tunnels as a key.
>>   * added support for appearing/disappearing of ingress address 
>> handling.
>>     Now it is allowed configure non-local ingress IP address, and 
>> thus the
>>     problem with if_ipsec(4) configuration that happens on boot, when
>>     ingress address is not yet configured, is solved.
>>
>>   MFC after:	1 month
>>   Sponsored by:	Yandex LLC
>>   Differential Revision:	https://reviews.freebsd.org/D17190
>>
> This panics during the pf tests.
I think I understand what’s happening here.

With this patch I see a different panic:

	diff --git a/sys/net/if_ipsec.c b/sys/net/if_ipsec.c
	index 91b11a455b3..146cb59aaaa 100644
	--- a/sys/net/if_ipsec.c
	+++ b/sys/net/if_ipsec.c
	@@ -134,6 +134,9 @@ ipsec_srchash(const struct sockaddr *sa)
	 {
	        uint32_t hval;

	+       KASSERT(V_ipsec4_srchtbl, ("NULL"));
	+       KASSERT(V_ipsec6_srchtbl, ("NULL (v6)"));
	+
	        switch (sa->sa_family) {
	 #ifdef INET
	        case AF_INET:
	@@ -265,17 +274,22 @@ static void
	 vnet_ipsec_uninit(const void *unused __unused)
	 {

	        if_clone_detach(V_ipsec_cloner);
	        free(V_ipsec_idhtbl, M_IPSEC);
	 #ifdef INET
	        if (IS_DEFAULT_VNET(curvnet))
	                ip_encap_unregister_srcaddr(ipsec4_srctab);
	        free(V_ipsec4_srchtbl, M_IPSEC);
	+       V_ipsec4_srchtbl = NULL;
	+
	 #endif
	 #ifdef INET6
	        if (IS_DEFAULT_VNET(curvnet))
	                ip6_encap_unregister_srcaddr(ipsec6_srctab);
	        free(V_ipsec6_srchtbl, M_IPSEC);
	+       V_ipsec4_srchtbl = NULL;
	 #endif
	 }
	 VNET_SYSUNINIT(vnet_ipsec_uninit, SI_SUB_PROTO_IFATTACHDOMAIN, 
SI_ORDER_ANY,

That trips the KASSERT() in ipsec_srchash(). Basically, the problem is 
that the V_ipsec4_srchtbl table gets freed in vnet_ipsec_uninit(), and 
later we get a srcaddr_change_event(), which tries to iterate the list 
(in ipsec_srcaddr()). Obviously iterating a freed list doesn’t go well 
for us (hence the 0xdeadc0dedeadc0de softc).

That also explains why this happens during the pf tests, even though 
they don’t actually use IPSec.

I’m not quite sure how to best fix this though. I suppose we could set 
V_ipsec4_srchtbl to NULL (well, copy the pointer first, set it to NULL 
and then free it so we don’t add a race condition) and then check for 
NULL in  ipsec_srcaddr().
It feels like the srcaddr_change_event needs to be per-vnet, so we can 
unregister before we free V_ipsec4_srchtbl.

Best regards,
Kristof