[PATCH nf] netfilter: flowtable: add and use NF_FLOW_CONFIRMED bit

This new NF_FLOW_CONFIRMED bit is set on once the flow entry has fully
been set up, so it can be exposed to the GC and software datapath via
lookup.

Apparently, there is a narrow window where GC might walk over a flow
entry which is in intermediate state, ie. flow entry has inserted in the
hashes but the NF_FLOW_HW has not been set up yet. While packets are
flowing in the software datapath that could teardown the flow.

This reverts commit 2014ac62df9d ("netfilter: flowtable: publish
GC-visible tuple last") since reversing the tuple order insertion only
mitigates the issue for one scenario.

Fixes: 2014ac62df9d ("netfilter: flowtable: publish GC-visible tuple last")
Signed-off-by: Pablo Neira Ayuso <[email protected]>
---
 include/net/netfilter/nf_flow_table.h |  1 +
 net/netfilter/nf_flow_table_core.c    | 20 +++++++++++++-------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index ce414118962f..618d26e49fdf 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -189,6 +189,7 @@ enum nf_flow_flags {
 	NF_FLOW_HW_PENDING,
 	NF_FLOW_HW_BIDIRECTIONAL,
 	NF_FLOW_HW_ESTABLISHED,
+	NF_FLOW_CONFIRMED,
 };
 
 enum flow_offload_type {
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 8b1165f2b5a4..1e2c4a1db9a7 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -332,18 +332,17 @@ int flow_offload_add(struct nf_flowtable *flow_table, struct flow_offload *flow)
 	flow->timeout = nf_flowtable_time_stamp + flow_offload_get_timeout(flow);
 
 	err = rhashtable_insert_fast(&flow_table->rhashtable,
-				     &flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].node,
+				     &flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].node,
 				     nf_flow_offload_rhash_params);
 	if (err < 0)
 		return err;
 
-	/* GC only iterates original-direction entries; publish original last. */
 	err = rhashtable_insert_fast(&flow_table->rhashtable,
-				     &flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].node,
+				     &flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].node,
 				     nf_flow_offload_rhash_params);
 	if (err < 0) {
 		rhashtable_remove_fast(&flow_table->rhashtable,
-				       &flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].node,
+				       &flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].node,
 				       nf_flow_offload_rhash_params);
 		return err;
 	}
@@ -353,6 +352,8 @@ int flow_offload_add(struct nf_flowtable *flow_table, struct flow_offload *flow)
 	if (nf_flowtable_hw_offload(flow_table))
 		nf_flow_offload_add(flow_table, flow);
 
+	set_bit(NF_FLOW_CONFIRMED, &flow->flags);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(flow_offload_add);
@@ -412,7 +413,8 @@ flow_offload_lookup(struct nf_flowtable *flow_table,
 
 	dir = tuplehash->tuple.dir;
 	flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
-	if (test_bit(NF_FLOW_TEARDOWN, &flow->flags))
+	if (unlikely(!test_bit(NF_FLOW_CONFIRMED, &flow->flags)) ||
+	    test_bit(NF_FLOW_TEARDOWN, &flow->flags))
 		return NULL;
 
 	if (unlikely(nf_ct_is_dying(flow->ct)))
@@ -568,15 +570,19 @@ static void nf_flow_table_extend_ct_timeout(struct nf_conn *ct)
 static void nf_flow_offload_gc_step(struct nf_flowtable *flow_table,
 				    struct flow_offload *flow, void *data)
 {
-	bool teardown = test_bit(NF_FLOW_TEARDOWN, &flow->flags);
+	bool teardown;
+
+	if (!test_bit(NF_FLOW_CONFIRMED, &flow->flags))
+		return;
 
 	if (nf_flow_has_expired(flow) ||
 	    nf_ct_is_dying(flow->ct) ||
 	    nf_flow_custom_gc(flow_table, flow)) {
 		flow_offload_teardown(flow);
 		teardown = true;
-	} else if (!teardown) {
+	} else if (!test_bit(NF_FLOW_TEARDOWN, &flow->flags)) {
 		nf_flow_table_extend_ct_timeout(flow->ct);
+		teardown = false;
 	}
 
 	if (teardown) {
-- 
2.47.3