On Wed, Oct 19, 2016 at 03:55:29PM +0200, Sabrina Dubroca wrote: > 2016-10-18, 22:33:31 -0400, Jarod Wilson wrote: > > geneve: > > - Merge __geneve_change_mtu back into geneve_change_mtu, set max_mtu > > - This one isn't quite as straight-forward as others, could use some > > closer inspection and testing > > > > macvlan: > > - set min/max_mtu > > > > tun: > > - set min/max_mtu, remove tun_net_change_mtu > > > > vxlan: > > - Merge __vxlan_change_mtu back into vxlan_change_mtu, set min/max_mtu > > - This one is also not as straight-forward and could use closer inspection > > and testing from vxlan folks > > > > bridge: > > - set max_mtu via br_min_mtu() > > > > openvswitch: > > - set min/max_mtu, remove internal_dev_change_mtu > > - note: max_mtu wasn't checked previously, it's been set to 65535, which > > is the largest possible size supported > > > > sch_teql: > > - set min/max_mtu (note: max_mtu previously unchecked, used max of 65535) > > Nothing for other virtual netdevices? (dummy, veth, bond, etc) Their > MTU is limited to 1500 now. Also missing macsec and ip_gre, probably > others that are using ether_setup.
Yeah, I've clearly missed more than I thought. Doing another sweep now. I'm thinking more and more that we ought to back out the patch that sets min/max in ether_setup, save it for last, after we're sure everyone that calls it has been prepared. > [...] > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > > index 89a687f..81fc79a 100644 > > --- a/net/bridge/br_device.c > > +++ b/net/bridge/br_device.c > > @@ -184,17 +184,15 @@ static struct rtnl_link_stats64 > > *br_get_stats64(struct net_device *dev, > > > > static int br_change_mtu(struct net_device *dev, int new_mtu) > > { > > +#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) > > struct net_bridge *br = netdev_priv(dev); > > - if (new_mtu < 68 || new_mtu > br_min_mtu(br)) > > - return -EINVAL; > > - > > - dev->mtu = new_mtu; > > > > -#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) > > /* remember the MTU in the rtable for PMTU */ > > dst_metric_set(&br->fake_rtable.dst, RTAX_MTU, new_mtu); > > #endif > > > > + dev->mtu = new_mtu; > > + > > return 0; > > } > > > > @@ -390,6 +388,7 @@ void br_dev_setup(struct net_device *dev) > > dev->hw_features = COMMON_FEATURES | NETIF_F_HW_VLAN_CTAG_TX | > > NETIF_F_HW_VLAN_STAG_TX; > > dev->vlan_features = COMMON_FEATURES; > > + dev->max_mtu = br_min_mtu(br); > > br_min_mtu uses br->port_list, which is only initialized a few lines > later (right after the spin_lock_init() at the end of the context of > this diff). Ah, okay, I'd just grouped it with the other dev->foo settings. > Besides, I don't think this works: br_min_mtu(br) changes when you add > and remove ports, or when you change the MTU of an enslaved > device. But this makes the max MTU for the bridge fixed (to 1500). Okay, how about this: set no max_mtu (or set it to IP_MAX_MTU/65535), and then retain a check against the possibly ever-changing br_min_mtu(br) in br_change_mtu()? > > diff --git a/net/openvswitch/vport-internal_dev.c > > b/net/openvswitch/vport-internal_dev.c > > index e7da290..d5d6cae 100644 > > --- a/net/openvswitch/vport-internal_dev.c > > +++ b/net/openvswitch/vport-internal_dev.c > > @@ -89,15 +89,6 @@ static const struct ethtool_ops internal_dev_ethtool_ops > > = { > > .get_link = ethtool_op_get_link, > > }; > > > > -static int internal_dev_change_mtu(struct net_device *netdev, int new_mtu) > > -{ > > - if (new_mtu < 68) > > - return -EINVAL; > > - > > - netdev->mtu = new_mtu; > > - return 0; > > -} > > - > > static void internal_dev_destructor(struct net_device *dev) > > { > > struct vport *vport = ovs_internal_dev_get_vport(dev); > > @@ -148,7 +139,6 @@ static const struct net_device_ops > > internal_dev_netdev_ops = { > > .ndo_stop = internal_dev_stop, > > .ndo_start_xmit = internal_dev_xmit, > > .ndo_set_mac_address = eth_mac_addr, > > - .ndo_change_mtu = internal_dev_change_mtu, > > .ndo_get_stats64 = internal_get_stats, > > .ndo_set_rx_headroom = internal_set_rx_headroom, > > }; > > vport-internal uses ether_setup, so the MTU is currently limited to > 1500, no? Yeah. Sweep ongoing... -- Jarod Wilson ja...@redhat.com