On Thu, Mar 09, 2017 at 12:42:43PM +0100, Martin Pieuchot wrote:
> On 09/03/17(Thu) 08:48, Stefan Sperling wrote:
> > This diff converts a struct ifnet pointer in pfsync's softc into an
> > ifindex with corresponding if_get()/if_put() calls.
>
> This avoid the panic but obfuscate the problem. What you want is a
> detachhook such that if the parent interface is destroyed you don't
> leave a sale pointer.
>
> You can look at how vlan(4) or other pseudo drivers do it.
Right. The diff below fixes the issue, too.
vlan(4) uses both a detachhook and if_get/if_put.
Fixing the crash is important so I would commit this fix separately first.
I could later redo my if_get/if_put diff on top unless that is not wanted.
Index: if_pfsync.c
===================================================================
RCS file: /cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.245
diff -u -p -r1.245 if_pfsync.c
--- if_pfsync.c 20 Feb 2017 06:30:39 -0000 1.245
+++ if_pfsync.c 10 Mar 2017 08:58:18 -0000
@@ -234,6 +234,7 @@ struct pfsync_softc {
TAILQ_HEAD(, tdb) sc_tdb_q;
void *sc_lhcookie;
+ void *sc_dhcookie;
struct timeout sc_tmo;
};
@@ -252,6 +253,7 @@ int pfsyncoutput(struct ifnet *, struct
int pfsyncioctl(struct ifnet *, u_long, caddr_t);
void pfsyncstart(struct ifnet *);
void pfsync_syncdev_state(void *);
+void pfsync_ifdetach(void *);
void pfsync_deferred(struct pf_state *, int);
void pfsync_undefer(struct pfsync_deferral *, int);
@@ -365,10 +367,13 @@ pfsync_clone_destroy(struct ifnet *ifp)
if (sc->sc_link_demoted)
carp_group_demote_adj(&sc->sc_if, -1, "pfsync destroy");
#endif
- if (sc->sc_sync_if)
+ if (sc->sc_sync_if) {
hook_disestablish(
sc->sc_sync_if->if_linkstatehooks,
sc->sc_lhcookie);
+ hook_disestablish(sc->sc_sync_if->if_detachhooks,
+ sc->sc_dhcookie);;
+ }
if_detach(ifp);
pfsync_drop(sc);
@@ -427,6 +432,14 @@ pfsync_syncdev_state(void *arg)
}
}
+void
+pfsync_ifdetach(void *arg)
+{
+ struct pfsync_softc *sc = arg;
+
+ sc->sc_sync_if = NULL;
+}
+
int
pfsync_alloc_scrub_memory(struct pfsync_state_peer *s,
struct pf_state_peer *d)
@@ -1313,10 +1326,14 @@ pfsyncioctl(struct ifnet *ifp, u_long cm
sc->sc_defer = pfsyncr.pfsyncr_defer;
if (pfsyncr.pfsyncr_syncdev[0] == 0) {
- if (sc->sc_sync_if)
+ if (sc->sc_sync_if) {
hook_disestablish(
sc->sc_sync_if->if_linkstatehooks,
sc->sc_lhcookie);
+ hook_disestablish(
+ sc->sc_sync_if->if_detachhooks,
+ sc->sc_dhcookie);;
+ }
sc->sc_sync_if = NULL;
if (imo->imo_num_memberships > 0) {
in_delmulti(imo->imo_membership[
@@ -1338,10 +1355,14 @@ pfsyncioctl(struct ifnet *ifp, u_long cm
sifp->if_mtu < MCLBYTES - sizeof(struct ip))
pfsync_sendout();
- if (sc->sc_sync_if)
+ if (sc->sc_sync_if) {
hook_disestablish(
sc->sc_sync_if->if_linkstatehooks,
sc->sc_lhcookie);
+ hook_disestablish(
+ sc->sc_sync_if->if_detachhooks,
+ sc->sc_dhcookie);;
+ }
sc->sc_sync_if = sifp;
if (imo->imo_num_memberships > 0) {
@@ -1388,6 +1409,8 @@ pfsyncioctl(struct ifnet *ifp, u_long cm
sc->sc_lhcookie =
hook_establish(sc->sc_sync_if->if_linkstatehooks, 1,
pfsync_syncdev_state, sc);
+ sc->sc_dhcookie = hook_establish(sc->sc_sync_if->if_detachhooks,
+ 0, pfsync_ifdetach, sc);
pfsync_request_full_update(sc);
splx(s);
