netdev - Re: [PATCH net 1/2] net: dsa: be compatible with DSA masters with max

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]

Date:   Tue, 21 Apr 2020 17:24:54 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net 1/2] net: dsa: be compatible with DSA masters with
 max_mtu of 1500 or less

On Tue, 21 Apr 2020 at 17:06, Andrew Lunn <andrew@...n.ch> wrote:
>
> On Tue, Apr 21, 2020 at 04:42:41PM +0300, Vladimir Oltean wrote:
> > Hi Andrew,
> >
> > On Tue, 21 Apr 2020 at 16:33, Andrew Lunn <andrew@...n.ch> wrote:
> > >
> > > On Tue, Apr 21, 2020 at 03:31:09PM +0300, Vladimir Oltean wrote:
> > > > From: Vladimir Oltean <vladimir.oltean@....com>
> > > >
> > > > It would be ideal if the DSA switch ports would support an MTU of 1500
> > > > bytes by default, same as any other net device. But there are 2 cases of
> > > > issues with trying to do that:
> > > >
> > > > - Drivers that are legitimately MTU-challenged and don't support
> > > >   anything larger than ETH_DATA_LEN. A very quick search shows that
> > > >   sungem.c is one such example - there may be many others.
> > > >
> > > > - Drivers that simply don't populate netdev->max_mtu. In that case, it
> > > >   seems that the ether_setup function sets dev->max_mtu to a default
> > > >   value of ETH_DATA_LEN. And due to the above cases which really are
> > > >   MTU-challenged, we can't really make any guesses.
> > > >
> > > > So for these cases, if the max_mtu of the master net_device is lower
> > > > than 1500, use that (minus the tagger overhead) as the max MTU of the
> > > > switch ports.
> > >
> > > I don't like this. I suspect this will also break in subtle ways.
> > >
> > > Please go back to the original behaviour. Make the call to request the
> > > minimum needed for DSA.
> >
> > In what sense "minimum needed"? It is minimum needed. If
> > master->max_mtu is 1500, the MTU will be set to 1496.
>
> Ah, sorry. This is the slave. I was thinking it was the master.
>
> We have always assumed the slave can send normal sized frames,
> independent of what the master supports. This is just a follow on from
> the fact we ignore errors setting the MTU on the master for the DSA
> overhead for normal size frames. So don't set the MTU to 1496, leave
> it at 1500. For all working boards out in the wild, 1500 will work.
>
>          Andrew

Does iperf3 TCP work on your Vybrid board with the master MTU equal to
the slave MTU equal to 1500 (without IP fragmentation, that is)? If it
does, ok, this patch can maybe be dropped.

qca7000 doesn't support packets larger than 1500 MTU either, neither
does broadcom b44, and neither do probably more adapters which I
didn't find now.

Why would I not limit the MTU to 1496 if the master's max_mtu is 1500?
Just to provoke more warnings in people's logs? I just don't want to
have further issue reports, but I am basically reading your reply as
"don't try to be correct". If you don't like this behavior for FEC,
you can set its max_mtu since it doesn't do that by itself.

Thanks,
-Vladimir