Re: [PATCH nf-next RFC 1/3] xt_statistic: taking GRO/GSO into account for nth-match

[Florian Westphal <fw@strlen.de>]

Jesper Dangaard Brouer <hawk@kernel.org> wrote:
> The iptables statistic nth mode is documented to match one packet every nth
> packets. When packets gets GRO/GSO aggregated before traversing the statistic
> nth match, then they get accounted as a single packet.
> 
> This patch takes into account the number of packet frags a GRO/GSO packet
> contains for the xt_statistic match.

I doubt we can do this upstream.  Two reasons that come to mind, in no
particular order:

1. It introduces asymmetry.  All matches use "skb == packet" design.
   Examples that come to mind: xt_limit, xt_length.
2. This adds a compat issue with nftables:
    iptables-translate -A INPUT -m statistic --mode nth --packet 0  --every 10
    nft 'add rule ip filter INPUT numgen inc mod 10 0 counter'

'numgen' increments a counter for every skb, i.e. reg := i++;.
But, after this patch -m statistics doesn't work this way anymore
and the two rules no longer do the same thing.

But even if we'd ignore this or add a flag to control behavior, I don't
see how this could be implemented in nft.

And last but not least, I'm not sure the premise is correct.
Yes, when you think of 'packet sampling', then we don't 'match'
often enough for gro/gso case.

However, when doing '-m statistic ... -j LOG' (or whatever), then the
entire GSO superpacket is logged, i.e. several 'packets' got matched
at once.

So the existing algorithm works correctly even when considering
aggregation because on average the correct amount of segments gets
matched (logged).

With this proposed new algo, we can now match 100% of skbs / aggregated
segments, even for something like '--every 10'.  And that seems fishy to
me.

As far as I understood its only 'more correct' in your case because the
logging backend picks one individual segment out from the NFLOG'd
superpacket.

But if it would NOT do that, then you now sample (almost) all segments
seen on wire.  Did I misunderstand something here?