netdev - Re: [PATCH net-next 4/6] net: use core MTU range checking in core net infra

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:   Wed, 19 Oct 2016 10:51:43 -0400
From:   Jarod Wilson <jarod@...hat.com>
To:     Jiri Benc <jbenc@...hat.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Nicolas Dichtel <nicolas.dichtel@...nd.com>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>,
        Tom Herbert <tom@...bertland.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexander Duyck <alexander.h.duyck@...el.com>,
        Paolo Abeni <pabeni@...hat.com>,
        WANG Cong <xiyou.wangcong@...il.com>,
        Roopa Prabhu <roopa@...ulusnetworks.com>,
        Pravin B Shelar <pshelar@....org>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Patrick McHardy <kaber@...sh.net>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Pravin Shelar <pshelar@...ira.com>
Subject: Re: [PATCH net-next 4/6] net: use core MTU range checking in core
 net infra

On Wed, Oct 19, 2016 at 02:17:03PM +0200, Jiri Benc wrote:
> On Tue, 18 Oct 2016 22:33:31 -0400, Jarod Wilson wrote:
> > --- a/drivers/net/vxlan.c
> > +++ b/drivers/net/vxlan.c
> > @@ -2367,43 +2367,31 @@ static void vxlan_set_multicast_list(struct net_device *dev)
> >  {
> >  }
> >  
> > -static int __vxlan_change_mtu(struct net_device *dev,
> > -			      struct net_device *lowerdev,
> > -			      struct vxlan_rdst *dst, int new_mtu, bool strict)
> > +static int vxlan_change_mtu(struct net_device *dev, int new_mtu)
> >  {
> > -	int max_mtu = IP_MAX_MTU;
> > -
> > -	if (lowerdev)
> > -		max_mtu = lowerdev->mtu;
> > +	struct vxlan_dev *vxlan = netdev_priv(dev);
> > +	struct vxlan_rdst *dst = &vxlan->default_dst;
> > +	struct net_device *lowerdev = __dev_get_by_index(vxlan->net,
> > +							 dst->remote_ifindex);
> > +	bool use_ipv6 = false;
> >  
> >  	if (dst->remote_ip.sa.sa_family == AF_INET6)
> > -		max_mtu -= VXLAN6_HEADROOM;
> > -	else
> > -		max_mtu -= VXLAN_HEADROOM;
> > -
> > -	if (new_mtu < 68)
> > -		return -EINVAL;
> > +		use_ipv6 = true;
> >  
> > -	if (new_mtu > max_mtu) {
> > -		if (strict)
> > +	/* We re-check this, because users *could* alter the mtu of the
> > +	 * lower device after we've initialized dev->max_mtu.
> > +	 */
> > +	if (lowerdev) {
> > +		dev->max_mtu = lowerdev->mtu -
> > +			       (use_ipv6 ? VXLAN6_HEADROOM : VXLAN_HEADROOM);
> > +		if (new_mtu > dev->max_mtu)
> >  			return -EINVAL;
> > -
> > -		new_mtu = max_mtu;
> >  	}
> >  
> >  	dev->mtu = new_mtu;
> >  	return 0;
> >  }
> 
> Sorry for the silly question, how does the min_mtu and max_mtu stuff
> works? I noticed your patches but haven't looked in depth into them.
> 
> When the ndo_change_mtu callback is defined, is the dev->min_mtu and
> dev->max_mtu checked first and if the desired mtu is not within range,
> ndo_change_mtu is not called?
> 
> Or does ndo_change_mtu override the checks?

The former. If the new value is outside min/max, ndo_change_mtu doesn't
get called, which is exactly the chicken and egg problem I introduced by
setting max_mtu to 1500 in ether_setup before having all drivers that call
ether_setup set a more appropriate max_mtu first. :\

> In either case, the code does not look correct. In the first case,
> increasing of lowerdev MTU wouldn't allow increasing of vxlan MTU
> without deleting and recreating the vxlan interface. In the second
> case, you're missing check against the min_mtu.

Okay, this sounds like a similar case to bridge that Sabrina pointed out.
Looks like virtual devices will need to just set no max_mtu directly (or
IP_MAX_MTU), and do dynamic checks in their ndo_change_mtu if they need to
compare against underlying devices on the fly.

...
> > @@ -2847,9 +2842,14 @@ static int vxlan_dev_configure(struct net *src_net, struct net_device *dev,
> >  	}
> >  
> >  	if (conf->mtu) {
> > -		err = __vxlan_change_mtu(dev, lowerdev, dst, conf->mtu, false);
> > -		if (err)
> > -			return err;
> > +		if (lowerdev)
> > +			dev->max_mtu = lowerdev->mtu;
> > +		dev->max_mtu -= (use_ipv6 ? VXLAN6_HEADROOM : VXLAN_HEADROOM);
> > +
> > +		dev->mtu = conf->mtu;
> > +
> > +		if (conf->mtu > dev->max_mtu)
> > +			dev->mtu = dev->max_mtu;
> >  	}
> 
> You removed the check for min_mtu but it's needed here. The conf->mtu
> value comes from the user space and can be anything.

Hm. Not sure why I did that... Will put it back now...

-- 
Jarod Wilson
jarod@...hat.com