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]

Message-ID: <42862507-a573-af7a-d4aa-fd8cdd89c01e@universe-factory.net>
Date:   Thu, 14 Dec 2017 01:25:40 +0100
From:   Matthias Schiffer <mschiffer@...verse-factory.net>
To:     Stefano Brivio <sbrivio@...hat.com>
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 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;
>  
>  	if (use_ipv6 || conf->flags & VXLAN_F_COLLECT_METADATA)


Regards,
Matthias



Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)