On Mon, Oct 01, 2018 at 12:09:25PM +0000, Visa Hankala wrote:
> pfsync_clone_destroy() lacks proper locking and its timeout cancellation
> is not robust. Please try the patch below.
OK bluhm@
> Index: net/if_pfsync.c
> ===================================================================
> RCS file: src/sys/net/if_pfsync.c,v
> retrieving revision 1.259
> diff -u -p -r1.259 if_pfsync.c
> --- net/if_pfsync.c 11 Sep 2018 07:53:38 -0000 1.259
> +++ net/if_pfsync.c 1 Oct 2018 12:03:16 -0000
> @@ -333,9 +333,9 @@ pfsync_clone_create(struct if_clone *ifc
> ifp->if_hdrlen = sizeof(struct pfsync_header);
> ifp->if_mtu = ETHERMTU;
> ifp->if_xflags = IFXF_CLONED;
> - timeout_set_proc(&sc->sc_tmo, pfsync_timeout, sc);
> - timeout_set_proc(&sc->sc_bulk_tmo, pfsync_bulk_update, sc);
> - timeout_set_proc(&sc->sc_bulkfail_tmo, pfsync_bulk_fail, sc);
> + timeout_set_proc(&sc->sc_tmo, pfsync_timeout, NULL);
> + timeout_set_proc(&sc->sc_bulk_tmo, pfsync_bulk_update, NULL);
> + timeout_set_proc(&sc->sc_bulkfail_tmo, pfsync_bulk_fail, NULL);
>
> if_attach(ifp);
> if_alloc_sadl(ifp);
> @@ -359,9 +359,8 @@ pfsync_clone_destroy(struct ifnet *ifp)
> struct pfsync_softc *sc = ifp->if_softc;
> struct pfsync_deferral *pd;
>
> - timeout_del(&sc->sc_bulkfail_tmo);
> - timeout_del(&sc->sc_bulk_tmo);
> - timeout_del(&sc->sc_tmo);
> + NET_LOCK();
> +
> #if NCARP > 0
> if (!pfsync_sync_ok)
> carp_group_demote_adj(&sc->sc_if, -1, "pfsync destroy");
> @@ -375,7 +374,11 @@ pfsync_clone_destroy(struct ifnet *ifp)
> hook_disestablish(sc->sc_sync_if->if_detachhooks,
> sc->sc_dhcookie);
> }
> +
> + /* XXXSMP breaks atomicity */
> + NET_UNLOCK();
> if_detach(ifp);
> + NET_LOCK();
>
> pfsync_drop(sc);
>
> @@ -385,12 +388,17 @@ pfsync_clone_destroy(struct ifnet *ifp)
> pfsync_undefer(pd, 0);
> }
>
> + pfsyncif = NULL;
> + timeout_del(&sc->sc_bulkfail_tmo);
> + timeout_del(&sc->sc_bulk_tmo);
> + timeout_del(&sc->sc_tmo);
> +
> + NET_UNLOCK();
> +
> pool_destroy(&sc->sc_pool);
> free(sc->sc_imo.imo_membership, M_IPMOPTS, 0);
> free(sc, M_DEVBUF, sizeof(*sc));
>
> - pfsyncif = NULL;
> -
> return (0);
> }
>
> @@ -1752,6 +1760,9 @@ pfsync_undefer(struct pfsync_deferral *p
>
> NET_ASSERT_LOCKED();
>
> + if (sc == NULL)
> + return;
> +
> TAILQ_REMOVE(&sc->sc_deferrals, pd, pd_entry);
> sc->sc_deferred--;
>
> @@ -2252,11 +2263,14 @@ pfsync_bulk_start(void)
> void
> pfsync_bulk_update(void *arg)
> {
> - struct pfsync_softc *sc = arg;
> + struct pfsync_softc *sc;
> struct pf_state *st;
> int i = 0;
>
> NET_LOCK();
> + sc = pfsyncif;
> + if (sc == NULL)
> + goto out;
> st = sc->sc_bulk_next;
>
> for (;;) {
> @@ -2287,6 +2301,7 @@ pfsync_bulk_update(void *arg)
> break;
> }
> }
> + out:
> NET_UNLOCK();
> }
>
> @@ -2316,9 +2331,12 @@ pfsync_bulk_status(u_int8_t status)
> void
> pfsync_bulk_fail(void *arg)
> {
> - struct pfsync_softc *sc = arg;
> + struct pfsync_softc *sc;
>
> NET_LOCK();
> + sc = pfsyncif;
> + if (sc == NULL)
> + goto out;
> if (sc->sc_bulk_tries++ < PFSYNC_MAX_BULKTRIES) {
> /* Try again */
> timeout_add_sec(&sc->sc_bulkfail_tmo, 5);
> @@ -2343,6 +2361,7 @@ pfsync_bulk_fail(void *arg)
> sc->sc_link_demoted = 0;
> DPFPRINTF(LOG_ERR, "failed to receive bulk update");
> }
> + out:
> NET_UNLOCK();
> }
>
