svn commit: r309394 - head/sys/netpfil/pf

[Gleb Smirnoff]

  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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pfsync.diff
Type: text/x-diff
Size: 5185 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20161208/84936264/attachment.diff>