Hi Florian,

Thanks for sending a patch for this.

On Thu, Mar 07, 2019 at 08:30:41PM +0100, Florian Westphal wrote:
> The abort path can cause a double-free of an (anon) set.
> 
> Added-and-to-be-aborted rule looks like this:
> 
> udp dport { 137, 138 } drop
> 
> The to-be-aborted transaction list looks like this:
> newset
> newsetelem
> newsetelem
> rule
> 
> This gets walked in reverse order, so first pass disables
> the rule, the set elements, then the set.
> 
> After synchronize_rcu(), we then destroy those in same order:
> rule, set element, set element, newset.
> 
> Problem is that the (anon) set has already been bound to the rule,
> so the rule (lookup expression destructor) already frees the set,
> when then cause use-after-free when trying to delete the elements
> from this set, then try to free the set again when handling the
> newset expression.
> 
> To resolve this, check in first phase if the newset is bound already.
> If so, remove the newset transaction from the list, rule destructor
> will handle cleanup.
> 
> This is still causes the use-after-free on set element removal.
> To handle this, move all affected set elements to a extra list
> and process it first.
>
> This forces strict 'destroy elements, then set' ordering.

So the problem is only the use-after-free from the NEWSETELEM abort
path, right?

Probably we can fix this problem with this patch too? Idea is to keep
this 'bound' internal flag, in that case, this turns the NEWSET and
NEWSETELEM abort path into noop.