From owner-svn-src-all@freebsd.org Thu Dec 8 22:48:45 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7742EC6DA1F; Thu, 8 Dec 2016 22:48:45 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (glebi.us [96.95.210.25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "cell.glebi.us", Issuer "cell.glebi.us" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 58F691DBD; Thu, 8 Dec 2016 22:48:44 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (localhost [127.0.0.1]) by cell.glebi.us (8.15.2/8.15.2) with ESMTPS id uB8Mmi9C030248 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 8 Dec 2016 14:48:44 -0800 (PST) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebi.us (8.15.2/8.15.2/Submit) id uB8MmiUI030247; Thu, 8 Dec 2016 14:48:44 -0800 (PST) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebi.us: glebius set sender to glebius@FreeBSD.org using -f Date: Thu, 8 Dec 2016 14:48:43 -0800 From: Gleb Smirnoff To: Marcel Moolenaar Cc: Marcel Moolenaar , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r309394 - head/sys/netpfil/pf Message-ID: <20161208224843.GY27748@FreeBSD.org> References: <201612020615.uB26Fxs1049431@repo.freebsd.org> <20161207210824.GN27748@FreeBSD.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="ISA6zNVXmTIvvkD5" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.7.0 (2016-08-17) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Dec 2016 22:48:45 -0000 --ISA6zNVXmTIvvkD5 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit Marcel, On Wed, Dec 07, 2016 at 05:06:08PM -0800, Marcel Moolenaar wrote: M> > thanks for the fixes. While the problem with the first chunk M> > in pfsync_sendout() is obvious, the problem you are fixing in th M> > second chunk in the pfsync_delete_state() is not clear to me. M> > Can you please explain what scenario are you fixing there? M> M> State updates may be pending for state being deleted. This M> means that the state is still sitting on either the PFSYNC_S_UPD M> or PFSYNC_S_UPD_C queues. What pfsync(4) does in that case is M> simply remove the state from those queues and add it to the M> PFSYNC_S_DEL queue. M> M> But, pf(4) has already dropped the reference count for state M> that’s deleted and the only reference is by pfsync(4) by virtue M> of being on the PFSYNC_S_UPD or PFSYNC_S_UPD_C queues. When the M> state gets dequeued from those queues, the reference count drops M> to 0 and the state is deleted (read: memory freed). But the same M> state is subsequently added to the PFSYNC_S_DEL queue — i.e. M> after the memory was freed. Thanks for explanation, Marcel! Potentially this problem also exists in pfsync_update_state() and in pfsync_update_state_req(). Your patch introduces extra unnecessary atomic operations, so let me suggest another patch. It should be applied on top of yours, and it also addresses pfsync_update_state() and in pfsync_update_state_req(). It isn't tested, but is pretty straightforward. -- Totus tuus, Glebius. --ISA6zNVXmTIvvkD5 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="pfsync.diff" Index: sys/netpfil/pf/if_pfsync.c =================================================================== --- sys/netpfil/pf/if_pfsync.c (revision 309722) +++ sys/netpfil/pf/if_pfsync.c (working copy) @@ -161,8 +161,8 @@ static struct pfsync_q pfsync_qs[] = { { pfsync_out_del, sizeof(struct pfsync_del_c), PFSYNC_ACT_DEL_C } }; -static void pfsync_q_ins(struct pf_state *, int); -static void pfsync_q_del(struct pf_state *); +static void pfsync_q_ins(struct pf_state *, int, bool); +static void pfsync_q_del(struct pf_state *, bool); static void pfsync_update_state(struct pf_state *); @@ -542,7 +542,7 @@ pfsync_state_import(struct pfsync_state *sp, u_int if (!(flags & PFSYNC_SI_IOCTL)) { st->state_flags &= ~PFSTATE_NOSYNC; if (st->state_flags & PFSTATE_ACK) { - pfsync_q_ins(st, PFSYNC_S_IACK); + pfsync_q_ins(st, PFSYNC_S_IACK, true); pfsync_push(sc); } } @@ -1668,7 +1668,7 @@ pfsync_insert_state(struct pf_state *st) if (sc->sc_len == PFSYNC_MINPKT) callout_reset(&sc->sc_tmo, 1 * hz, pfsync_timeout, V_pfsyncif); - pfsync_q_ins(st, PFSYNC_S_INS); + pfsync_q_ins(st, PFSYNC_S_INS, true); PFSYNC_UNLOCK(sc); st->sync_updates = 0; @@ -1789,7 +1789,7 @@ static void pfsync_update_state(struct pf_state *st) { struct pfsync_softc *sc = V_pfsyncif; - int sync = 0; + bool sync = false, ref = true; PF_STATE_LOCK_ASSERT(st); PFSYNC_LOCK(sc); @@ -1798,7 +1798,7 @@ pfsync_update_state(struct pf_state *st) pfsync_undefer_state(st, 0); if (st->state_flags & PFSTATE_NOSYNC) { if (st->sync_state != PFSYNC_S_NONE) - pfsync_q_del(st); + pfsync_q_del(st, true); PFSYNC_UNLOCK(sc); return; } @@ -1815,14 +1815,17 @@ pfsync_update_state(struct pf_state *st) if (st->key[PF_SK_WIRE]->proto == IPPROTO_TCP) { st->sync_updates++; if (st->sync_updates >= sc->sc_maxupdates) - sync = 1; + sync = true; } break; case PFSYNC_S_IACK: - pfsync_q_del(st); + pfsync_q_del(st, false); + ref = false; + /* FALLTHROUGH */ + case PFSYNC_S_NONE: - pfsync_q_ins(st, PFSYNC_S_UPD_C); + pfsync_q_ins(st, PFSYNC_S_UPD_C, ref); st->sync_updates = 0; break; @@ -1880,6 +1883,7 @@ static void pfsync_update_state_req(struct pf_state *st) { struct pfsync_softc *sc = V_pfsyncif; + bool ref = true; PF_STATE_LOCK_ASSERT(st); PFSYNC_LOCK(sc); @@ -1886,7 +1890,7 @@ pfsync_update_state_req(struct pf_state *st) if (st->state_flags & PFSTATE_NOSYNC) { if (st->sync_state != PFSYNC_S_NONE) - pfsync_q_del(st); + pfsync_q_del(st, true); PFSYNC_UNLOCK(sc); return; } @@ -1894,9 +1898,12 @@ pfsync_update_state_req(struct pf_state *st) switch (st->sync_state) { case PFSYNC_S_UPD_C: case PFSYNC_S_IACK: - pfsync_q_del(st); + pfsync_q_del(st, false); + ref = false; + /* FALLTHROUGH */ + case PFSYNC_S_NONE: - pfsync_q_ins(st, PFSYNC_S_UPD); + pfsync_q_ins(st, PFSYNC_S_UPD, true); pfsync_push(sc); break; @@ -1917,6 +1924,7 @@ static void pfsync_delete_state(struct pf_state *st) { struct pfsync_softc *sc = V_pfsyncif; + bool ref = true; PFSYNC_LOCK(sc); if (st->state_flags & PFSTATE_ACK) @@ -1923,7 +1931,7 @@ pfsync_delete_state(struct pf_state *st) pfsync_undefer_state(st, 1); if (st->state_flags & PFSTATE_NOSYNC) { if (st->sync_state != PFSYNC_S_NONE) - pfsync_q_del(st); + pfsync_q_del(st, true); PFSYNC_UNLOCK(sc); return; } @@ -1931,22 +1939,21 @@ pfsync_delete_state(struct pf_state *st) if (sc->sc_len == PFSYNC_MINPKT) callout_reset(&sc->sc_tmo, 1 * hz, pfsync_timeout, V_pfsyncif); - pf_ref_state(st); - switch (st->sync_state) { case PFSYNC_S_INS: /* We never got to tell the world so just forget about it. */ - pfsync_q_del(st); + pfsync_q_del(st, true); break; case PFSYNC_S_UPD_C: case PFSYNC_S_UPD: case PFSYNC_S_IACK: - pfsync_q_del(st); - /* FALLTHROUGH to putting it on the del list */ + pfsync_q_del(st, false); + ref = false; + /* FALLTHROUGH */ case PFSYNC_S_NONE: - pfsync_q_ins(st, PFSYNC_S_DEL); + pfsync_q_ins(st, PFSYNC_S_DEL, ref); break; default: @@ -1953,8 +1960,6 @@ pfsync_delete_state(struct pf_state *st) panic("%s: unexpected sync state %d", __func__, st->sync_state); } - pf_release_state(st); - PFSYNC_UNLOCK(sc); } @@ -1982,7 +1987,7 @@ pfsync_clear_states(u_int32_t creatorid, const cha } static void -pfsync_q_ins(struct pf_state *st, int q) +pfsync_q_ins(struct pf_state *st, int q, bool ref) { struct pfsync_softc *sc = V_pfsyncif; size_t nlen = pfsync_qs[q].len; @@ -2006,11 +2011,12 @@ static void sc->sc_len += nlen; TAILQ_INSERT_TAIL(&sc->sc_qs[q], st, sync_list); st->sync_state = q; - pf_ref_state(st); + if (ref) + pf_ref_state(st); } static void -pfsync_q_del(struct pf_state *st) +pfsync_q_del(struct pf_state *st, bool unref) { struct pfsync_softc *sc = V_pfsyncif; int q = st->sync_state; @@ -2022,7 +2028,8 @@ static void sc->sc_len -= pfsync_qs[q].len; TAILQ_REMOVE(&sc->sc_qs[q], st, sync_list); st->sync_state = PFSYNC_S_NONE; - pf_release_state(st); + if (unref) + pf_release_state(st); if (TAILQ_EMPTY(&sc->sc_qs[q])) sc->sc_len -= sizeof(struct pfsync_subheader); --ISA6zNVXmTIvvkD5--