Date: Fri, 9 Dec 2016 17:02:27 -0800 From: Marcel Moolenaar <marcel@xcllnt.net> To: Gleb Smirnoff <glebius@FreeBSD.org> 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: <853E1226-7B6F-426E-ACD6-9A1E5A126DDF@xcllnt.net> In-Reply-To: <20161208224843.GY27748@FreeBSD.org> References: <201612020615.uB26Fxs1049431@repo.freebsd.org> <20161207210824.GN27748@FreeBSD.org> <FF76DB0B-F6A0-4331-9224-0DC94E91A398@xcllnt.net> <20161208224843.GY27748@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Dec 8, 2016, at 2:48 PM, Gleb Smirnoff <glebius@FreeBSD.org = <mailto:glebius@FreeBSD.org>> wrote: >=20 > Marcel, >=20 > 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>=20 > 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>=20 > M> But, pf(4) has already dropped the reference count for state > M> that=E2=80=99s 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 =E2=80=94 = i.e. > M> after the memory was freed. >=20 > Thanks for explanation, Marcel! Potentially this problem also exists > in pfsync_update_state() and in pfsync_update_state_req(). >=20 > 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(). >=20 > It isn't tested, but is pretty straightforward.=20 I=E2=80=99ll give it a spin and commit. --=20 Marcel Moolenaar marcel@xcllnt.net <mailto:marcel@xcllnt.net>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?853E1226-7B6F-426E-ACD6-9A1E5A126DDF>