Notify that the ifnet will go away, even on vnet shutdown

[kp]

Summary

pf subscribes to ifnet_departure_event events, so it can clean up the
ifg_pf_kif and if_pf_kif pointers in the ifnet.
During vnet shutdown interfaces could go away without sending the event,
so pf ends up cleaning these up as part of its shutdown sequence, which
happens after the ifnet has already been freed.

Send the ifnet_departure_event during vnet shutdown, allowing pf to
clean up correctly.

[kp]

Test plan:
kldload pfsync
cd /usr/tests/sys/netpfil/pf
kyua test

This panics with:
Memory modified after free 0xfffff800073ba800(2040) val=0 @ 0xfffff800073bab98
panic: Most recently used by ifnet

The issue is that pf keeps information in the struct ifnet (if_of_kif) and expects to get told when interfaces go away so it can clean that up.
If they're not cleaned up before the jail is torn down we try to from pfi_cleanup_vnet() (through of_unload_vnet()), but at that point the struct ifnet no longer exists.

[bz]

Still wonder if pf should hold a reference to the interface as well.

[kp]

The following also resolves the use-after-free issue.

diff --git a/sys/netpfil/pf/pf_if.c b/sys/netpfil/pf/pf_if.c
index 5b4446d68db..17ad894bc87 100644
--- a/sys/netpfil/pf/pf_if.c
+++ b/sys/netpfil/pf/pf_if.c
@@ -165,8 +165,10 @@ pfi_cleanup_vnet(void)
                RB_REMOVE(pfi_ifhead, &V_pfi_ifs, kif);
                if (kif->pfik_group)
                        kif->pfik_group->ifg_pf_kif = NULL;
-               if (kif->pfik_ifp)
+               if (kif->pfik_ifp) {
+                       if_rele(kif->pfik_ifp);
                        kif->pfik_ifp->if_pf_kif = NULL;
+               }
                free(kif, PFI_MTYPE);
        }

@@ -322,6 +324,8 @@ pfi_attach_ifnet(struct ifnet *ifp)
        V_pfi_update++;
        kif = pfi_kif_attach(kif, ifp->if_xname);

+       if_ref(ifp);
+
        kif->pfik_ifp = ifp;
        ifp->if_pf_kif = kif;

@@ -848,6 +852,8 @@ pfi_detach_ifnet_event(void *arg __unused, struct ifnet *ifp)
        V_pfi_update++;
        pfi_kif_update(kif);

+       if_rele(kif->pfik_ifp);
+
        kif->pfik_ifp = NULL;
        ifp->if_pf_kif = NULL;
 #ifdef ALTQ

That makes sense, as the struct ifnet won't get freed until we say we're done with it. Both the original patch and the one above individually solve the issue.
I'm not sure if we want one or the other, or both.

[bz]

Maybe both? We also don't want to not get notified and have an interface hanging around for ever given pf never releases a reference?
In the first block of you patch you might want to switch the order and set the kif to NULL before releasing the reference.

[kp]

We also don't want to not get notified and have an interface hanging around for ever given pf never releases a reference?

pf will still clean up those things on (vnet) shutdown (via pfi_cleanup_vnet() and of_unload_vnet()), so we're not going to leak references one way or the other.