Date: Thu, 8 Dec 2016 14:48:43 -0800 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Marcel Moolenaar <marcel@xcllnt.net> Cc: Marcel Moolenaar <marcel@FreeBSD.org>, 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> In-Reply-To: <FF76DB0B-F6A0-4331-9224-0DC94E91A398@xcllnt.net> References: <201612020615.uB26Fxs1049431@repo.freebsd.org> <20161207210824.GN27748@FreeBSD.org> <FF76DB0B-F6A0-4331-9224-0DC94E91A398@xcllnt.net>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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.
[-- Attachment #2 --]
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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20161208224843.GY27748>