Re: [PATCH net-next v2 7/9] net: use core MTU range checking in misc drivers

[Jarod Wilson]

On Sat, Oct 22, 2016 at 09:27:59PM +0200, Stefan Richter wrote:
> Adding Cc: linux1394-devel, dropping several Ccs, no additional comment.
> 
> On Oct 22 Stefan Richter wrote:
> > On Oct 20 Jarod Wilson wrote:
> > > firewire-net:
> > > - set min/max_mtu
> > > - remove fwnet_change_mtu  
> > [...]
> > > --- a/drivers/firewire/net.c
> > > +++ b/drivers/firewire/net.c
> > > @@ -1349,15 +1349,6 @@ static netdev_tx_t fwnet_tx(struct sk_buff
> > > *skb, struct net_device *net) return NETDEV_TX_OK;
> > >  }
> > >  
> > > -static int fwnet_change_mtu(struct net_device *net, int new_mtu)
> > > -{
> > > - if (new_mtu < 68)
> > > - return -EINVAL;
> > > -
> > > - net->mtu = new_mtu;
> > > - return 0;
> > > -}
> > > -
> > >  static const struct ethtool_ops fwnet_ethtool_ops = {
> > >   .get_link   = ethtool_op_get_link,
> > >  };
> > > @@ -1366,7 +1357,6 @@ static const struct net_device_ops
> > > fwnet_netdev_ops = { .ndo_open   = fwnet_open,
> > >   .ndo_stop   = fwnet_stop,
> > >   .ndo_start_xmit = fwnet_tx,
> > > - .ndo_change_mtu = fwnet_change_mtu,
> > >  };
> > >  
> > >  static void fwnet_init_dev(struct net_device *net)
> > > @@ -1435,7 +1425,6 @@ static int fwnet_probe(struct fw_unit *unit,
> > >   struct net_device *net;
> > >   bool allocated_netdev = false;
> > >   struct fwnet_device *dev;
> > > - unsigned max_mtu;
> > >   int ret;
> > >   union fwnet_hwaddr *ha;
> > >  
> > > @@ -1478,9 +1467,10 @@ static int fwnet_probe(struct fw_unit *unit,
> > >* Use the RFC 2734 default 1500 octets or the maximum payload
> > >* as initial MTU
> > >*/
> > > - max_mtu = (1 << (card->max_receive + 1))
> > > -   - sizeof(struct rfc2734_header) - IEEE1394_GASP_HDR_SIZE;
> > > - net->mtu = min(1500U, max_mtu);
> > > + net->max_mtu = (1 << (card->max_receive + 1))
> > > +- sizeof(struct rfc2734_header) - IEEE1394_GASP_HDR_SIZE;
> > > + net->mtu = min(1500U, net->max_mtu);
> > > + net->min_mtu = ETH_MIN_MTU;
> > >  
> > >   /* Set our hardware address while we're at it */
> > >   ha = (union fwnet_hwaddr *)net->dev_addr;
> > 
> > Please preserve the current behavior, i.e. do not enforce any particular
> > upper bound.  (Especially none based on the local link layer controller's
> > max_receive parameter.)
> > 
> > BTW, after having read RFC 2734, RFC 3146, and the code, I am convinced
> > that net->mtu should be initialized to 1500, not less.  But such a change
> > should be done in a separate patch.

Okay, since it's already merged in net-next, I can do a follow-up patch
here to set max_mtu to ETH_MAX_MTU (65535), which is the largest possible
size the kernel can handle, so far as I can tell. But as long as I'm going
to be in here, if we just want to use an initial mtu of 1500, I could
clean that up at the same time, and entirely remove the max_mtu
calculation stuff, if that's what you think is more correct here.

-- 
Jarod Wilson
ja...@redhat.com

Re: [PATCH net-next v2 7/9] net: use core MTU range checking in misc drivers

[Stefan Richter]

Adding Cc: linux1394-devel, dropping several Ccs, no additional comment.

On Oct 22 Stefan Richter wrote:
> On Oct 20 Jarod Wilson wrote:
> > firewire-net:
> > - set min/max_mtu
> > - remove fwnet_change_mtu  
> [...]
> > --- a/drivers/firewire/net.c
> > +++ b/drivers/firewire/net.c
> > @@ -1349,15 +1349,6 @@ static netdev_tx_t fwnet_tx(struct sk_buff
> > *skb, struct net_device *net) return NETDEV_TX_OK;
> >  }
> >  
> > -static int fwnet_change_mtu(struct net_device *net, int new_mtu)
> > -{
> > -   if (new_mtu < 68)
> > -   return -EINVAL;
> > -
> > -   net->mtu = new_mtu;
> > -   return 0;
> > -}
> > -
> >  static const struct ethtool_ops fwnet_ethtool_ops = {
> > .get_link   = ethtool_op_get_link,
> >  };
> > @@ -1366,7 +1357,6 @@ static const struct net_device_ops
> > fwnet_netdev_ops = { .ndo_open   = fwnet_open,
> > .ndo_stop   = fwnet_stop,
> > .ndo_start_xmit = fwnet_tx,
> > -   .ndo_change_mtu = fwnet_change_mtu,
> >  };
> >  
> >  static void fwnet_init_dev(struct net_device *net)
> > @@ -1435,7 +1425,6 @@ static int fwnet_probe(struct fw_unit *unit,
> > struct net_device *net;
> > bool allocated_netdev = false;
> > struct fwnet_device *dev;
> > -   unsigned max_mtu;
> > int ret;
> > union fwnet_hwaddr *ha;
> >  
> > @@ -1478,9 +1467,10 @@ static int fwnet_probe(struct fw_unit *unit,
> >  * Use the RFC 2734 default 1500 octets or the maximum payload
> >  * as initial MTU
> >  */
> > -   max_mtu = (1 << (card->max_receive + 1))
> > - - sizeof(struct rfc2734_header) - IEEE1394_GASP_HDR_SIZE;
> > -   net->mtu = min(1500U, max_mtu);
> > +   net->max_mtu = (1 << (card->max_receive + 1))
> > +  - sizeof(struct rfc2734_header) - IEEE1394_GASP_HDR_SIZE;
> > +   net->mtu = min(1500U, net->max_mtu);
> > +   net->min_mtu = ETH_MIN_MTU;
> >  
> > /* Set our hardware address while we're at it */
> > ha = (union fwnet_hwaddr *)net->dev_addr;
> 
> Please preserve the current behavior, i.e. do not enforce any particular
> upper bound.  (Especially none based on the local link layer controller's
> max_receive parameter.)
> 
> BTW, after having read RFC 2734, RFC 3146, and the code, I am convinced
> that net->mtu should be initialized to 1500, not less.  But such a change
> should be done in a separate patch.
-- 
Stefan Richter
-==- =-=- =-==-
http://arcgraph.de/sr/

Re: [PATCH net-next v2 7/9] net: use core MTU range checking in misc drivers

[Stefan Richter]

On Oct 20 Jarod Wilson wrote:
> firewire-net:
> - set min/max_mtu
> - remove fwnet_change_mtu
[...]
> --- a/drivers/firewire/net.c
> +++ b/drivers/firewire/net.c
[...]
> @@ -1478,9 +1467,10 @@ static int fwnet_probe(struct fw_unit *unit,
>* Use the RFC 2734 default 1500 octets or the maximum payload
>* as initial MTU
>*/
> - max_mtu = (1 << (card->max_receive + 1))
> -   - sizeof(struct rfc2734_header) - IEEE1394_GASP_HDR_SIZE;
> - net->mtu = min(1500U, max_mtu);
> + net->max_mtu = (1 << (card->max_receive + 1))
> +- sizeof(struct rfc2734_header) - IEEE1394_GASP_HDR_SIZE;
> + net->mtu = min(1500U, net->max_mtu);
> + net->min_mtu = ETH_MIN_MTU;
>  
>   /* Set our hardware address while we're at it */
>   ha = (union fwnet_hwaddr *)net->dev_addr;

Please preserve the current behavior, i.e. do not enforce any particular
upper bound.  (Especially none based on the local link layer controller's
max_receive parameter.)

BTW, after having read RFC 2734, RFC 3146, and the code, I am convinced
that net->mtu should be initialized to 1500, not less.  But such a change
should be done in a separate patch.
-- 
Stefan Richter
-==- =-=- =-==-
http://arcgraph.de/sr/

Re: [PATCH net-next v2 7/9] net: use core MTU range checking in misc drivers

[Sebastian Reichel]

Hi,

On Thu, Oct 20, 2016 at 01:55:22PM -0400, Jarod Wilson wrote:
> hsi/clients/ssi_protocol:
> - use core MTU range checking
> - remove now redundant ssip_pn_set_mtu

Acked-By: Sebastian Reichel 

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH net-next v2 7/9] net: use core MTU range checking in misc drivers

[Rémi Denis-Courmont]

Acked-by: Rémi Denis-Courmont 

-- 
Rémi Denis-Courmont
http://www.remlab.net/CV.pdf

[PATCH net-next v2 7/9] net: use core MTU range checking in misc drivers

[Jarod Wilson]

firewire-net:
- set min/max_mtu
- remove fwnet_change_mtu

nes:
- set max_mtu
- clean up nes_netdev_change_mtu

xpnet:
- set min/max_mtu
- remove xpnet_dev_change_mtu

hippi:
- set min/max_mtu
- remove hippi_change_mtu

batman-adv:
- set max_mtu
- remove batadv_interface_change_mtu
- initialization is a little async, not 100% certain that max_mtu is set
  in the optimal place, don't have hardware to test with

rionet:
- set min/max_mtu
- remove rionet_change_mtu

slip:
- set min/max_mtu
- streamline sl_change_mtu

um/net_kern:
- remove pointless ndo_change_mtu

hsi/clients/ssi_protocol:
- use core MTU range checking
- remove now redundant ssip_pn_set_mtu

ipoib:
- set a default max MTU value
- Note: ipoib's actual max MTU can vary, depending on if the device is in
  connected mode or not, so we'll just set the max_mtu value to the max
  possible, and let the ndo_change_mtu function continue to validate any new
  MTU change requests with checks for CM or not. Note that ipoib has no
  min_mtu set, and thus, the network core's mtu > 0 check is the only lower
  bounds here.

mptlan:
- use net core MTU range checking
- remove now redundant mpt_lan_change_mtu

fddi:
- min_mtu = 21, max_mtu = 4470
- remove now redundant fddi_change_mtu (including export)

fjes:
- min_mtu = 8192, max_mtu = 65536
- The max_mtu value is actually one over IP_MAX_MTU here, but the idea is to
  get past the core net MTU range checks so fjes_change_mtu can validate a
  new MTU against what it supports (see fjes_support_mtu in fjes_hw.c)

hsr:
- min_mtu = 0 (calls ether_setup, max_mtu is 1500)

f_phonet:
- min_mtu = 6, max_mtu = 65541

u_ether:
- min_mtu = 14, max_mtu = 15412

phonet/pep-gprs:
- min_mtu = 576, max_mtu = 65530
- remove redundant gprs_set_mtu

CC: netdev@vger.kernel.org
CC: linux-r...@vger.kernel.org
CC: Stefan Richter 
CC: Faisal Latif 
CC: linux-r...@vger.kernel.org
CC: Cliff Whickman 
CC: Robin Holt 
CC: Jes Sorensen 
CC: Marek Lindner 
CC: Simon Wunderlich 
CC: Antonio Quartulli 
CC: Sathya Prakash 
CC: Chaitra P B 
CC: Suganath Prabu Subramani 
CC: mpt-fusionlinux@broadcom.com
CC: Sebastian Reichel 
CC: Felipe Balbi 
CC: Arvid Brodin 
CC: Remi Denis-Courmont 
Signed-off-by: Jarod Wilson 
---
 arch/um/drivers/net_kern.c|  8 
 drivers/firewire/net.c| 18 --
 drivers/hsi/clients/ssi_protocol.c| 14 --
 drivers/infiniband/hw/nes/nes.c   |  1 -
 drivers/infiniband/hw/nes/nes.h   |  4 ++--
 drivers/infiniband/hw/nes/nes_nic.c   | 10 +++---
 drivers/infiniband/ulp/ipoib/ipoib_main.c |  1 +
 drivers/message/fusion/mptlan.c   | 15 ---
 drivers/misc/sgi-xp/xpnet.c   | 21 -
 drivers/net/fddi/skfp/skfddi.c|  1 -
 drivers/net/fjes/fjes_main.c  |  2 ++
 drivers/net/hippi/rrunner.c   |  1 -
 drivers/net/rionet.c  | 15 +++
 drivers/net/slip/slip.c   | 11 +--
 drivers/usb/gadget/function/f_phonet.c| 11 ++-
 drivers/usb/gadget/function/u_ether.c | 14 --
 include/linux/fddidevice.h|  1 -
 include/linux/hippidevice.h   |  1 -
 net/802/fddi.c| 11 ++-
 net/802/hippi.c   | 14 ++
 net/batman-adv/soft-interface.c   | 13 +
 net/hsr/hsr_device.c  |  1 +
 net/phonet/pep-gprs.c | 12 ++--
 23 files changed, 46 insertions(+), 154 deletions(-)

diff --git a/arch/um/drivers/net_kern.c b/arch/um/drivers/net_kern.c
index 2cd5b68..1669240 100644
--- a/arch/um/drivers/net_kern.c
+++ b/arch/um/drivers/net_kern.c
@@ -256,13 +256,6 @@ static void uml_net_tx_timeout(struct net_device *dev)
netif_wake_queue(dev);
 }
 
-static int uml_net_change_mtu(struct net_device *dev, int new_mtu)
-{
-   dev->mtu = new_mtu;
-
-   return 0;
-}
-
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void uml_net_poll_controller(struct net_device *dev)
 {
@@ -374,7 +367,6 @@ static const struct net_device_ops uml_netdev_ops = {
.ndo_set_rx_mode= uml_net_set_multicast_list,
.ndo_tx_timeout = uml_net_tx_timeout,
.ndo_set_mac_address= eth_mac_addr,
-   .ndo_change_mtu = uml_net_change_mtu,
.ndo_validate_addr  = eth_validate_addr,
 #ifdef CONFIG_NET_POLL_CONTROLLER
.ndo_poll_controller = uml_net_poll_controller,
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index