[PATCH net] net: remove MTU limits on a few ether_setup callers

[PATCH net] net: remove MTU limits on a few ether_setup callers

[Jarod Wilson]

These few drivers call ether_setup(), but have no ndo_change_mtu, and thus
were overlooked for changes to MTU range checking behavior. They
previously had no range checks, so for feature-parity, set their min_mtu
to 0 and max_mtu to ETH_MAX_MTU (65535), instead of the 68 and 1500
inherited from the ether_setup() changes. Fine-tuning can come after we get
back to full feature-parity here.

CC: netdev@vger.kernel.org
Reported-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
CC: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
CC: R Parameswaran <parameswaran.r7@gmail.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 net/atm/br2684.c            | 4 +++-
 net/bluetooth/bnep/netdev.c | 2 ++
 net/dsa/slave.c             | 2 ++
 net/irda/irlan/irlan_eth.c  | 3 ++-
 net/l2tp/l2tp_eth.c         | 2 ++
 5 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/atm/br2684.c b/net/atm/br2684.c
index c7d82f4..fca84e1 100644
--- a/net/atm/br2684.c
+++ b/net/atm/br2684.c
@@ -649,7 +649,9 @@ static void br2684_setup_routed(struct net_device *netdev)
 	netdev->hard_header_len = sizeof(llc_oui_ipv4); /* worst case */
 	netdev->netdev_ops = &br2684_netdev_ops_routed;
 	netdev->addr_len = 0;
-	netdev->mtu = 1500;
+	netdev->mtu = ETH_DATA_LEN;
+	netdev->min_mtu = 0;
+	netdev->max_mtu = ETH_MAX_MTU;
 	netdev->type = ARPHRD_PPP;
 	netdev->flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST;
 	netdev->tx_queue_len = 100;
diff --git a/net/bluetooth/bnep/netdev.c b/net/bluetooth/bnep/netdev.c
index 0f25ddc..2b875ed 100644
--- a/net/bluetooth/bnep/netdev.c
+++ b/net/bluetooth/bnep/netdev.c
@@ -221,6 +221,8 @@ void bnep_net_setup(struct net_device *dev)
 	dev->addr_len = ETH_ALEN;
 
 	ether_setup(dev);
+	dev->min_mtu = 0;
+	dev->max_mtu = ETH_MAX_MTU;
 	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 	dev->netdev_ops = &bnep_netdev_ops;
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 68714a5..d0c7bce 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1247,6 +1247,8 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	slave_dev->priv_flags |= IFF_NO_QUEUE;
 	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
 	slave_dev->switchdev_ops = &dsa_slave_switchdev_ops;
+	slave_dev->min_mtu = 0;
+	slave_dev->max_mtu = ETH_MAX_MTU;
 	SET_NETDEV_DEVTYPE(slave_dev, &dsa_type);
 
 	netdev_for_each_tx_queue(slave_dev, dsa_slave_set_lockdep_class_one,
diff --git a/net/irda/irlan/irlan_eth.c b/net/irda/irlan/irlan_eth.c
index 8192eae..74d09f9 100644
--- a/net/irda/irlan/irlan_eth.c
+++ b/net/irda/irlan/irlan_eth.c
@@ -66,7 +66,8 @@ static void irlan_eth_setup(struct net_device *dev)
 
 	dev->netdev_ops		= &irlan_eth_netdev_ops;
 	dev->destructor		= free_netdev;
-
+	dev->min_mtu		= 0;
+	dev->max_mtu		= ETH_MAX_MTU;
 
 	/*
 	 * Lets do all queueing in IrTTP instead of this device driver.
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 965f7e3..e2c6ae0 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -259,6 +259,8 @@ static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 p
 		session->mtu = dev->mtu - session->hdr_len;
 	dev->mtu = session->mtu;
 	dev->needed_headroom += session->hdr_len;
+	dev->min_mtu = 0;
+	dev->max_mtu = ETH_MAX_MTU;
 
 	priv = netdev_priv(dev);
 	priv->dev = dev;
-- 
2.10.0

Re: [PATCH net] net: remove MTU limits on a few ether_setup callers

[Florian Fainelli]

Le 20/10/2016 à 20:25, Jarod Wilson a écrit :
> These few drivers call ether_setup(), but have no ndo_change_mtu, and thus
> were overlooked for changes to MTU range checking behavior. They
> previously had no range checks, so for feature-parity, set their min_mtu
> to 0 and max_mtu to ETH_MAX_MTU (65535), instead of the 68 and 1500
> inherited from the ether_setup() changes. Fine-tuning can come after we get
> back to full feature-parity here.
> 
> CC: netdev@vger.kernel.org
> Reported-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> CC: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> CC: R Parameswaran <parameswaran.r7@gmail.com>
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---

> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 68714a5..d0c7bce 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1247,6 +1247,8 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
>  	slave_dev->priv_flags |= IFF_NO_QUEUE;
>  	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
>  	slave_dev->switchdev_ops = &dsa_slave_switchdev_ops;
> +	slave_dev->min_mtu = 0;
> +	slave_dev->max_mtu = ETH_MAX_MTU;
>  	SET_NETDEV_DEVTYPE(slave_dev, &dsa_type);

Actually for DSA, a reasonable minimum is probably 68 for the following
reasons: ETH_ZLEN of 60 bytes + FCS (4 bytes) + 2/4/8 bytes of Ethernet
switch tag (which is dependent on the tagging protocol used). Such an
Ethernet interface would submit packets through the conduit interface
which is connected to an Ethernet switches, and those typically discard
packets smaller than 64 bytes +/- their custom tag length.

One driver for sure pads the packets to the appropriate length:
bcmsysport, but I don't know about e1000e or e.g: mv643xx_eth which are
also commonly used for the same purposes.
-- 
Florian

Re: [PATCH net] net: remove MTU limits on a few ether_setup callers

[Andrew Lunn]

On Thu, Oct 20, 2016 at 08:42:46PM -0700, Florian Fainelli wrote:
> Le 20/10/2016 à 20:25, Jarod Wilson a écrit :
> > These few drivers call ether_setup(), but have no ndo_change_mtu, and thus
> > were overlooked for changes to MTU range checking behavior. They
> > previously had no range checks, so for feature-parity, set their min_mtu
> > to 0 and max_mtu to ETH_MAX_MTU (65535), instead of the 68 and 1500
> > inherited from the ether_setup() changes. Fine-tuning can come after we get
> > back to full feature-parity here.
> > 
> > CC: netdev@vger.kernel.org
> > Reported-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> > CC: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> > CC: R Parameswaran <parameswaran.r7@gmail.com>
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> > ---
> 
> > diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> > index 68714a5..d0c7bce 100644
> > --- a/net/dsa/slave.c
> > +++ b/net/dsa/slave.c
> > @@ -1247,6 +1247,8 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
> >  	slave_dev->priv_flags |= IFF_NO_QUEUE;
> >  	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
> >  	slave_dev->switchdev_ops = &dsa_slave_switchdev_ops;
> > +	slave_dev->min_mtu = 0;
> > +	slave_dev->max_mtu = ETH_MAX_MTU;
> >  	SET_NETDEV_DEVTYPE(slave_dev, &dsa_type);
> 
> Actually for DSA, a reasonable minimum is probably 68 for the following
> reasons: ETH_ZLEN of 60 bytes + FCS (4 bytes) + 2/4/8 bytes of
> Ethernet switch tag (which is dependent on the tagging protocol
> used).

Humm, isn't the switch tag added by the layer below this? So this
should be - 2/4/8 bytes, so that the assembled frame that actually
hits the conduit interface has an MTU of 64.

> Such an Ethernet interface would submit packets through the conduit
> interface which is connected to an Ethernet switches, and those
> typically discard packets smaller than 64 bytes +/- their custom tag
> length.

I have a purely theoretical observation, i.e. i have not checked the
datasheets. You can also control some of the Marvell switches by
sending it ethernet frames containing commands. Most commands are 4
bytes long. So an Ethernet header + 4 bytes is < 64. I expect the
switch will accept command frames which are padded to stop them being
runts. Also, such frames will be submitted to the conduit interface,
not the slave interface.

    Andrew

Re: [PATCH net] net: remove MTU limits on a few ether_setup callers

[Jarod Wilson]

On Fri, Oct 21, 2016 at 10:44:41AM +0200, Andrew Lunn wrote:
> On Thu, Oct 20, 2016 at 08:42:46PM -0700, Florian Fainelli wrote:
> > Le 20/10/2016 à 20:25, Jarod Wilson a écrit :
> > > These few drivers call ether_setup(), but have no ndo_change_mtu, and thus
> > > were overlooked for changes to MTU range checking behavior. They
> > > previously had no range checks, so for feature-parity, set their min_mtu
> > > to 0 and max_mtu to ETH_MAX_MTU (65535), instead of the 68 and 1500
> > > inherited from the ether_setup() changes. Fine-tuning can come after we get
> > > back to full feature-parity here.
> > > 
> > > CC: netdev@vger.kernel.org
> > > Reported-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> > > CC: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> > > CC: R Parameswaran <parameswaran.r7@gmail.com>
> > > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> > > ---
> > 
> > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> > > index 68714a5..d0c7bce 100644
> > > --- a/net/dsa/slave.c
> > > +++ b/net/dsa/slave.c
> > > @@ -1247,6 +1247,8 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
> > >  	slave_dev->priv_flags |= IFF_NO_QUEUE;
> > >  	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
> > >  	slave_dev->switchdev_ops = &dsa_slave_switchdev_ops;
> > > +	slave_dev->min_mtu = 0;
> > > +	slave_dev->max_mtu = ETH_MAX_MTU;
> > >  	SET_NETDEV_DEVTYPE(slave_dev, &dsa_type);
> > 
> > Actually for DSA, a reasonable minimum is probably 68 for the following
> > reasons: ETH_ZLEN of 60 bytes + FCS (4 bytes) + 2/4/8 bytes of
> > Ethernet switch tag (which is dependent on the tagging protocol
> > used).
> 
> Humm, isn't the switch tag added by the layer below this? So this
> should be - 2/4/8 bytes, so that the assembled frame that actually
> hits the conduit interface has an MTU of 64.

I'm all for fine-tuning things to be more correct, but my initial approach
here was to restore the ranges that were previously there, and DSA had no
upper or lower bounds checks. I'm not at all familiar with DSA either way.

> > Such an Ethernet interface would submit packets through the conduit
> > interface which is connected to an Ethernet switches, and those
> > typically discard packets smaller than 64 bytes +/- their custom tag
> > length.
> 
> I have a purely theoretical observation, i.e. i have not checked the
> datasheets. You can also control some of the Marvell switches by
> sending it ethernet frames containing commands. Most commands are 4
> bytes long. So an Ethernet header + 4 bytes is < 64. I expect the
> switch will accept command frames which are padded to stop them being
> runts. Also, such frames will be submitted to the conduit interface,
> not the slave interface.

So presumably, slave_dev->min_mtu wouldn't come into play in that
scenario, and we wouldn't break that functionality if min_mtu did get get
to 64 or 68 or whatever > 0.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH net] net: remove MTU limits on a few ether_setup callers

[Andrew Lunn]

> I'm all for fine-tuning things to be more correct, but my initial
> approach here was to restore the ranges that were previously there,
> and DSA had no upper or lower bounds checks. I'm not at all familiar
> with DSA either way.

Please just restore the old behaviour.

We can consider changes later.

       Andrew

Re: [PATCH net] net: remove MTU limits on a few ether_setup callers

[David Miller]

From: Jarod Wilson <jarod@redhat.com>
Date: Thu, 20 Oct 2016 23:25:27 -0400

> These few drivers call ether_setup(), but have no ndo_change_mtu, and thus
> were overlooked for changes to MTU range checking behavior. They
> previously had no range checks, so for feature-parity, set their min_mtu
> to 0 and max_mtu to ETH_MAX_MTU (65535), instead of the 68 and 1500
> inherited from the ether_setup() changes. Fine-tuning can come after we get
> back to full feature-parity here.
> 
> CC: netdev@vger.kernel.org
> Reported-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> CC: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> CC: R Parameswaran <parameswaran.r7@gmail.com>
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

Applied.

Please use "[PATCH net-next]" in your subject line when a patch
targets net-next.