On Sun, Dec 23, 2018 at 01:34:27PM -0600, Shawn Bohrer wrote:
> If we are about to replace a rbnode because it is dead we need to ensure
> that the old node gets GCed. All other places that look for nodes to GC
> rely on walking the tree to find them so if we don't do it here the node
> will be lost.
>
> Fixes: 5c789e131cbb9 ("netfilter: nf_conncount: Add list lock and gc worker, and RCU for init tree search")
> Signed-off-by: Shawn Bohrer <sbohrer@xxxxxxxxxxxxxx>
> ---
> net/netfilter/nf_conncount.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/nf_conncount.c b/net/netfilter/nf_conncount.c
> index 372015e3f18d..df447877e3ac 100644
> --- a/net/netfilter/nf_conncount.c
> +++ b/net/netfilter/nf_conncount.c
> @@ -381,11 +381,15 @@ insert_tree(struct net *net,
> */
> node_found = false;
> parent = rb_parent(*rbnode);
> + gc_nodes[gc_count++] = rbconn;
Sorry, this isn't correct. This only needs to be done for the first
node in the tree. Most other nodes will have already been checked by
nf_conncount_gc_list() and thus would end up in a double free. I say
most because any node that is an exact match after gc_count >=
ARRAY_SIZE(gc_nodes) would also be leaked.
--
Shawn
