Re: [PATCH net-next 03/15] netfilter: flowtable: retrieve stats on flow deletion

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

On Sat, Nov 14, 2026 at 03:23:41AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The core change only does anything when a driver writes
>   f->stats in its FLOW_CLS_DESTROY handler.
> - [Medium] The final hardware stats are added to the conntrack
>   asynchronously in flow_offload_work_del() ->…
> 
> --
> 
> netfilter: flowtable: retrieve stats on flow deletion
> 
> This commit passes a flow_stats object to the driver on FLOW_CLS_DESTROY,
> so the final hardware counters can be synced into the conntrack entry
> when a flow is removed from hardware. The stats update logic moves out of
> flow_offload_work_stats() into a new flow_offload_update_stats() helper,
> which both the stats path and the delete path now use.
> 
> Closes: https://patch.msgid.link/netfilter-devel/20261021175030.4800-1-dreyksmail@gmail.com/
> 
> > diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> > index 3a18fa48cadb0..c302051489f23 100644
> > --- a/net/netfilter/nf_flow_table_offload.c
> > +++ b/net/netfilter/nf_flow_table_offload.c
> > @@ -928,12 +928,33 @@ static int nf_flow_offload_tuple(struct nf_flowtable *flowtable,
> 
> [ ... ]
> 
> > +static void flow_offload_update_stats(struct flow_offload_work *offload,
> > +				      struct flow_stats *stats)
> > +{
> > +	u64 lastused;
> > +
> 
> [ ... ]
> 
> > +	if (offload->flowtable->flags & NF_FLOWTABLE_COUNTER) {
> > +		if (stats[0].pkts)
> > +			nf_ct_acct_add(offload->flow->ct,
> > +				       FLOW_OFFLOAD_DIR_ORIGINAL,
> > +				       stats[0].pkts, stats[0].bytes);
> 
> [Severity: Medium]
> When we get here from flow_offload_work_del(), could the conntrack entry
> already have been deleted and reported?
> 
> One of the triggers that queues the DESTROY work is a dying ct in
> nf_flow_offload_gc_step():
> 
> net/netfilter/nf_flow_table_core.c:nf_flow_offload_gc_step() {
>     ...
> 	    nf_ct_is_dying(flow->ct) ||
>     ...
> }
> 
> By that point nf_ct_delete() has already set IPS_DYING, unlinked the
> entry and sent IPCT_DESTROY. That event carries the ct accounting
> counters to ctnetlink/ulogd:
> 
> net/netfilter/nf_conntrack_core.c:nf_ct_delete() {
>     ...
> 	if (test_and_set_bit(IPS_DYING_BIT, &ct->status))
> 		return false;
>     ...
> 	if (nf_conntrack_event_report(IPCT_DESTROY, ct,
> 				    portid, report) < 0) {
>     ...
> }
> 
> So when an offloaded ct is removed by conntrack -D or -F

This patch does not handle the conntrack -D or -F scenario, that would
require postponing the event delivery.

That is a pre-existing issue, this patch is not targeting at this
scenario.

I don't think the fix needed for this belong to this patch.

> by nf_ct_iterate_cleanup() on device or NAT events, or by netns teardown,
> the sequence looks like this:
> 
> nf_ct_delete()
>   IPCT_DESTROY event sent with the current counters
> nf_flow_offload_gc_step()
>   nf_ct_is_dying() -> flow_offload_teardown() -> nf_flow_offload_del()
> flow_offload_work_handler()
>   flow_offload_work_del()
>     flow_offload_update_stats()
>       nf_ct_acct_add()
> 
> The counters added here go into a ct that nothing reports any more. On
> this path, the hardware traffic since the last FLOW_CLS_STATS poll is
> still lost.
> 
> The common case, a flow expiring while its ct is still alive, looks
> fine. flow_offload_teardown() leaves the ct alive, and this update runs
> before the ct's own destroy event.
> 
> Is the deleted-ct case meant to be covered by the "missing last stats
> sync" that the commit message describes?
> 
> > +		if (stats[1].pkts)
> > +			nf_ct_acct_add(offload->flow->ct,
> > +				       FLOW_OFFLOAD_DIR_REPLY,
> > +				       stats[1].pkts, stats[1].bytes);
> > +	}
> > +}
> 
> [ ... ]
> 
> > @@ -991,10 +1013,15 @@ static void flow_offload_work_add(struct flow_offload_work *offload)
> >  
> >  static void flow_offload_work_del(struct flow_offload_work *offload)
> >  {
> > +	struct flow_stats stats[FLOW_OFFLOAD_DIR_MAX] = {};
> > +
> >  	clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
> > -	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
> > +	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL, &stats[0]);
> >  	if (test_bit(NF_FLOW_HW_BIDIRECTIONAL, &offload->flow->flags))
> > -		flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
> > +		flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY, &stats[1]);
> > +
> > +	flow_offload_update_stats(offload, stats);
> 
> [Severity: Medium]
> Won't stats[] still be all zero here for mlx5 and nfp?
> 
> cls_flow is zero-initialised in nf_flow_offload_tuple(). If a driver's
> FLOW_CLS_DESTROY handler never writes f->stats, the memcpy() copies back
> zeroes, and flow_offload_update_stats() skips nf_ct_acct_add().
> 
> Later patches in this series convert mtk_ppe and airoha ("net: ethernet:
> mtk_ppe: update stats on flow deletion" and "net: airoha: update stats on
> flow deletion"). Two other in-tree consumers still report nothing on
> DESTROY at the end of the series.
> 
> In mlx5, mlx5_tc_ct_block_flow_offload_del() drops the counter through
> mlx5_tc_ct_entry_put(). It never calls mlx5_fc_query_cached() or
> flow_stats_update().
> 
> In nfp, nfp_fl_ct_handle_nft_flow() does:
> 
> 	case FLOW_CLS_DESTROY:
> 		ct_map_ent = rhashtable_lookup_fast(...);
> 		return nfp_fl_ct_del_flow(ct_map_ent);
> 
> Both drivers report packet and byte deltas only from their
> FLOW_CLS_STATS handlers, mlx5_tc_ct_block_flow_offload_stats() and
> nfp_fl_ct_stats().
> 
> Both register through act_ct, which turns on counter accounting:
> 
> net/sched/act_ct.c:tcf_ct_flow_table_get() {
>     ...
> 	ct_ft->nf_ft.flags |= NF_FLOWTABLE_HW_OFFLOAD |
> 			      NF_FLOWTABLE_COUNTER;
>     ...
> }
> 
> So with tc act_ct offload on mlx5 or nfp, traffic forwarded between the
> last FLOW_CLS_STATS poll and the hardware delete is still missing from
> the conntrack counters.
> 
> Given the Reported-by and Closes tags, should mlx5 and nfp be converted
> in this series as well? sfc doesn't seem to be affected, because
> efx_tc_ct_stats() only reports lastused.
> 
> > +
> >  	set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
> >  }
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113031251.2437074-2-pablo%40netfilter.org