netdev - Re: [PATCH net-next 1/3] net: dsa: Configure the MTU for switch ports

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: <fb158342-c2f5-f8d1-6987-1dbd79a11472@gmail.com>
Date:   Sat, 23 Nov 2019 13:47:10 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/3] net: dsa: Configure the MTU for switch ports



On 11/23/2019 1:29 PM, Vladimir Oltean wrote:
> On Sat, 23 Nov 2019 at 23:14, Florian Fainelli <f.fainelli@...il.com> wrote:
>>
>>
>>
>> On 11/23/2019 12:46 PM, Vladimir Oltean wrote:
>>>
>>> Correct. I was actually held back a bit while looking at Andrew's
>>> patch dc0fe7d47f9f ("net: dsa: Set the master device's MTU to account
>>> for DSA overheads") where he basically discarded errors, so that's the
>>> approach I took too (thinking that some DSA masters would not have ops
>>> for changing or reporting the MTU).
>>>
>>>> I had prepared a patch series with Murali doing nearly the same thing
>>>> and targeting Broadcom switches nearly a year ago but since I never got
>>>> feedback whether this worked properly for the use case he was after, I
>>>> did not submit it since I did not need it personally and found it to be
>>>> a nice can of worms.
>>>>
>>>
>>> Nice, do you mind if I take your series instead then?
>>
>> Not at all, if it works, please go ahead, not sure how hard it is going
>> to be to rebase.
>>
>>>
>>>> Another thing that I had not gotten around testing was making sure that
>>>> when a slave_dev gets enslaved as a bridge port member, that bridge MTU
>>>> normalization would kick in and make sure that if you have say: port 0
>>>> configured with MTU 1500 and port 1 configured with MTU 9000, the bridge
>>>> would normalize to MTU 1500 as you would expect.
>>>>
>>>
>>> Nope, that doesn't happen by default, at least in my implementation.
>>> Is there code in the bridge core for it?
>>
>> net/bridge/br_if.c::br_mtu_auto_adjust() takes care of adjusting the
>> bridge master device's MTU based on the minimum MTU of all ports within
>> the bridge, but what it seems to be missing is ensuring that if bridge
>> ports are enslaved, and those bridge ports happen to be part of the same
>> switch id (similar decision path to setting skb->fwd_offload_mark), then
>> the bridge port's MTU should also be auto adjusted. mlxsw also supports
>> changing the MTU, so I am surprised this is not something they fixed
>> already.
>>
> 
> But then how would you even change a bridged interface's MTU? Delete
> bridge, change MTU of all ports to same value, create bridge again?

I am afraid so, given that the NETDEV_CHANGEMTU even for which
br_device_event() listens to and processes with br_mtu_auto_adjust()
would lead to selecting the lowest MTU again. Unfortunately, I don't
really see a way to solve that other than walk all ports (which could be
any network device driver) and ask them if they support the new MTU of
that other port, and if so, commit, else rollback. Do you see another way?
-- 
Florian