netdev - Re: [PATCH net] vxlan: Restore initial MTU setting based on lower device

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:   Thu, 14 Dec 2017 14:23:36 +0300
From:   Alexey Kodanev <alexey.kodanev@...cle.com>
To:     Stefano Brivio <sbrivio@...hat.com>,
        Matthias Schiffer <mschiffer@...verse-factory.net>
Cc:     "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Junhan Yan <juyan@...hat.com>, Jiri Benc <jbenc@...hat.com>,
        Hangbin Liu <haliu@...hat.com>
Subject: Re: [PATCH net] vxlan: Restore initial MTU setting based on lower
 device

On 12/14/2017 03:31 AM, Stefano Brivio wrote:
> On Thu, 14 Dec 2017 01:25:40 +0100
> Matthias Schiffer <mschiffer@...verse-factory.net> wrote:
> 
>> On 12/14/2017 01:10 AM, Stefano Brivio wrote:
>>> On Thu, 14 Dec 2017 00:57:32 +0100
>>> Matthias Schiffer <mschiffer@...verse-factory.net> wrote:
>>>   
>>>> As you note, there is another occurrence of this calculation in
>>>> vxlan_config_apply():
>>>>
>>>>
>>>> [...]
>>>>         if (lowerdev) {
>>>> [...]
>>>>                 max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
>>>>                                            VXLAN_HEADROOM);
>>>>         }
>>>>
>>>>         if (dev->mtu > max_mtu)
>>>>                 dev->mtu = max_mtu;
>>>> [...]
>>>>
>>>>
>>>> Unless I'm overlooking something, this should already do the same thing and
>>>> your patch is redundant.  
>>>
>>> The code above sets max_mtu, and only if dev->mtu exceeds that, the
>>> latter is then clamped.
>>>
>>> What my patch does is to actually set dev->mtu to that value, no matter
>>> what's the previous value set by ether_setup() (only on creation, and
>>> only if lowerdev is there), just like the previous behaviour used to be.
>>>
>>> Let's consider these two cases, on the existing code:
>>>
>>> 1. lowerdev->mtu is 1500:
>>>    - ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
>>>    - here max_mtu is 1450
>>>    - we enter the second if clause above (dev->mtu > max_mtu)
>>>    - at the end of vxlan_config_apply(), dev->mtu will be 1450
>>>
>>> which is consistent with the previous behaviour.
>>>
>>> 2. lowerdev->mtu is 9000:
>>>    - ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
>>>    - here max_mtu is 8950
>>>    - we do not enter the second if clause above (dev->mtu < max_mtu)
>>>    - at the end of vxlan_config_apply(), dev->mtu will still be 1500
>>>
>>> which is not consistent with the previous behaviour, where it used to
>>> be 8950 instead.  
>>
>> Ah, thank you for the explanation, I was missing the context that this was
>> about higher rather than lower MTUs.
>>
>> Personally, I would prefer a change like the following, as it does not
>> introduce another duplication of the MTU calculation (not tested at all):
>>
>>> --- a/drivers/net/vxlan.c
>>> +++ b/drivers/net/vxlan.c
>>> @@ -3105,7 +3105,7 @@ static void vxlan_config_apply(struct net_device *dev,
>>>  					   VXLAN_HEADROOM);
>>>  	}
>>>  
>>> -	if (dev->mtu > max_mtu)
>>> +	if (dev->mtu > max_mtu || (!changelink && !conf->mtu))
>>>  		dev->mtu = max_mtu;
> 
> You would also need to check that lowerdev is present, though.
> 


if we move it up in "if (lowerdev) { ..." branch we will be checking the presence
of "lowerdev" and also not calculating it again. Also I would check max_mtu for
minimum as it might happen to be negative, though unlikely corner case...


diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 19b9cc5..1000b0e 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3103,6 +3103,11 @@ static void vxlan_config_apply(struct net_device *dev,

                max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
                                           VXLAN_HEADROOM);
+               if (max_mtu < ETH_MIN_MTU)
+                       max_mtu = ETH_MIN_MTU;
+
+               if (!changelink && !conf->mtu)
+                       dev->mtu = max_mtu;
        }

        if (dev->mtu > max_mtu)


Thanks,
Alexey


> Otherwise, you're changing the behaviour again, that is, if lowerdev is
> not present, we want to keep 1500 and not set ETH_MAX_MTU (65535).
> 
> Sure you can change the if condition to reflect that, but IMHO it
> becomes quite awkward.
>