netdev - Re: Minimum MTU Mess

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, 7 Sep 2016 19:43:06 -0400
From:   Jarod Wilson <jarod@...hat.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: Minimum MTU Mess

On Wed, Sep 07, 2016 at 10:31:12PM +0200, Andrew Lunn wrote:
> Hi Jarod
> 
> > -	/*	MTU must be positive.	 */
> > -	if (new_mtu < 0)
> > +	if (new_mtu < dev->min_mtu) {
> > +		netdev_err(dev, "Invalid MTU %d requested, hw min %d\n",
> > +			   new_mtu, dev->min_mtu);
> >  		return -EINVAL;
> > +	}
> > +
> > +	if (new_mtu > dev->max_mtu) {
> > +		netdev_err(dev, "Invalid MTU %d requested, hw max %d\n",
> > +			   new_mtu, dev->min_mtu);
> > +		return -EINVAL;
> > +	}
> 
> I doubt you can make such a big change like this in one go. Can you
> really guarantee all interfaces, of what ever type, will have some
> value for dev->min_mtu and dev->max_mtu? What may fly is something
> more like:
> 
> > +	if (dev->max_mtu && new_mtu > dev->max_mtu) {
> > +		netdev_err(dev, "Invalid MTU %d requested, hw max %d\n",
> > +			   new_mtu, dev->min_mtu);
> > +		return -EINVAL;
> > +	}
> 
> Maybe in a few cycles you can add a WARN_ON(!dev->max_mtu), and a few
> cycles after that go with (new_mtu > dev->max_mtu).

My local tree actually has if (dev->max_mtu > 0 && new_mtu > dev->max_mtu)
since just after I'd sent my mail for exactly that reason, though looking
at alloc_netdev_mqs(), it does seem we're at least guaranteed the value
will be 0 if not otherwise initialized, so your version looks perfectly
fine, and in the min_mtu case, without any additional handling, things
behave exactly as they did before. This is definitely going to require a
few passes... (Working my way through every driver with an ndo_change_mtu
wired up right now to see just how crazy this might get).

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