Re: [PATCH net 1/8] netfilter: flowtable: publish HW_DEAD after worker is done

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete fix: the FLOW_CLS_STATS (and, in a narrower window,
  FLOW_CLS_REPLACE) offload work items still hold an unreferenced, non-…

--

netfilter: flowtable: publish HW_DEAD after worker is done

The NF_FLOW_HW_DEAD publication is moved out of flow_offload_work_del()
into the common flow_offload_work_handler() epilogue, after
NF_FLOW_HW_PENDING is cleared, so that the destroy work item does not
touch the flow after garbage collection is allowed to free it.

> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index 801a3dd9ceea..6757fd89c1f1 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -995,7 +995,6 @@ static void flow_offload_work_del(struct flow_offload_work *offload)
>  	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
>  	if (test_bit(NF_FLOW_HW_BIDIRECTIONAL, &offload->flow->flags))
>  		flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
> -	set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
>  }
>  
>  static void flow_offload_tuple_stats(struct flow_offload_work *offload,
> @@ -1059,6 +1058,12 @@ static void flow_offload_work_handler(struct work_struct *work)
>  	}
>  
>  	clear_bit(NF_FLOW_HW_PENDING, &offload->flow->flags);
> +	if (offload->cmd == FLOW_CLS_DESTROY) {
> +		/* Publish after the worker's last flow access. */
> +		smp_mb__before_atomic();
> +		set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
> +	}
> +
>  	kfree(offload);
>  }

[Severity: High]
Does this reordering also cover the FLOW_CLS_STATS and FLOW_CLS_REPLACE work
items?  The clear_bit() above runs for every command, and for stats work the
flow can already be gone by the time the worker reaches it.

The sequence I think is still possible, all on a flow that has NF_FLOW_HW set:

A remote FIN/RST marks the flow closing in nf_flow_state_check():

net/netfilter/nf_flow_table_ip.c:nf_flow_state_check() {
	if ((tcph->fin || tcph->rst) &&
	    !test_bit(NF_FLOW_CLOSING, &flow->flags))
		set_bit(NF_FLOW_CLOSING, &flow->flags);
}

nf_flow_offload_gc_step() then queues the destroy work, and the flow stays
linked in the rhashtable:

net/netfilter/nf_flow_table_core.c:nf_flow_offload_gc_step() {
	} else if (test_bit(NF_FLOW_CLOSING, &flow->flags) &&
		   test_bit(NF_FLOW_HW, &flow->flags) &&
		   !test_bit(NF_FLOW_HW_DYING, &flow->flags)) {
		nf_flow_offload_del(flow_table, flow);
	} else if (test_bit(NF_FLOW_HW, &flow->flags)) {
		nf_flow_offload_stats(flow_table, flow);
	}
}

The destroy worker now clears NF_FLOW_HW_PENDING and publishes
NF_FLOW_HW_DEAD.  Since NF_FLOW_HW, NF_FLOW_HW_DYING and NF_FLOW_HW_DEAD are
never cleared again anywhere in the tree, a later garbage collection pass on
the same flow falls through to the last branch above and queues a stats work
item, because nf_flow_offload_work_alloc() only gates on the pending bit:

net/netfilter/nf_flow_table_offload.c:nf_flow_offload_work_alloc() {
	if (test_and_set_bit(NF_FLOW_HW_PENDING, &flow->flags))
		return NULL;
	...
	offload->flow = flow;
}

So a stats work item can exist for a flow that already has HW_DYING and
HW_DEAD set.  When NF_FLOW_TEARDOWN is then set (expiry, ct dying, dst
invalid, netdev down, or a SYN on the closing flow via
nf_flow_state_check() -> flow_offload_teardown()), the next pass frees it:

net/netfilter/nf_flow_table_core.c:nf_flow_offload_gc_step() {
	if (teardown) {
		if (test_bit(NF_FLOW_HW, &flow->flags)) {
			if (!test_bit(NF_FLOW_HW_DYING, &flow->flags))
				nf_flow_offload_del(flow_table, flow);
			else if (test_bit(NF_FLOW_HW_DEAD, &flow->flags))
				flow_offload_del(flow_table, flow);
	...
}

flow_offload_del() -> flow_offload_free() -> call_rcu(&flow->rcu_head,
flow_offload_free_rcu) waits only for RCU readers.  The stats worker holds no
reference and no RCU read lock, and it sleeps in
down_read(&flowtable->flow_block_lock) and in the driver callbacks, so can
the grace period elapse while flow_offload_work_stats() is still using the
flow here?

net/netfilter/nf_flow_table_offload.c:flow_offload_work_stats() {
	if (test_bit(NF_FLOW_HW_BIDIRECTIONAL, &offload->flow->flags))
	...
	offload->flow->timeout = max_t(u64, offload->flow->timeout,
				       lastused + flow_offload_get_timeout(offload->flow));
}

and then in the shared epilogue:

	clear_bit(NF_FLOW_HW_PENDING, &offload->flow->flags);

That looks like the same "worker writes flow flags after the flow may have
been freed" pattern the commit message describes, just reached through the
stats work item instead of the destroy one.

The ordering in nf_flow_table_gc_cleanup() seems to make this explicit on the
netdev-down and flowtable-delete paths:

net/netfilter/nf_flow_table_core.c:nf_flow_table_gc_cleanup() {
	nf_flow_table_iterate(flowtable, nf_flow_table_do_cleanup, dev);
	flush_delayed_work(&flowtable->gc_work);
	nf_flow_table_offload_flush(flowtable);
}

Teardown is set, the gc flush frees the HW_DEAD flows, and only afterwards
are the add/del/stats workqueues drained.  Should the flush of the offload
workqueues happen before the gc flush here?

Would it make sense for the free decision in nf_flow_offload_gc_step() to
also consult NF_FLOW_HW_PENDING, or for the work items to take a reference
on the flow (or for work admission to be refused once NF_FLOW_HW_DYING is
set), rather than only reordering the two bit operations in the destroy
worker?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918112844.194503-2-pablo%40netfilter.org