Re: [PATCH net-next 04/15] net: ethernet: mtk_ppe: update stats on flow deletion

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

On Sat, Nov 14, 2026 at 03:23:42AM +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] Bridge (L2) flows on mtk_ppe: the new destroy-time call to
>   mtk_flow_offload_update_stats() read-clears the hardware MIB counter…
> - [Medium] mtk_flow_offload_destroy() takes the final MIB snapshot before
>   it invalidates the hardware FOE entry, so the 'last stats sync' on…
> 
> --
> 
> net: ethernet: mtk_ppe: update stats on flow deletion
> 
> This moves the stats collection out of mtk_flow_offload_stats() into a new
> helper, mtk_flow_offload_update_stats(). The helper is also called from
> mtk_flow_offload_destroy(), so the final hardware counters are reported
> when a flow is deleted. This follows "netfilter: flowtable: retrieve stats
> on flow deletion".
> 
> > diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> > index 99b28aaa7cc47..8ca694bb5e1b7 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> > +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> > @@ -526,6 +526,24 @@ mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f,
> >  	return err;
> >  }
> >  
> > +static void
> > +mtk_flow_offload_update_stats(struct mtk_eth *eth, struct flow_cls_offload *f,
> > +			      struct mtk_flow_entry *entry)
> > +{
> > +	struct mtk_foe_accounting diff;
> > +	u32 idle;
> > +
> > +	idle = mtk_foe_entry_idle_time(eth->ppe[entry->ppe_index], entry);
> > +	f->stats.lastused = jiffies - idle * HZ;
> > +
> > +	if (entry->hash != 0xFFFF &&
> > +	    mtk_foe_entry_get_mib(eth->ppe[entry->ppe_index], entry->hash,
> > +				  &diff)) {
> 
> [Severity: Medium]
> When a bridge (L2) flow is destroyed, can this read-clear the hardware
> counter of an unrelated flow?

Maybe, but that is a pre-existing issue.

This patch is just adapting the driver to use the new call to stats
when deleting a flow, it reuses the existing codebase.

> For a flower rule with only ETH_ADDRS, mtk_flow_offload_replace()
> allocates the entry with kzalloc_obj(), so entry->hash starts at 0.
> mtk_foe_entry_commit() then takes the L2 path:
> 
> drivers/net/ethernet/mediatek/mtk_ppe.c:mtk_foe_entry_commit() {
> 	...
> 	if (type == MTK_PPE_PKT_TYPE_BRIDGE)
> 		return mtk_foe_entry_commit_l2(ppe, entry);
> 
> 	hash = mtk_ppe_hash_entry(ppe->eth, &entry->data);
> 	entry->hash = 0xffff;
> 	...
> }
> 
> mtk_foe_entry_commit_l2() never sets entry->hash = 0xffff. The base L2
> entry is not on the foe_flow lists, so __mtk_ppe_check_skb() never updates
> its hash. mtk_flow_entry_update_l2() only touches the subflow hashes. As a
> result, the base entry keeps hash == 0 for its whole lifetime.
> 
> That means the entry->hash != 0xFFFF check passes and
> mtk_foe_entry_get_mib(ppe, 0, &diff) runs. mtk_ppe_start() enables
> MTK_PPE_MIB_CFG_RD_CLR, so this read zeroes the MIB counter of whatever
> flow is bound at FOE index 0, for example an nf_flowtable L3 flow. Those
> packets and bytes get charged to the L2 rule and never reach their real
> owner.
> 
> The bridge flow's own counters live in the subflow slots on
> entry->l2_flows, and this function never reads them. So the destroy-time
> stats for L2 flows are wrong as well.
> 
> The missing 0xffff on L2 base entries is not new. Before this patch,
> though, mtk_flow_offload_destroy() never touched the MIB. Now every
> FLOW_CLS_DESTROY of a bridge flow does this read-clear, even when nobody
> requested stats for that rule.
> 
> Should L2 base entries be skipped here? Alternatively, should
> mtk_foe_entry_commit_l2() set entry->hash to 0xffff?
> 
> > +		f->stats.pkts += diff.packets;
> > +		f->stats.bytes += diff.bytes;
> > +	}
> > +}
> > +
> >  static int
> >  mtk_flow_offload_destroy(struct mtk_eth *eth, struct flow_cls_offload *f)
> >  {
> > @@ -536,6 +554,8 @@ mtk_flow_offload_destroy(struct mtk_eth *eth, struct flow_cls_offload *f)
> >  	if (!entry)
> >  		return -ENOENT;
> >  
> > +	mtk_flow_offload_update_stats(eth, f, entry);
> > +
> >  	mtk_foe_entry_clear(eth->ppe[entry->ppe_index], entry);
> 
> [Severity: Medium]
> Is this snapshot really the final one for the flow?
> 
> mtk_flow_offload_update_stats() read-clears MIB[entry->hash] while the
> hardware FOE entry is still in BIND state. ppe_lock is dropped between
> the two steps. The entry is only invalidated later, under ppe_lock, in
> __mtk_foe_entry_clear():
> 
> drivers/net/ethernet/mediatek/mtk_ppe.c:__mtk_foe_entry_clear() {
> 	...
> 		hwe->ib1 &= ~MTK_FOE_IB1_STATE;
> 		hwe->ib1 |= FIELD_PREP(MTK_FOE_IB1_STATE, MTK_FOE_STATE_INVALID);
> 		dma_wmb();
> 		mtk_ppe_cache_clear(ppe);
> 
> 		if (ppe->accounting) {
> 			...
> 			acct->packets = 0;
> 			acct->bytes = 0;
> 		}
> 	}
> 	entry->hash = 0xffff;
> 	...
> }
> 
> Between the MIB read and the invalidation, the PPE keeps forwarding and
> counting packets for this flow. That traffic never reaches f->stats. It
> stays in the hardware MIB counter, because only the software acct_table
> is zeroed here.
> 
> __mtk_foe_entry_commit() doesn't clear the MIB either. So the next
> read-clear of that index charges those packets to whichever flow binds
> the slot next.
> 
> This happens whenever an active flow is torn down, for example on a
> flowtable flush, a conntrack flush or a ruleset deletion.
> 
> Would it work to invalidate the entry first and then drain
> MIB[saved hash]?
> 
> >  	rhashtable_remove_fast(&eth->flow_table, &entry->node,
> >  			       mtk_flow_ht_params);
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113031251.2437074-2-pablo%40netfilter.org