netdev - Re: [PATCH net] net: dsa: apply MTU normalization for ports that join a LAG under a bridge too

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20210811225346.46l3qd5kwtv5zglf@skbuf>
Date:   Thu, 12 Aug 2021 01:53:46 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH net] net: dsa: apply MTU normalization for ports that
 join a LAG under a bridge too

On Wed, Aug 11, 2021 at 03:38:33PM -0700, Jakub Kicinski wrote:
> On Wed, 11 Aug 2021 19:44:41 +0300 Vladimir Oltean wrote:
> > On Wed, Aug 11, 2021 at 03:45:20PM +0300, Vladimir Oltean wrote:
> > > We want the MTU normalization logic to apply each time
> > > dsa_port_bridge_join is called, so instead of chasing all callers of
> > > that function, we should move the call within the bridge_join function
> > > itself.
> > > 
> > > Fixes: 185c9a760a61 ("net: dsa: call dsa_port_bridge_join when joining a LAG that is already in a bridge")
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > > ---  
> > 
> > I forgot to rebase this patch on top of 'net' and now I notice that it
> > conflicts with the switchdev_bridge_port_offload() work.
> > 
> > Do we feel that the issue this patch fixes isn't too big and the patch
> > can go into net-next, to avoid conflicts and all that?
> 
> The commit message doesn't really describe the impact so hard to judge,
> but either way you want to go - we'll need a repost so it can be build
> tested.

The impact is that if a DSA interface joins a bonding/team that is in a
bridge and the bonding/team had an MTU of 9000 bytes, the DSA interface
will still have what it had before (probably 1500). I found this through
code inspection, didn't hit an actual bug or anything like that. It
seems fairly low impact to me, and a rare occurrence in any case. The
common path is for the DSA interface to first join the bond, then the
bond to join the bridge anyway, and that would work I think.

Later edit: I just realized, while typing, that it's going to be more
complicated than that, so I'm going to just drop the patch at least for
now. While bond_enslave() does in fact make the lower interface inherit
the bond's MTU, that's not what we want here. DSA switches want their
bridged ports to have the same MTU, and they change it dynamically
whenever one MTU changes, but they don't change the MTU of any upper
interfaces they might have. So with the normalization logic as applied
by this patch, ports that join a bond will have an MTU of 9000, but the
bond itself will still have 1500, since
(a) DSA doesn't change it
(b) the bonding driver has a NETDEV_CHANGEMTU handler implementation
    which just wonders whether it should let DSA lowers change their MTU
    in the first place or not.

> Conflicts are not a huge deal. Obviously always best to wait for trees
> to merge if that fixes things, but if net has dependency on net-next
> you should just describe what you want the resolution to look like we
> should be able to execute :)

Yeah, thanks, I noticed you duly applied the instructions in the last
conflicting patch I sent exactly as described, and I appreciate it.
But I don't like conflicts either way, they're a pain to deal with when
you're backporting patches, since you can never get a clean cherry pick
list, so I try to avoid them myself now if I can.