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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Sat, Sep 19, 2026 at 01:34:48PM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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.

This is a pre-existing issue. I have a patch for this already in the
queue for the stats case.

This patch is already improving the situation.

> > 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