| Message ID | 20181223193428.15375-3-sbohrer@cloudflare.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | nf_conntrack bugfixes | expand |
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@cloudflare.com> > --- > 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
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; } break; } - if (gc_count >= ARRAY_SIZE(gc_nodes)) + /* Must keep one free array slot in case we find an + * exact match that needs to be reclaimed. + */ + if (gc_count >= ARRAY_SIZE(gc_nodes) - 1) continue; if (nf_conncount_gc_list(net, &rbconn->list))
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@cloudflare.com> --- net/netfilter/nf_conncount.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)