Patchwork [net-next] net: make dev_set_mtu() honor notification return code

login
register
mail settings
Submitter Veaceslav Falico
Date Jan. 10, 2014, 12:48 p.m.
Message ID <1389358097-5396-1-git-send-email-vfalico@redhat.com>
Download mbox | patch
Permalink /patch/309261/
State Superseded
Headers show

Comments

Veaceslav Falico - Jan. 10, 2014, 12:48 p.m.
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(-)
Alexander Duyck - Jan. 10, 2014, 3:36 p.m.
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
Veaceslav Falico - Jan. 10, 2014, 3:47 p.m.
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
David Miller - Jan. 13, 2014, 11:18 p.m.
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

Patch

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);