linux-kernel - Re: [PATCH net] net: dsa: report rx_bytes unadjusted for ETH

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3b5ffccc-f1cd-b2f8-1e78-72997b43c12c@gmail.com>
Date:   Mon, 20 Mar 2023 11:06:49 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: report rx_bytes unadjusted for ETH_HLEN

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@....com>

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