[PATCH nf,v3 1/4] netfilter: flowtable: add and use NF_FLOW_CONFIRMED bit to skip GC

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.

Apparently, there is a narrow window where GC might walk over a flow
entry while it is in a partial state, ie. flow entry has been 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.

Note that the flowtable iterators can still find reach flows in partial
state. This is fine because they are used for cleanup scenario (ie.
netdevice is gone) where the teardown bit is set on. On flowtable
removal, no flow is seen in partial state because the flow_offload
action already does not see packets (ie. flowtable in detached from
rule).

The datapath lookup function still can find the flow in partial state,
this is addressed by a follow up patch which adds a check to skip
expired flows.

Fixes: 2014ac62df9d ("netfilter: flowtable: publish GC-visible tuple last")
Signed-off-by: Pablo Neira Ayuso <[email protected]>
---
v3: - remove check from lookup path
    - add smp_mb__before_atomic() before set_bit(NF_FLOW_CONFIRMED, ...

 include/net/netfilter/nf_flow_table.h |  1 +
 net/netfilter/nf_flow_table_core.c    | 16 +++++++++++-----
 2 files changed, 12 insertions(+), 5 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..cbb56ca94717 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,9 @@ 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);
 
+	smp_mb__before_atomic();
+	set_bit(NF_FLOW_CONFIRMED, &flow->flags);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(flow_offload_add);
@@ -568,8 +570,12 @@ 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;
 
+	teardown = test_bit(NF_FLOW_TEARDOWN, &flow->flags);
 	if (nf_flow_has_expired(flow) ||
 	    nf_ct_is_dying(flow->ct) ||
 	    nf_flow_custom_gc(flow_table, flow)) {
-- 
2.47.3