* Minimum MTU Mess @ 2016-09-02 17:07 Jarod Wilson 2016-09-06 16:50 ` [PATCH net-next] sfc: check MTU against minimum threshold Bert Kenward 2016-09-06 23:55 ` Minimum MTU Mess David Miller 0 siblings, 2 replies; 16+ messages in thread From: Jarod Wilson @ 2016-09-02 17:07 UTC (permalink / raw) To: netdev So... I had a bug reported, about a NIC that ceased to work, if it's MTU was set to 0, then back to it's original value (1500). This got me thinking... What does an MTU of 0 even mean? Why should it be allowed? As it turns out, most (but not all) network drivers have a check in their ndo_change_mtu function that returns -EINVAL if new_mtu < $magic_number, which is often 68 (per page 25 of RFC 791), but is sometimes 64, or 60, or 46... Sometimes other manipulations are done. But the short version is that it seems there's an almost universal need to check for a minimum MTU. There's just some disagreement on what that minimum is. So, rather than having nearly every ndo_change_mut callback do the exact same thing, would it make sense to settle on one minimum MTU value, and check that unilaterally (at least for certain netdev types) in net/core/dev.c's dev_set_mtu()? Or is intentionally left vague, because it's really up to the hardware to care? Alternatively, perhaps each driver should set a netdev->min_mtu value, and net/core/dev.c could check against that. Could even have a centralized IP_MIN_MTU of 68 that all devices using ether_setup() and/or alloc_etherdev() used by default. In any case, the number of "mtu < 68" and "#define FOO_MIN_MTU 68", or variations thereof, under drivers/net/ is kind of crazy. In the interim, I think I'll just do a 3-line patch for this one driver that mirrors all the existing drivers to keep it from going splat... -- Jarod Wilson jarod@redhat.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net-next] sfc: check MTU against minimum threshold 2016-09-02 17:07 Minimum MTU Mess Jarod Wilson @ 2016-09-06 16:50 ` Bert Kenward 2016-09-06 21:31 ` Jarod Wilson 2016-09-06 23:54 ` David Miller 2016-09-06 23:55 ` Minimum MTU Mess David Miller 1 sibling, 2 replies; 16+ messages in thread From: Bert Kenward @ 2016-09-06 16:50 UTC (permalink / raw) To: Dave Miller; +Cc: netdev, linux-net-drivers, Jarod Wilson Reported-by: Ma Yuying <yuma@redhat.com> Suggested-by: Jarod Wilson <jarod@redhat.com> Signed-off-by: Bert Kenward <bkenward@solarflare.com> --- drivers/net/ethernet/sfc/efx.c | 12 +++++++++++- drivers/net/ethernet/sfc/net_driver.h | 3 +++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c index f3826ae..3cf3557 100644 --- a/drivers/net/ethernet/sfc/efx.c +++ b/drivers/net/ethernet/sfc/efx.c @@ -2263,8 +2263,18 @@ static int efx_change_mtu(struct net_device *net_dev, int new_mtu) rc = efx_check_disabled(efx); if (rc) return rc; - if (new_mtu > EFX_MAX_MTU) + if (new_mtu > EFX_MAX_MTU) { + netif_err(efx, drv, efx->net_dev, + "Requested MTU of %d too big (max: %d)\n", + new_mtu, EFX_MAX_MTU); return -EINVAL; + } + if (new_mtu < EFX_MIN_MTU) { + netif_err(efx, drv, efx->net_dev, + "Requested MTU of %d too small (min: %d)\n", + new_mtu, EFX_MIN_MTU); + return -EINVAL; + } netif_dbg(efx, drv, efx->net_dev, "changing MTU to %d\n", new_mtu); diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h index 0a2504b..99d8c82 100644 --- a/drivers/net/ethernet/sfc/net_driver.h +++ b/drivers/net/ethernet/sfc/net_driver.h @@ -76,6 +76,9 @@ /* Maximum possible MTU the driver supports */ #define EFX_MAX_MTU (9 * 1024) +/* Minimum MTU, from RFC791 (IP) */ +#define EFX_MIN_MTU 68 + /* Size of an RX scatter buffer. Small enough to pack 2 into a 4K page, * and should be a multiple of the cache line size. */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net-next] sfc: check MTU against minimum threshold 2016-09-06 16:50 ` [PATCH net-next] sfc: check MTU against minimum threshold Bert Kenward @ 2016-09-06 21:31 ` Jarod Wilson 2016-09-06 23:54 ` David Miller 1 sibling, 0 replies; 16+ messages in thread From: Jarod Wilson @ 2016-09-06 21:31 UTC (permalink / raw) To: Bert Kenward; +Cc: Dave Miller, netdev, linux-net-drivers On Tue, Sep 06, 2016 at 05:50:00PM +0100, Bert Kenward wrote: > Reported-by: Ma Yuying <yuma@redhat.com> > Suggested-by: Jarod Wilson <jarod@redhat.com> > Signed-off-by: Bert Kenward <bkenward@solarflare.com> Works for me, until we can get some dialog going about possible centralization (or not) of MTU checking, so we don't have this slightly crazy proliferation of *_MIN_MTU anymore. :) Reviewed-by: Jarod Wilson <jarod@redhat.com> -- Jarod Wilson jarod@redhat.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next] sfc: check MTU against minimum threshold 2016-09-06 16:50 ` [PATCH net-next] sfc: check MTU against minimum threshold Bert Kenward 2016-09-06 21:31 ` Jarod Wilson @ 2016-09-06 23:54 ` David Miller 1 sibling, 0 replies; 16+ messages in thread From: David Miller @ 2016-09-06 23:54 UTC (permalink / raw) To: bkenward; +Cc: netdev, linux-net-drivers, jarod From: Bert Kenward <bkenward@solarflare.com> Date: Tue, 6 Sep 2016 17:50:00 +0100 > Reported-by: Ma Yuying <yuma@redhat.com> > Suggested-by: Jarod Wilson <jarod@redhat.com> > Signed-off-by: Bert Kenward <bkenward@solarflare.com> Applied. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-02 17:07 Minimum MTU Mess Jarod Wilson 2016-09-06 16:50 ` [PATCH net-next] sfc: check MTU against minimum threshold Bert Kenward @ 2016-09-06 23:55 ` David Miller 2016-09-07 19:53 ` Jarod Wilson 1 sibling, 1 reply; 16+ messages in thread From: David Miller @ 2016-09-06 23:55 UTC (permalink / raw) To: jarod; +Cc: netdev From: Jarod Wilson <jarod@redhat.com> Date: Fri, 2 Sep 2016 13:07:42 -0400 > In any case, the number of "mtu < 68" and "#define FOO_MIN_MTU 68", or > variations thereof, under drivers/net/ is kind of crazy. Agreed, we can have a default and let the different cases provide overrides. Mostly what to do here is a function of the hardware though. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-06 23:55 ` Minimum MTU Mess David Miller @ 2016-09-07 19:53 ` Jarod Wilson 2016-09-07 20:31 ` Andrew Lunn ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Jarod Wilson @ 2016-09-07 19:53 UTC (permalink / raw) To: David Miller; +Cc: netdev On Tue, Sep 06, 2016 at 04:55:29PM -0700, David Miller wrote: > From: Jarod Wilson <jarod@redhat.com> > Date: Fri, 2 Sep 2016 13:07:42 -0400 > > > In any case, the number of "mtu < 68" and "#define FOO_MIN_MTU 68", or > > variations thereof, under drivers/net/ is kind of crazy. > > Agreed, we can have a default and let the different cases provide > overrides. > > Mostly what to do here is a function of the hardware though. So I've been tinkering with this some, and it looks like having both centralized min and max checking could be useful here. I'm hacking away at drivers now, but the basis of all this would potentially look about like the patch below, and each device would have to set dev->m{in,ax}_mtu one way or another. Drivers using alloc_etherdev and/or ether_setup would get the "default" values, and then they can be overridden. Probably need something to make sure dev->max_mtu isn't set to 0 though... Possibly on the right track here, or might there be a better way to approach this? diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h index 117d02e..864d6f2 100644 --- a/include/uapi/linux/if_ether.h +++ b/include/uapi/linux/if_ether.h @@ -35,6 +35,8 @@ #define ETH_FRAME_LEN 1514 /* Max. octets in frame sans FCS */ #define ETH_FCS_LEN 4 /* Octets in the FCS */ +#define ETH_MIN_MTU 68 /* Min IPv4 MTU per RFC791 */ + /* * These are the defined Ethernet Protocol ID's. */ diff --git a/net/core/dev.c b/net/core/dev.c index dd6ce59..d20fd5d 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6466,9 +6466,17 @@ int dev_set_mtu(struct net_device *dev, int new_mtu) if (new_mtu == dev->mtu) return 0; - /* 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; + } if (!netif_device_present(dev)) return -ENODEV; diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c index 66dff5e..2be0203 100644 --- a/net/ethernet/eth.c +++ b/net/ethernet/eth.c @@ -322,8 +322,6 @@ EXPORT_SYMBOL(eth_mac_addr); */ int eth_change_mtu(struct net_device *dev, int new_mtu) { - if (new_mtu < 68 || new_mtu > ETH_DATA_LEN) - return -EINVAL; dev->mtu = new_mtu; return 0; } @@ -357,6 +355,8 @@ void ether_setup(struct net_device *dev) dev->type = ARPHRD_ETHER; dev->hard_header_len = ETH_HLEN; dev->mtu = ETH_DATA_LEN; + dev->min_mtu = ETH_MIN_MTU; + dev->max_mtu = ETH_DATA_LEN; dev->addr_len = ETH_ALEN; dev->tx_queue_len = 1000; /* Ethernet wants good queues */ dev->flags = IFF_BROADCAST|IFF_MULTICAST; -- Jarod Wilson jarod@redhat.com ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-07 19:53 ` Jarod Wilson @ 2016-09-07 20:31 ` Andrew Lunn 2016-09-07 23:43 ` Jarod Wilson 2016-09-07 20:35 ` Stephen Hemminger 2016-09-12 2:59 ` YOSHIFUJI Hideaki 2 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2016-09-07 20:31 UTC (permalink / raw) To: Jarod Wilson; +Cc: David Miller, netdev 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). Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-07 20:31 ` Andrew Lunn @ 2016-09-07 23:43 ` Jarod Wilson 2016-09-08 1:24 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Jarod Wilson @ 2016-09-07 23:43 UTC (permalink / raw) To: Andrew Lunn; +Cc: David Miller, netdev 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@redhat.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-07 23:43 ` Jarod Wilson @ 2016-09-08 1:24 ` Andrew Lunn 2016-09-09 18:05 ` Jarod Wilson 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2016-09-08 1:24 UTC (permalink / raw) To: Jarod Wilson; +Cc: David Miller, netdev > 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). It might be something Coccinelle can help you with. Try describing the transformation you want to do, to their mailing list, and they might come up with a script for you. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-08 1:24 ` Andrew Lunn @ 2016-09-09 18:05 ` Jarod Wilson 2016-09-12 2:41 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Jarod Wilson @ 2016-09-09 18:05 UTC (permalink / raw) To: Andrew Lunn; +Cc: David Miller, netdev On Thu, Sep 08, 2016 at 03:24:13AM +0200, Andrew Lunn wrote: > > 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). > > It might be something Coccinelle can help you with. Try describing the > transformation you want to do, to their mailing list, and they might > come up with a script for you. >From looking everything over, I'd be very surprised if they could. The places where things need changing vary quite wildly by driver, but I've actually got a full set of compiling changes with a cumulative diffstat of: 153 files changed, 599 insertions(+), 1002 deletions(-) Actually breaking this up into easily digestable/mergeable chunks is going to be kind of entertaining... Suggestions welcomed on that. First up is obviously the core change, which touches just net/ethernet/eth.c, net/core/dev.c, include/linux/netdevice.h and include/uapi/linux/if_ether.h, and should let existing code continue to Just Work(tm), though devices using ether_setup() that had no MTU range checking (or one or the other missing) will wind up with new bounds. For the most part, after the initial patch, very few of the others would have any direct interaction with any others, so they could all be singletons, or small batches per-vendor, or whatever. Full diffstat for the aid of discussion on how to break it up: drivers/char/pcmcia/synclink_cs.c | 1 - drivers/firewire/net.c | 14 ++--- drivers/infiniband/hw/nes/nes.c | 1 - drivers/infiniband/hw/nes/nes.h | 4 +- drivers/infiniband/hw/nes/nes_nic.c | 7 +-- drivers/misc/sgi-xp/xpnet.c | 21 ++------ drivers/net/ethernet/agere/et131x.c | 7 +-- drivers/net/ethernet/altera/altera_tse.h | 1 - drivers/net/ethernet/altera/altera_tse_main.c | 12 ++--- drivers/net/ethernet/amd/amd8111e.c | 5 +- drivers/net/ethernet/atheros/alx/hw.h | 1 - drivers/net/ethernet/atheros/alx/main.c | 9 +--- drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 41 +++++++++----- drivers/net/ethernet/atheros/atl1e/atl1e_main.c | 11 ++-- drivers/net/ethernet/atheros/atlx/atl1.c | 15 +++--- drivers/net/ethernet/atheros/atlx/atl2.c | 14 +++-- drivers/net/ethernet/broadcom/b44.c | 5 +- drivers/net/ethernet/broadcom/bcm63xx_enet.c | 30 +++-------- drivers/net/ethernet/broadcom/bnx2.c | 8 ++- drivers/net/ethernet/broadcom/bnx2.h | 6 +-- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 +- drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 8 +-- drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c | 22 +++----- drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 4 ++ drivers/net/ethernet/broadcom/bnxt/bnxt.c | 7 +-- drivers/net/ethernet/broadcom/tg3.c | 7 +-- drivers/net/ethernet/brocade/bna/bnad.c | 7 +-- drivers/net/ethernet/cadence/macb.c | 17 +++--- drivers/net/ethernet/calxeda/xgmac.c | 18 ++----- drivers/net/ethernet/cavium/liquidio/lio_main.c | 15 ++---- .../net/ethernet/cavium/liquidio/octeon_network.h | 2 +- drivers/net/ethernet/cavium/octeon/octeon_mgmt.c | 5 +- drivers/net/ethernet/cavium/thunder/nicvf_main.c | 10 ++-- drivers/net/ethernet/chelsio/cxgb/cxgb2.c | 2 - drivers/net/ethernet/cisco/enic/enic_main.c | 7 +-- drivers/net/ethernet/cisco/enic/enic_res.h | 2 +- drivers/net/ethernet/dlink/dl2k.c | 22 ++------ drivers/net/ethernet/dlink/sundance.c | 6 ++- drivers/net/ethernet/freescale/gianfar.c | 9 ++-- drivers/net/ethernet/hisilicon/hns/hns_enet.c | 4 -- drivers/net/ethernet/ibm/ehea/ehea_main.c | 13 ++--- drivers/net/ethernet/ibm/emac/core.c | 7 +-- drivers/net/ethernet/intel/e100.c | 9 ---- drivers/net/ethernet/intel/e1000/e1000_main.c | 12 ++--- drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++-- drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 15 ++---- drivers/net/ethernet/intel/i40e/i40e_main.c | 10 ++-- drivers/net/ethernet/intel/i40evf/i40evf_main.c | 8 +-- drivers/net/ethernet/intel/igb/e1000_defines.h | 3 +- drivers/net/ethernet/intel/igb/igb_main.c | 16 ++---- drivers/net/ethernet/intel/igbvf/defines.h | 3 +- drivers/net/ethernet/intel/igbvf/netdev.c | 14 ++--- drivers/net/ethernet/intel/ixgb/ixgb_main.c | 16 ++---- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 ++-- drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 33 ++++++------ drivers/net/ethernet/marvell/mvneta.c | 36 ++++--------- drivers/net/ethernet/marvell/mvpp2.c | 36 ++++--------- drivers/net/ethernet/marvell/pxa168_eth.c | 7 +-- drivers/net/ethernet/marvell/skge.c | 7 +-- drivers/net/ethernet/marvell/sky2.c | 18 +++---- drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 8 +-- drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 23 +++----- drivers/net/ethernet/micrel/ksz884x.c | 33 ++++++------ drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 20 +++---- drivers/net/ethernet/natsemi/natsemi.c | 7 +-- drivers/net/ethernet/neterion/s2io.c | 9 ++-- drivers/net/ethernet/neterion/vxge/vxge-config.h | 2 +- drivers/net/ethernet/neterion/vxge/vxge-main.c | 9 ++-- .../net/ethernet/netronome/nfp/nfp_net_common.c | 10 ++-- drivers/net/ethernet/nvidia/forcedeth.c | 9 ++-- .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 11 ++-- drivers/net/ethernet/pasemi/pasemi_mac.c | 12 +++-- drivers/net/ethernet/qlogic/qede/qede.h | 5 +- drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 8 --- drivers/net/ethernet/qlogic/qede/qede_main.c | 4 ++ drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c | 6 --- drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 4 ++ drivers/net/ethernet/qualcomm/qca_framing.h | 6 +-- drivers/net/ethernet/qualcomm/qca_spi.c | 16 ++---- drivers/net/ethernet/realtek/8139cp.c | 8 +-- drivers/net/ethernet/realtek/8139too.c | 13 ++--- drivers/net/ethernet/realtek/r8169.c | 8 +-- drivers/net/ethernet/rocker/rocker_main.c | 12 ++--- drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c | 17 ++---- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++----- drivers/net/ethernet/sun/cassini.c | 7 +-- drivers/net/ethernet/sun/ldmvsw.c | 5 +- drivers/net/ethernet/sun/niu.c | 7 +-- drivers/net/ethernet/sun/sungem.c | 11 ++-- drivers/net/ethernet/sun/sunvnet.c | 4 ++ drivers/net/ethernet/sun/sunvnet_common.c | 10 ---- drivers/net/ethernet/sun/sunvnet_common.h | 3 +- drivers/net/ethernet/tehuti/tehuti.c | 14 ++--- drivers/net/ethernet/tehuti/tehuti.h | 3 ++ drivers/net/ethernet/ti/netcp_core.c | 20 ++----- drivers/net/ethernet/tile/tilegx.c | 21 +++----- drivers/net/ethernet/tile/tilepro.c | 27 ++-------- drivers/net/ethernet/toshiba/ps3_gelic_net.c | 23 ++------ drivers/net/ethernet/toshiba/spider_net.c | 24 ++------- drivers/net/ethernet/via/via-velocity.c | 11 ++-- drivers/net/geneve.c | 48 +++++++---------- drivers/net/hippi/rrunner.c | 1 - drivers/net/hyperv/hyperv_net.h | 4 +- drivers/net/hyperv/netvsc_drv.c | 14 ++--- drivers/net/macvlan.c | 6 ++- drivers/net/rionet.c | 15 ++---- drivers/net/slip/slip.c | 11 ++-- drivers/net/tun.c | 20 +++---- drivers/net/usb/lan78xx.c | 8 ++- drivers/net/usb/r8152.c | 18 +++++-- drivers/net/usb/usbnet.c | 2 - drivers/net/virtio_net.c | 23 ++++---- drivers/net/vmxnet3/vmxnet3_drv.c | 7 +-- drivers/net/vxlan.c | 62 +++++++++++----------- drivers/net/wan/c101.c | 1 - drivers/net/wan/cosa.c | 1 - drivers/net/wan/dscc4.c | 1 - drivers/net/wan/farsync.c | 1 - drivers/net/wan/fsl_ucc_hdlc.c | 1 - drivers/net/wan/hdlc.c | 11 +--- drivers/net/wan/hdlc_fr.c | 3 +- drivers/net/wan/hostess_sv11.c | 1 - drivers/net/wan/ixp4xx_hss.c | 1 - drivers/net/wan/lmc/lmc_main.c | 1 - drivers/net/wan/n2.c | 1 - drivers/net/wan/pc300too.c | 1 - drivers/net/wan/pci200syn.c | 1 - drivers/net/wan/sealevel.c | 1 - drivers/net/wan/wanxl.c | 1 - drivers/net/wireless/ath/wil6210/netdev.c | 17 +----- drivers/net/wireless/atmel/atmel.c | 13 ++--- drivers/net/wireless/cisco/airo.c | 14 ++--- drivers/net/wireless/intel/ipw2x00/ipw2100.c | 3 +- drivers/net/wireless/intel/ipw2x00/ipw2200.c | 8 ++- drivers/net/wireless/intel/ipw2x00/libipw.h | 1 - drivers/net/wireless/intel/ipw2x00/libipw_module.c | 9 ---- drivers/staging/octeon/ethernet.c | 16 ++---- drivers/staging/wlan-ng/p80211netdev.c | 18 ++----- drivers/tty/synclink.c | 1 - drivers/tty/synclink_gt.c | 1 - drivers/tty/synclinkmp.c | 1 - include/linux/hdlc.h | 2 - include/linux/hippidevice.h | 1 - include/linux/netdevice.h | 4 ++ include/uapi/linux/if_ether.h | 2 + net/802/hippi.c | 14 +---- net/atm/lec.c | 11 +--- net/batman-adv/soft-interface.c | 13 +---- net/bridge/br_device.c | 3 +- net/core/dev.c | 12 ++++- net/ethernet/eth.c | 4 +- net/openvswitch/vport-internal_dev.c | 10 ---- net/sched/sch_teql.c | 5 +- 153 files changed, 599 insertions(+), 1002 deletions(-) -- Jarod Wilson jarod@redhat.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-09 18:05 ` Jarod Wilson @ 2016-09-12 2:41 ` Andrew Lunn 2016-09-12 14:27 ` Jarod Wilson 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2016-09-12 2:41 UTC (permalink / raw) To: Jarod Wilson; +Cc: David Miller, netdev > Actually breaking this up into easily digestable/mergeable chunks is going > to be kind of entertaining... Suggestions welcomed on that. First up is > obviously the core change, which touches just net/ethernet/eth.c, > net/core/dev.c, include/linux/netdevice.h and > include/uapi/linux/if_ether.h, and should let existing code continue to > Just Work(tm), though devices using ether_setup() that had no MTU range > checking (or one or the other missing) will wind up with new bounds. Hi Jarod Did you find any drivers which support jumbo packets, but don't have checks? These drivers, if there are any, need handling first, before this core change is made. Otherwise you introduce regressions. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-12 2:41 ` Andrew Lunn @ 2016-09-12 14:27 ` Jarod Wilson 0 siblings, 0 replies; 16+ messages in thread From: Jarod Wilson @ 2016-09-12 14:27 UTC (permalink / raw) To: Andrew Lunn; +Cc: David Miller, netdev On Mon, Sep 12, 2016 at 04:41:40AM +0200, Andrew Lunn wrote: > > Actually breaking this up into easily digestable/mergeable chunks is going > > to be kind of entertaining... Suggestions welcomed on that. First up is > > obviously the core change, which touches just net/ethernet/eth.c, > > net/core/dev.c, include/linux/netdevice.h and > > include/uapi/linux/if_ether.h, and should let existing code continue to > > Just Work(tm), though devices using ether_setup() that had no MTU range > > checking (or one or the other missing) will wind up with new bounds. > > Hi Jarod > > Did you find any drivers which support jumbo packets, but don't have > checks? These drivers, if there are any, need handling first, before > this core change is made. Otherwise you introduce regressions. Surprisingly, very few. There was dvb_net using eth_change_mtu and it's max of 1500 while setting it's initial mtu to 4096, and I swear there was at least one or two drivers that had no upper bounds checking at all that I set max_mtu to IP_MAX_MTU (65535), but it's possible I missed something. At the moment, things are roughly chopped up into: 1) core change 2) deprecate eth_change_mtu and remove in-kernel users 3) set m{in,ax}_mtu in ethernet drivers 4) set m{in,ax}_mtu in wireless drivers 5) set m{in,ax}_mtu in wan drivers 6) set m{in,ax}_mtu in usb ethernet drivers 7) set m{in,ax}_mtu in net infra 8) set m{in,ax}_mtu in virt drivers 9) set m{in,ax}_mtu in misc drivers The ethernet drivers one is by far the largest, and was thinking I'd start breaking that up next. -- Jarod Wilson jarod@redhat.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-07 19:53 ` Jarod Wilson 2016-09-07 20:31 ` Andrew Lunn @ 2016-09-07 20:35 ` Stephen Hemminger 2016-09-07 23:44 ` Jarod Wilson 2016-09-12 2:59 ` YOSHIFUJI Hideaki 2 siblings, 1 reply; 16+ messages in thread From: Stephen Hemminger @ 2016-09-07 20:35 UTC (permalink / raw) To: Jarod Wilson; +Cc: David Miller, netdev On Wed, 7 Sep 2016 15:53:56 -0400 Jarod Wilson <jarod@redhat.com> wrote: > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -6466,9 +6466,17 @@ int dev_set_mtu(struct net_device *dev, int new_mtu) > if (new_mtu == dev->mtu) > return 0; > > - /* 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; > + } > Maybe don't log something that can be triggered from a user program. Or at least rate limit it. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-07 20:35 ` Stephen Hemminger @ 2016-09-07 23:44 ` Jarod Wilson 0 siblings, 0 replies; 16+ messages in thread From: Jarod Wilson @ 2016-09-07 23:44 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David Miller, netdev On Wed, Sep 07, 2016 at 01:35:35PM -0700, Stephen Hemminger wrote: > On Wed, 7 Sep 2016 15:53:56 -0400 > Jarod Wilson <jarod@redhat.com> wrote: > > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -6466,9 +6466,17 @@ int dev_set_mtu(struct net_device *dev, int new_mtu) > > if (new_mtu == dev->mtu) > > return 0; > > > > - /* 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; > > + } > > > > Maybe don't log something that can be triggered from a user program. > Or at least rate limit it. Yeah, I was a little bit on the fence on whether to log anything, make it netdev_err, netdev_dbg, or what. Quite a few drivers have a netdev_err for failed MTU changes, while others also have netdev_info spew for successful MTU changes. Maybe a rate-limited netdev_err is the way to go here. -- Jarod Wilson jarod@redhat.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-07 19:53 ` Jarod Wilson 2016-09-07 20:31 ` Andrew Lunn 2016-09-07 20:35 ` Stephen Hemminger @ 2016-09-12 2:59 ` YOSHIFUJI Hideaki 2016-09-12 14:31 ` Jarod Wilson 2 siblings, 1 reply; 16+ messages in thread From: YOSHIFUJI Hideaki @ 2016-09-12 2:59 UTC (permalink / raw) To: Jarod Wilson, David Miller; +Cc: hideaki.yoshifuji, netdev Jarod Wilson wrote: > On Tue, Sep 06, 2016 at 04:55:29PM -0700, David Miller wrote: >> From: Jarod Wilson <jarod@redhat.com> >> Date: Fri, 2 Sep 2016 13:07:42 -0400 >> >>> In any case, the number of "mtu < 68" and "#define FOO_MIN_MTU 68", or >>> variations thereof, under drivers/net/ is kind of crazy. >> >> Agreed, we can have a default and let the different cases provide >> overrides. >> >> Mostly what to do here is a function of the hardware though. > > So I've been tinkering with this some, and it looks like having both > centralized min and max checking could be useful here. I'm hacking away at > drivers now, but the basis of all this would potentially look about like > the patch below, and each device would have to set dev->m{in,ax}_mtu one > way or another. Drivers using alloc_etherdev and/or ether_setup would get > the "default" values, and then they can be overridden. Probably need > something to make sure dev->max_mtu isn't set to 0 though... > > Possibly on the right track here, or might there be a better way to > approach this? > > diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h > index 117d02e..864d6f2 100644 > --- a/include/uapi/linux/if_ether.h > +++ b/include/uapi/linux/if_ether.h > @@ -35,6 +35,8 @@ > #define ETH_FRAME_LEN 1514 /* Max. octets in frame sans FCS */ > #define ETH_FCS_LEN 4 /* Octets in the FCS */ > > +#define ETH_MIN_MTU 68 /* Min IPv4 MTU per RFC791 */ > + > /* > * These are the defined Ethernet Protocol ID's. > */ Why don't we disable IPv4 if the MTU is lower than this value as we do for IPv6? -- Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com> Technical Division, MIRACLE LINUX CORPORATION ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Minimum MTU Mess 2016-09-12 2:59 ` YOSHIFUJI Hideaki @ 2016-09-12 14:31 ` Jarod Wilson 0 siblings, 0 replies; 16+ messages in thread From: Jarod Wilson @ 2016-09-12 14:31 UTC (permalink / raw) To: YOSHIFUJI Hideaki; +Cc: David Miller, netdev On Mon, Sep 12, 2016 at 11:59:41AM +0900, YOSHIFUJI Hideaki wrote: > > > Jarod Wilson wrote: > > On Tue, Sep 06, 2016 at 04:55:29PM -0700, David Miller wrote: > >> From: Jarod Wilson <jarod@redhat.com> > >> Date: Fri, 2 Sep 2016 13:07:42 -0400 > >> > >>> In any case, the number of "mtu < 68" and "#define FOO_MIN_MTU 68", or > >>> variations thereof, under drivers/net/ is kind of crazy. > >> > >> Agreed, we can have a default and let the different cases provide > >> overrides. > >> > >> Mostly what to do here is a function of the hardware though. > > > > So I've been tinkering with this some, and it looks like having both > > centralized min and max checking could be useful here. I'm hacking away at > > drivers now, but the basis of all this would potentially look about like > > the patch below, and each device would have to set dev->m{in,ax}_mtu one > > way or another. Drivers using alloc_etherdev and/or ether_setup would get > > the "default" values, and then they can be overridden. Probably need > > something to make sure dev->max_mtu isn't set to 0 though... > > > > Possibly on the right track here, or might there be a better way to > > approach this? > > > > diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h > > index 117d02e..864d6f2 100644 > > --- a/include/uapi/linux/if_ether.h > > +++ b/include/uapi/linux/if_ether.h > > @@ -35,6 +35,8 @@ > > #define ETH_FRAME_LEN 1514 /* Max. octets in frame sans FCS */ > > #define ETH_FCS_LEN 4 /* Octets in the FCS */ > > > > +#define ETH_MIN_MTU 68 /* Min IPv4 MTU per RFC791 */ > > + > > /* > > * These are the defined Ethernet Protocol ID's. > > */ > > Why don't we disable IPv4 if the MTU is lower than this value > as we do for IPv6? What will you be left with that is actually usable? Quite a few NIC drivers already enforce this as a minimum MTU, and for drivers that really want to allow less, they just set min_mtu to whatever they like. I'm actually aiming to be 100% functionally identical wrt all existing minimum mtu checks already in existence, just trying to improve how they're done. -- Jarod Wilson jarod@redhat.com ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-09-12 14:31 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-02 17:07 Minimum MTU Mess Jarod Wilson 2016-09-06 16:50 ` [PATCH net-next] sfc: check MTU against minimum threshold Bert Kenward 2016-09-06 21:31 ` Jarod Wilson 2016-09-06 23:54 ` David Miller 2016-09-06 23:55 ` Minimum MTU Mess David Miller 2016-09-07 19:53 ` Jarod Wilson 2016-09-07 20:31 ` Andrew Lunn 2016-09-07 23:43 ` Jarod Wilson 2016-09-08 1:24 ` Andrew Lunn 2016-09-09 18:05 ` Jarod Wilson 2016-09-12 2:41 ` Andrew Lunn 2016-09-12 14:27 ` Jarod Wilson 2016-09-07 20:35 ` Stephen Hemminger 2016-09-07 23:44 ` Jarod Wilson 2016-09-12 2:59 ` YOSHIFUJI Hideaki 2016-09-12 14:31 ` Jarod Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.