somewhat reproducable vimage panic

[Bjoern A. Zeeb]

On 23 Jul 2020, at 8:09, Kristof Provost wrote:

> On 23 Jul 2020, at 9:19, Kristof Provost wrote:
>> On 23 Jul 2020, at 0:15, John-Mark Gurney wrote:
>>> So, it's pretty easy to trigger, just attach a couple USB ethernet
>>> adapters, in my case, they were ure, but likely any two spare 
>>> ethernet
>>> interfaces will work, and wire them back to back..
>>>
>> I’ve been able to trigger it using epair as well:
>>
>> `sudo sh testinterfaces.txt epair0a epair0b`
>>
>> I did have to comment out the waitcarrier() check.
>>
> I’ve done a little bit of digging, and I think I’m starting to see 
> how this breaks.
>
> This always affects the jailed vlan interfaces. They’re getting 
> deleted, but the ifp doesn’t go away just yet because it’s still 
> in use by the multicast code.
> The multicast code does its cleanup in task queues,

Wow, did I miss that back then? Did I review a change and not notice? 
Sorry if that was the case.

Vnet teardown is blocking and forceful.
Doing deferred cleanup work isn’t a good idea at all.
I think that is the real problem here.

I’d rather have us fix this than putting more bandaids into the code.

/bz

PS:  I love that you can repro this with epairs, means we can write a 
generic test code piece for this and commit it.


> so by the time it gets around to doing that the ifp is already marked 
> as dying and the vnet is gone.
> There are still references to the ifp though, and when the multicast 
> code tries to do its cleanup we get the panic.
>
> This hack stops the panic for me, but I don’t know if this is the 
> best solution:
>
> 	diff --git a/sys/net/if.c b/sys/net/if.c
> 	index 59dd38267cf..bd0c87eddf1 100644
> 	--- a/sys/net/if.c
> 	+++ b/sys/net/if.c
> 	@@ -3681,6 +3685,10 @@ if_delmulti_ifma_flags(struct ifmultiaddr 
> *ifma, int flags)
> 	 			ifp = NULL;
> 	 	}
> 	 #endif
> 	+
> 	+	if (ifp && ifp->if_flags & IFF_DYING)
> 	+		return;
> 	+
> 	 	/*
> 	 	 * If and only if the ifnet instance exists: Acquire the address 
> lock.
> 	 	 */
> 	diff --git a/sys/netinet/in_mcast.c b/sys/netinet/in_mcast.c
> 	index 39fc82c5372..6493e2a5bfb 100644
> 	--- a/sys/netinet/in_mcast.c
> 	+++ b/sys/netinet/in_mcast.c
> 	@@ -623,7 +623,7 @@ inm_release(struct in_multi *inm)
>
> 	 	/* XXX this access is not covered by IF_ADDR_LOCK */
> 	 	CTR2(KTR_IGMPV3, "%s: purging ifma %p", __func__, ifma);
> 	-	if (ifp != NULL) {
> 	+	if (ifp != NULL && (ifp->if_flags & IFF_DYING) == 0) {
> 	 		CURVNET_SET(ifp->if_vnet);
> 	 		inm_purge(inm);
> 	 		free(inm, M_IPMADDR);
>
> Best regards,
> Kristof