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

[netdev-bot+sashiko@kernel.org]

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