Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S945687AbcJSPq6 (ORCPT ); Wed, 19 Oct 2016 11:46:58 -0400 Received: from mail-lf0-f41.google.com ([209.85.215.41]:33339 "EHLO mail-lf0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S942863AbcJSPqz (ORCPT ); Wed, 19 Oct 2016 11:46:55 -0400 Date: Wed, 19 Oct 2016 11:46:38 -0400 From: Jarod Wilson To: Sabrina Dubroca Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, Nicolas Dichtel , Hannes Frederic Sowa , Tom Herbert , Daniel Borkmann , Alexander Duyck , Paolo Abeni , Jiri Benc , WANG Cong , Roopa Prabhu , Pravin B Shelar , Patrick McHardy , Stephen Hemminger , Pravin Shelar Subject: Re: [PATCH net-next 4/6] net: use core MTU range checking in core net infra Message-ID: <20161019154638.GH18569@redhat.com> References: <20161019023333.15760-1-jarod@redhat.com> <20161019023333.15760-5-jarod@redhat.com> <20161019135529.GA11224@bistromath.localdomain> <20161019144006.GF18569@redhat.com> <20161019152759.GB11224@bistromath.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161019152759.GB11224@bistromath.localdomain> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2824 Lines: 72 On Wed, Oct 19, 2016 at 05:28:00PM +0200, Sabrina Dubroca wrote: > 2016-10-19, 10:40:06 -0400, Jarod Wilson wrote: > > On Wed, Oct 19, 2016 at 03:55:29PM +0200, Sabrina Dubroca wrote: > > > 2016-10-18, 22:33:31 -0400, Jarod Wilson wrote: ... > > 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. > > I'm not sure how that would work now, if some of the patches that > already went in for ethernet drivers assume that ether_setup will > configure a basic {min,max}_mtu pair (at least e100 makes that > assumption, but that might be the only one). Argh. Yeah. Hrm. Would have to do the revert *and* have e100 and possibly others set their own min/max pair. So I guess it's a race to fix all the fallout... Crap. > > > [...] > > > > 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()? > > Sounds good to me. I think I have something here locally that looks sane. Working on a few other similar cases now. -- Jarod Wilson jarod@redhat.com