From owner-svn-src-all@freebsd.org Sat Dec 10 01:02:35 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 1E4E9C6D6F6; Sat, 10 Dec 2016 01:02:35 +0000 (UTC) (envelope-from marcel@xcllnt.net) Received: from mail.xcllnt.net (mail.xcllnt.net [50.0.150.214]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id E63B01D8A; Sat, 10 Dec 2016 01:02:34 +0000 (UTC) (envelope-from marcel@xcllnt.net) Received: from [192.168.2.21] (atc.xcllnt.net [50.0.150.213]) by mail.xcllnt.net (8.15.2/8.15.2) with ESMTPS id uBA12Rwj027912 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 9 Dec 2016 17:02:28 -0800 (PST) (envelope-from marcel@xcllnt.net) From: Marcel Moolenaar Message-Id: <853E1226-7B6F-426E-ACD6-9A1E5A126DDF@xcllnt.net> Mime-Version: 1.0 (Mac OS X Mail 10.1 \(3251\)) Subject: Re: svn commit: r309394 - head/sys/netpfil/pf Date: Fri, 9 Dec 2016 17:02:27 -0800 In-Reply-To: <20161208224843.GY27748@FreeBSD.org> Cc: Marcel Moolenaar , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org To: Gleb Smirnoff References: <201612020615.uB26Fxs1049431@repo.freebsd.org> <20161207210824.GN27748@FreeBSD.org> <20161208224843.GY27748@FreeBSD.org> X-Mailer: Apple Mail (2.3251) X-Greylist: Sender IP whitelisted, ACL 39 matched, not delayed by milter-greylist-4.6.1 (mail.xcllnt.net [50.0.150.214]); Fri, 09 Dec 2016 17:02:28 -0800 (PST) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.23 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: Sat, 10 Dec 2016 01:02:35 -0000 > On Dec 8, 2016, at 2:48 PM, Gleb Smirnoff > 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