On Thu, Dec 27, 2018 at 11:03:04AM -0600, Shawn Bohrer wrote:
> On Wed, Dec 26, 2018 at 11:08:43PM +0100, Pablo Neira Ayuso wrote:
[...]
> > @@ -360,11 +340,6 @@ insert_tree(struct net *net,
> > count = 0; /* hotdrop */
> > } else if (ret == NF_CONNCOUNT_ADDED) {
> > count = rbconn->list.count;
> > - } else {
> > - /* NF_CONNCOUNT_SKIP, rbconn is already
> > - * reclaimed by gc, insert a new tree node
> > - */
> > - node_found = false;
> > }
> > break;
> > }
> > @@ -451,11 +426,6 @@ count_tree(struct net *net,
> > return 0; /* hotdrop */
> > } else if (ret == NF_CONNCOUNT_ADDED) {
> > return rbconn->list.count;
> > - } else {
> > - /* NF_CONNCOUNT_SKIP, rbconn is already
> > - * reclaimed by gc, insert a new tree node
> > - */
> > - break;
> > }
> > }
> > }
>
> The packet path can run concurrently on multiple CPUs correct? If
> yes, then there is a race where an empty node is about to be freed by
> tree_nodes_free() which holds the rbconn->list.list_lock, while
> nf_conncount_add() inside count_tree is waiting to acquire the lock.
> Once tree_nodes_free() releases the lock nf_conncount_add() will add
> the connection to the list, but it is too late the node has already
> been rb_erased from the tree.
Right, if the CPU walking over nf_conncount_add() loses race, we have
a connection object on a list that is gone.
> I think count_tree needs to break here if addit is true. Then
> insert_tree will handle calling nf_conncount_add() while holding both
> the rbconn->list.list_lock and nf_conncount_locks.
Therefore we still need the NF_CONNCOUNT_SKIP handling (or rename it
to NF_CONNCOUNT_LOCK as Florian suggested), so we can fall back on
adding a new list in this case. It would be probably good to add a
comment on this on the code.
We can probably rename list->dead to list->gc, given that now that
placing a list of gc does not necessarily mean that it will be
released, in case CPU walking over nf_conncount_add() wins race
towards tree_nodes_free().
Then, we'll need to fetch the parent node as you proposed in your
patch before falling back.
