netdev - Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220308112156.6s2ssnkhifxuuw2w@skbuf>
Date:   Tue, 8 Mar 2022 13:21:56 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Woojung Huh <woojung.huh@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>,
        kernel@...gutronix.de, Jakub Kicinski <kuba@...nel.org>,
        UNGLinuxDriver@...rochip.com,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement
 MTU configuration

On Tue, Mar 08, 2022 at 11:06:44AM +0100, Oleksij Rempel wrote:
> > > > I was saying:
> > > > 
> > > > ip link set lan1 up
> > > > ip link add link lan1 name lan1.5 type vlan id 5
> > > > ip addr add 172.17.0.2/24 dev lan1.5 && ip link set lan1.5 up
> > > > iperf3 -c 172.17.0.10
> > > 
> > > It works.
> > 
> > This is akin to saying that without any calls to ksz9477_change_mtu(),
> > just writing VLAN_ETH_FRAME_LEN + ETH_FCS_LEN into REG_SW_MTU__2 is
> > sufficient to get VLAN-tagged MTU-sized packets to pass through the CPU
> > port and the lan1 user port.
> > 
> > So my question is: is this necessary?
> > 
> > 	if (dsa_is_cpu_port(ds, port))
> > 		new_mtu += KSZ9477_INGRESS_TAG_LEN;
> > 
> 
> No.
> 
> I did some extra tests with following results: REG_SW_MTU__2 should be
> configured to 1518 to pass 1514 frame. Independent if the frame is
> passed between external ports or external to CPU port. So, I assume,
> ETH_FRAME_LEN + ETH_FCS_LEN should be used instead of VLAN_ETH_FRAME_LEN
> + ETH_FCS_LEN. Correct?

Oleksij, to be clear, I only had an issue with consistency.
You were adding KSZ9477_INGRESS_TAG_LEN during ksz9477_change_mtu() but
not during initial setup. That prompted the question: is that particular
member of the sum needed or not? Either it's needed in both places, or
in none.

Then, apart from removing KSZ9477_INGRESS_TAG_LEN, you've also made an
unsolicited change (subtracted VLAN_HLEN from the value programmed to
hardware) without a clear confirmation that you understand what this
does, and without explicitly saying that the iperf3 test above still
works with this formula applied.

Since the VLAN header is part of L2, it means that a port configured for
MTU 1500 must also support VLAN-tagged packets with an L2 payload of
1500 octets. 1500 + ETH_HLEN + VLAN_HLEN == 1518 octets.
And since you need to add ETH_HLEN + ETH_FCS_LEN, I have an unconfirmed
hunch that VLAN_HLEN is also needed for the case above.

So, I'm sorry for being paranoid, but you aren't really giving me a
choice but to ask again, and again.