diff mbox series

[net] net: dsa: report rx_bytes unadjusted for ETH_HLEN

Message ID 20230317231900.3944446-1-vladimir.oltean@nxp.com
State New
Headers
Series [net] net: dsa: report rx_bytes unadjusted for ETH_HLEN |

Commit Message

Vladimir Oltean March 17, 2023, 11:19 p.m. UTC 
  We collect the software statistics counters for RX bytes (reported to
/proc/net/dev and to ethtool -S $dev | grep 'rx_bytes: ") at a time when
skb->len has already been adjusted by the eth_type_trans() ->
skb_pull_inline(skb, ETH_HLEN) call to exclude the L2 header.

This means that when connecting 2 DSA interfaces back to back and
sending 1 packet with length 100, the sending interface will report
tx_bytes as incrementing by 100, and the receiving interface will report
rx_bytes as incrementing by 86.

Since accounting for that in scripts is quirky and is something that
would be DSA-specific behavior (requiring users to know that they are
running on a DSA interface in the first place), the proposal is that we
treat it as a bug and fix it.

This design bug has always existed in DSA, according to my analysis:
commit 91da11f870f0 ("net: Distributed Switch Architecture protocol
support") also updates skb->dev->stats.rx_bytes += skb->len after the
eth_type_trans() call. Technically, prior to Florian's commit
a86d8becc3f0 ("net: dsa: Factor bottom tag receive functions"), each and
every vendor-specific tagging protocol driver open-coded the same bug,
until the buggy code was consolidated into something resembling what can
be seen now. So each and every driver should have its own Fixes: tag,
because of their different histories until the convergence point.
I'm not going to do that, for the sake of simplicity, but just blame the
oldest appearance of buggy code.

There are 2 ways to fix the problem. One is the obvious way, and the
other is how I ended up doing it. Obvious would have been to move
dev_sw_netstats_rx_add() one line above eth_type_trans(), and below
skb_push(skb, ETH_HLEN). But DSA processing is not as simple as that.
We count the bytes after removing everything DSA-related from the
packet, to emulate what the packet's length was, on the wire, when the
user port received it.

When eth_type_trans() executes, dsa_untag_bridge_pvid() has not run yet,
so in case the switch driver requests this behavior - commit
412a1526d067 ("net: dsa: untag the bridge pvid from rx skbs") has the
details - the obvious variant of the fix wouldn't have worked, because
the positioning there would have also counted the not-yet-stripped VLAN
header length, something which is absent from the packet as seen on the
wire (there it may be untagged, whereas software will see it as
PVID-tagged).

Fixes: f613ed665bb3 ("net: dsa: Add support for 64-bit statistics")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/tag.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

patchwork-bot+netdevbpf@kernel.org March 20, 2023, 9:20 a.m. UTC | #1 
Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Sat, 18 Mar 2023 01:19:00 +0200 you wrote:
> We collect the software statistics counters for RX bytes (reported to
> /proc/net/dev and to ethtool -S $dev | grep 'rx_bytes: ") at a time when
> skb->len has already been adjusted by the eth_type_trans() ->
> skb_pull_inline(skb, ETH_HLEN) call to exclude the L2 header.
> 
> This means that when connecting 2 DSA interfaces back to back and
> sending 1 packet with length 100, the sending interface will report
> tx_bytes as incrementing by 100, and the receiving interface will report
> rx_bytes as incrementing by 86.
> 
> [...]

Here is the summary with links:
  - [net] net: dsa: report rx_bytes unadjusted for ETH_HLEN
    https://git.kernel.org/netdev/net/c/a8eff03545d4

You are awesome, thank you!
Florian Fainelli March 20, 2023, 6:06 p.m. UTC | #2 
On 3/17/23 16:19, Vladimir Oltean wrote:
> We collect the software statistics counters for RX bytes (reported to
> /proc/net/dev and to ethtool -S $dev | grep 'rx_bytes: ") at a time when
> skb->len has already been adjusted by the eth_type_trans() ->
> skb_pull_inline(skb, ETH_HLEN) call to exclude the L2 header.
> 
> This means that when connecting 2 DSA interfaces back to back and
> sending 1 packet with length 100, the sending interface will report
> tx_bytes as incrementing by 100, and the receiving interface will report
> rx_bytes as incrementing by 86.
> 
> Since accounting for that in scripts is quirky and is something that
> would be DSA-specific behavior (requiring users to know that they are
> running on a DSA interface in the first place), the proposal is that we
> treat it as a bug and fix it.
> 
> This design bug has always existed in DSA, according to my analysis:
> commit 91da11f870f0 ("net: Distributed Switch Architecture protocol
> support") also updates skb->dev->stats.rx_bytes += skb->len after the
> eth_type_trans() call. Technically, prior to Florian's commit
> a86d8becc3f0 ("net: dsa: Factor bottom tag receive functions"), each and
> every vendor-specific tagging protocol driver open-coded the same bug,
> until the buggy code was consolidated into something resembling what can
> be seen now. So each and every driver should have its own Fixes: tag,
> because of their different histories until the convergence point.
> I'm not going to do that, for the sake of simplicity, but just blame the
> oldest appearance of buggy code.
> 
> There are 2 ways to fix the problem. One is the obvious way, and the
> other is how I ended up doing it. Obvious would have been to move
> dev_sw_netstats_rx_add() one line above eth_type_trans(), and below
> skb_push(skb, ETH_HLEN). But DSA processing is not as simple as that.
> We count the bytes after removing everything DSA-related from the
> packet, to emulate what the packet's length was, on the wire, when the
> user port received it.
> 
> When eth_type_trans() executes, dsa_untag_bridge_pvid() has not run yet,
> so in case the switch driver requests this behavior - commit
> 412a1526d067 ("net: dsa: untag the bridge pvid from rx skbs") has the
> details - the obvious variant of the fix wouldn't have worked, because
> the positioning there would have also counted the not-yet-stripped VLAN
> header length, something which is absent from the packet as seen on the
> wire (there it may be untagged, whereas software will see it as
> PVID-tagged).
> 
> Fixes: f613ed665bb3 ("net: dsa: Add support for 64-bit statistics")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Already applied, but I agree with you that this is the simplest way to 
go about fixign this. Thanks!
diff mbox series

Patch

diff --git a/net/dsa/tag.c b/net/dsa/tag.c
index b2fba1a003ce..5105a5ff58fa 100644
--- a/net/dsa/tag.c
+++ b/net/dsa/tag.c
@@ -114,7 +114,7 @@  static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 		skb = nskb;
 	}
 
-	dev_sw_netstats_rx_add(skb->dev, skb->len);
+	dev_sw_netstats_rx_add(skb->dev, skb->len + ETH_HLEN);
 
 	if (dsa_skb_defer_rx_timestamp(p, skb))
 		return 0;