| Message ID | 1389358097-5396-1-git-send-email-vfalico@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 01/10/2014 04:48 AM, Veaceslav Falico wrote: > Currently, after changing the MTU for a device, dev_set_mtu() calls > NETDEV_CHANGEMTU notification, however doesn't verify it's return code - > which can be NOTIFY_BAD - i.e. some of the net notifier blocks refused this > change, and continues nevertheless. > > To fix this, verify the return code, and if it's an error - then revert the > MTU to the original one, and pass the error code. > > CC: Jiri Pirko <jiri@resnulli.us> > CC: "David S. Miller" <davem@davemloft.net> > CC: Eric Dumazet <edumazet@google.com> > CC: Alexander Duyck <alexander.h.duyck@intel.com> > CC: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > --- > net/core/dev.c | 29 ++++++++++++++++++++--------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index ce01847..1c570ff 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -5287,6 +5287,17 @@ int dev_change_flags(struct net_device *dev, unsigned int flags) > } > EXPORT_SYMBOL(dev_change_flags); > > +static int __dev_set_mtu(struct net_device *dev, int new_mtu) > +{ > + const struct net_device_ops *ops = dev->netdev_ops; > + > + if (ops->ndo_change_mtu) > + return ops->ndo_change_mtu(dev, new_mtu); > + > + dev->mtu = new_mtu; > + return 0; > +} > + > /** > * dev_set_mtu - Change maximum transfer unit > * @dev: device > @@ -5296,8 +5307,7 @@ EXPORT_SYMBOL(dev_change_flags); > */ > int dev_set_mtu(struct net_device *dev, int new_mtu) > { > - const struct net_device_ops *ops = dev->netdev_ops; > - int err; > + int err, orig_mtu; > > if (new_mtu == dev->mtu) > return 0; > @@ -5309,14 +5319,15 @@ int dev_set_mtu(struct net_device *dev, int new_mtu) > if (!netif_device_present(dev)) > return -ENODEV; > > - err = 0; > - if (ops->ndo_change_mtu) > - err = ops->ndo_change_mtu(dev, new_mtu); > - else > - dev->mtu = new_mtu; > + orig_mtu = dev->mtu; > + err = __dev_set_mtu(dev, new_mtu); > > - if (!err) > - call_netdevice_notifiers(NETDEV_CHANGEMTU, dev); > + if (!err) { > + err = call_netdevice_notifiers(NETDEV_CHANGEMTU, dev); > + err = notifier_to_errno(err); > + if (err) > + __dev_set_mtu(dev, orig_mtu); > + } > return err; > } > EXPORT_SYMBOL(dev_set_mtu); So what about the netdevices that succeeded in changing the MTU based on the notifiers? It seems like you still have an inconsistent state after the failure unless you issue a second call with a notification that you reverted to the old MTU. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 10, 2014 at 07:36:45AM -0800, Alexander Duyck wrote: >On 01/10/2014 04:48 AM, Veaceslav Falico wrote: ...snip... >> - err = 0; >> - if (ops->ndo_change_mtu) >> - err = ops->ndo_change_mtu(dev, new_mtu); >> - else >> - dev->mtu = new_mtu; >> + orig_mtu = dev->mtu; >> + err = __dev_set_mtu(dev, new_mtu); >> >> - if (!err) >> - call_netdevice_notifiers(NETDEV_CHANGEMTU, dev); >> + if (!err) { >> + err = call_netdevice_notifiers(NETDEV_CHANGEMTU, dev); >> + err = notifier_to_errno(err); >> + if (err) >> + __dev_set_mtu(dev, orig_mtu); >> + } >> return err; >> } >> EXPORT_SYMBOL(dev_set_mtu); > >So what about the netdevices that succeeded in changing the MTU based on >the notifiers? It seems like you still have an inconsistent state >after the failure unless you issue a second call with a notification >that you reverted to the old MTU. Good point, thank you. I'll add a second call_netdevice_notifiers() after setting the original MTU and resend. Thank you! > >Thanks, > >Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Veaceslav Falico <vfalico@redhat.com> Date: Fri, 10 Jan 2014 13:48:17 +0100 > Currently, after changing the MTU for a device, dev_set_mtu() calls > NETDEV_CHANGEMTU notification, however doesn't verify it's return code - > which can be NOTIFY_BAD - i.e. some of the net notifier blocks refused this > change, and continues nevertheless. > > To fix this, verify the return code, and if it's an error - then revert the > MTU to the original one, and pass the error code. > > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> This is really a precariously designed code path. If one of the notifiers says NOTIFY_BAD, well we've already changed dev->mtu, therefore what if a packet got sent during this time? Whoever the NOTIFY_BAD signaller is, obviously cannot handle an MTU setting which we've already set in the netdev. So allowing it's a terribly idea to allow visibility of the new MTU value until we can be sure everyone can handle it. The problem is that we really need a transaction of some sort to fix this properly. First, we'd need to ask all the notifiers if they can handle the new MTU, then we somehow atomically set netdev->mtu and have the notifiers commit their own state changes. Then we'll have the stick issue of what to do if a notifier is unregistered between the check and the commit. :-) Your patch is an improvement so I will apply it, this stuff really is full of holes still. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/dev.c b/net/core/dev.c index ce01847..1c570ff 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5287,6 +5287,17 @@ int dev_change_flags(struct net_device *dev, unsigned int flags) } EXPORT_SYMBOL(dev_change_flags); +static int __dev_set_mtu(struct net_device *dev, int new_mtu) +{ + const struct net_device_ops *ops = dev->netdev_ops; + + if (ops->ndo_change_mtu) + return ops->ndo_change_mtu(dev, new_mtu); + + dev->mtu = new_mtu; + return 0; +} + /** * dev_set_mtu - Change maximum transfer unit * @dev: device @@ -5296,8 +5307,7 @@ EXPORT_SYMBOL(dev_change_flags); */ int dev_set_mtu(struct net_device *dev, int new_mtu) { - const struct net_device_ops *ops = dev->netdev_ops; - int err; + int err, orig_mtu; if (new_mtu == dev->mtu) return 0; @@ -5309,14 +5319,15 @@ int dev_set_mtu(struct net_device *dev, int new_mtu) if (!netif_device_present(dev)) return -ENODEV; - err = 0; - if (ops->ndo_change_mtu) - err = ops->ndo_change_mtu(dev, new_mtu); - else - dev->mtu = new_mtu; + orig_mtu = dev->mtu; + err = __dev_set_mtu(dev, new_mtu); - if (!err) - call_netdevice_notifiers(NETDEV_CHANGEMTU, dev); + if (!err) { + err = call_netdevice_notifiers(NETDEV_CHANGEMTU, dev); + err = notifier_to_errno(err); + if (err) + __dev_set_mtu(dev, orig_mtu); + } return err; } EXPORT_SYMBOL(dev_set_mtu);
Currently, after changing the MTU for a device, dev_set_mtu() calls NETDEV_CHANGEMTU notification, however doesn't verify it's return code - which can be NOTIFY_BAD - i.e. some of the net notifier blocks refused this change, and continues nevertheless. To fix this, verify the return code, and if it's an error - then revert the MTU to the original one, and pass the error code. CC: Jiri Pirko <jiri@resnulli.us> CC: "David S. Miller" <davem@davemloft.net> CC: Eric Dumazet <edumazet@google.com> CC: Alexander Duyck <alexander.h.duyck@intel.com> CC: Nicolas Dichtel <nicolas.dichtel@6wind.com> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- net/core/dev.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)