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: <20171214013148.3da52cc9@elisabeth>
Date:   Thu, 14 Dec 2017 01:31:48 +0100
From:   Stefano Brivio <sbrivio@...hat.com>
To:     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 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.

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