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

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

[Stefano Brivio]

Commit a985343ba906 ("vxlan: refactor verification and
application of configuration") introduced a change in the
behaviour of initial MTU setting: earlier, the MTU for a link
created on top of a given lower device, without an initial MTU
specification, was set to the MTU of the lower device minus
headroom as a result of this path in vxlan_dev_configure():

	if (!conf->mtu)
		dev->mtu = lowerdev->mtu -
			   (use_ipv6 ? VXLAN6_HEADROOM : VXLAN_HEADROOM);

which is now gone. Now, the initial MTU, in absence of a
configured value, is simply set by ether_setup() to ETH_DATA_LEN
(1500 bytes).

This breaks userspace expectations in case the MTU of
the lower device is higher than 1500 bytes minus headroom.

Restore the previous behaviour by calculating, for a new link,
the MTU from the lower device, if present, and if no value is
explicitly configured.

Reported-by: Junhan Yan <juyan@redhat.com>
Fixes: a985343ba906 ("vxlan: refactor verification and application of configuration")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
I guess this should be queued up for -stable, back to 4.13.

I'm actually introducing the third occurrence of this calculation (there's
another one in vxlan_config_apply(), and one in vxlan_change_mtu()). I would
anyway fix the userspace breakage first, and then plan on getting rid of several
bits of MTU logic duplication, which spans further than this.

 drivers/net/vxlan.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 19b9cc51079e..3a7e36cdf2c7 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3085,6 +3085,9 @@ static void vxlan_config_apply(struct net_device *dev,
 
 		if (conf->mtu)
 			dev->mtu = conf->mtu;
+		else if (lowerdev)
+			dev->mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
+							       VXLAN_HEADROOM);
 
 		vxlan->net = src_net;
 	}
-- 
2.9.4

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

[Stephen Hemminger]

On Wed, 13 Dec 2017 23:37:00 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> Commit a985343ba906 ("vxlan: refactor verification and
> application of configuration") introduced a change in the
> behaviour of initial MTU setting: earlier, the MTU for a link
> created on top of a given lower device, without an initial MTU
> specification, was set to the MTU of the lower device minus
> headroom as a result of this path in vxlan_dev_configure():
> 
> 	if (!conf->mtu)
> 		dev->mtu = lowerdev->mtu -
> 			   (use_ipv6 ? VXLAN6_HEADROOM : VXLAN_HEADROOM);
> 
> which is now gone. Now, the initial MTU, in absence of a
> configured value, is simply set by ether_setup() to ETH_DATA_LEN
> (1500 bytes).
> 
> This breaks userspace expectations in case the MTU of
> the lower device is higher than 1500 bytes minus headroom.
> 
> Restore the previous behaviour by calculating, for a new link,
> the MTU from the lower device, if present, and if no value is
> explicitly configured.
> 
> Reported-by: Junhan Yan <juyan@redhat.com>
> Fixes: a985343ba906 ("vxlan: refactor verification and application of configuration")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---

Good catch.
Acked-by: Stephen Hemminger <stephen@networkplumber.org>

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

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 2574 bytes --]

On 12/13/2017 11:37 PM, Stefano Brivio wrote:
> Commit a985343ba906 ("vxlan: refactor verification and
> application of configuration") introduced a change in the
> behaviour of initial MTU setting: earlier, the MTU for a link
> created on top of a given lower device, without an initial MTU
> specification, was set to the MTU of the lower device minus
> headroom as a result of this path in vxlan_dev_configure():
> 
> 	if (!conf->mtu)
> 		dev->mtu = lowerdev->mtu -
> 			   (use_ipv6 ? VXLAN6_HEADROOM : VXLAN_HEADROOM);
> 
> which is now gone. Now, the initial MTU, in absence of a
> configured value, is simply set by ether_setup() to ETH_DATA_LEN
> (1500 bytes).
> 
> This breaks userspace expectations in case the MTU of
> the lower device is higher than 1500 bytes minus headroom.
> 
> Restore the previous behaviour by calculating, for a new link,
> the MTU from the lower device, if present, and if no value is
> explicitly configured.
> 
> Reported-by: Junhan Yan <juyan@redhat.com>
> Fixes: a985343ba906 ("vxlan: refactor verification and application of configuration")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> I guess this should be queued up for -stable, back to 4.13.
> 
> I'm actually introducing the third occurrence of this calculation (there's
> another one in vxlan_config_apply(), and one in vxlan_change_mtu()). I would
> anyway fix the userspace breakage first, and then plan on getting rid of several
> bits of MTU logic duplication, which spans further than this.

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.

Regards,
Matthias


> 
>  drivers/net/vxlan.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 19b9cc51079e..3a7e36cdf2c7 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -3085,6 +3085,9 @@ static void vxlan_config_apply(struct net_device *dev,
>  
>  		if (conf->mtu)
>  			dev->mtu = conf->mtu;
> +		else if (lowerdev)
> +			dev->mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
> +							       VXLAN_HEADROOM);
>  
>  		vxlan->net = src_net;
>  	}
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefano Brivio]

On Thu, 14 Dec 2017 00:57:32 +0100
Matthias Schiffer <mschiffer@universe-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.

-- 
Stefano

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

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 2397 bytes --]

On 12/14/2017 01:10 AM, Stefano Brivio wrote:
> On Thu, 14 Dec 2017 00:57:32 +0100
> Matthias Schiffer <mschiffer@universe-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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefano Brivio]

On Thu, 14 Dec 2017 01:25:40 +0100
Matthias Schiffer <mschiffer@universe-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@universe-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.

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.

-- 
Stefano

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

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 3044 bytes --]

On 12/14/2017 01:31 AM, Stefano Brivio wrote:
> On Thu, 14 Dec 2017 01:25:40 +0100
> Matthias Schiffer <mschiffer@universe-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@universe-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.
> 
> 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.
> 

Hmm, right. So here's a

Reviewed-by: Matthias Schiffer <mschiffer@universe-factory.net>

for the minimal change for -stable.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Alexey Kodanev]

On 12/14/2017 03:31 AM, Stefano Brivio wrote:
> On Thu, 14 Dec 2017 01:25:40 +0100
> Matthias Schiffer <mschiffer@universe-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@universe-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.
> 

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

[Stefano Brivio]

On Thu, 14 Dec 2017 14:23:36 +0300
Alexey Kodanev <alexey.kodanev@oracle.com> wrote:

> On 12/14/2017 03:31 AM, Stefano Brivio wrote:
> > On Thu, 14 Dec 2017 01:25:40 +0100
> > Matthias Schiffer <mschiffer@universe-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@universe-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...

Indeed it might happen to be negative (only for IPv6, down to -2), good
catch.

For the benefit of others: it took me a few minutes to see how this is
*not* unrelated, because we are introducing a direct assignment of
dev->mtu to set max_mtu, whereas earlier it was just used in
comparisons, so it didn't matter whether it was negative.

> 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;

I don't really have a strong preference here. On one hand, you're
hiding this a bit from the "device creation" path. On the other hand,
it's a bit more compact. So I'm also fine with this.

Can you perhaps submit a formal patch?

-- 
Stefano

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

[Alexey Kodanev]

On 12/14/2017 03:36 PM, Stefano Brivio wrote:
> On Thu, 14 Dec 2017 14:23:36 +0300
> Alexey Kodanev <alexey.kodanev@oracle.com> wrote:
> 
>> On 12/14/2017 03:31 AM, Stefano Brivio wrote:
...
>>
>> 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...
> 
> Indeed it might happen to be negative (only for IPv6, down to -2), good
> catch.
> 
> For the benefit of others: it took me a few minutes to see how this is
> *not* unrelated, because we are introducing a direct assignment of
> dev->mtu to set max_mtu, whereas earlier it was just used in
> comparisons, so it didn't matter whether it was negative.
> 
>> 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;
> 
> I don't really have a strong preference here. On one hand, you're
> hiding this a bit from the "device creation" path. On the other hand,
> it's a bit more compact. So I'm also fine with this.
> 
> Can you perhaps submit a formal patch?
> 

OK, I'll send a patch.

Thanks,
Alexey