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

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

[Veaceslav Falico]

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

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

[Alexander Duyck]

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

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

[Veaceslav Falico]

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

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

[David Miller]

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.

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

[Veaceslav Falico]

On Mon, Jan 13, 2014 at 03:18:55PM -0800, David Miller wrote:
>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.

Yeah, but I can't think of a method to atomically set it for both netdev
and its notifiers... As in, for example, bridge (but not only) takes the
lowest MTU of its ports.

>
>Then we'll have the stick issue of what to do if a notifier is
>unregistered between the check and the commit. :-)

Maybe you've meant 'registered between ...' ? :-) Anyway, I guess
dev_set_mtu() should always be called under RTNL, and this way we won't
have these problems. Though I might be wrong, everyone seems playing with
MTU the way they want.

>
>Your patch is an improvement so I will apply it, this stuff really
>is full of holes still.

One (least intrusive) approach would be to add NETDEV_PRECHANGEMTU, which
would be used to verify if the notifiers all agree with changing, and leave
the NETDEV_CHANGEMTU fail only when something really bad happened. That's
your idea, basically.

As, currently, only team can signal NOTIFY_BAD on mtu change, it's really
easy to implement. What do you think?

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 736050d..4e50e04 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2850,7 +2850,7 @@ static int team_device_event(struct notifier_block *unused,
  	case NETDEV_FEAT_CHANGE:
  		team_compute_features(port->team);
  		break;
-	case NETDEV_CHANGEMTU:
+	case NETDEV_PRECHANGEMTU:
  		/* Forbid to change mtu of underlaying device */
  		return NOTIFY_BAD;
  	case NETDEV_PRE_TYPE_CHANGE:
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a2a70cc..7e023c4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1731,6 +1731,7 @@ struct pcpu_sw_netstats {
  #define NETDEV_JOIN		0x0014
  #define NETDEV_CHANGEUPPER	0x0015
  #define NETDEV_RESEND_IGMP	0x0016
+#define NETDEV_PRECHANGEMTU	0x0017
  
  int register_netdevice_notifier(struct notifier_block *nb);
  int unregister_netdevice_notifier(struct notifier_block *nb);
diff --git a/net/core/dev.c b/net/core/dev.c
index 87312dc..096d4dd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5332,6 +5332,10 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
  	if (!netif_device_present(dev))
  		return -ENODEV;
  
+	err = call_netdevice_notifiers(NETDEV_PRECHANGEMTU, dev);
+	if (!err)
+		return notifier_to_errno(err);
+
  	orig_mtu = dev->mtu;
  	err = __dev_set_mtu(dev, new_mtu);
  

>
>Thanks.
>

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

[David Miller]

From: Veaceslav Falico <vfalico@redhat.com>
Date: Tue, 14 Jan 2014 13:13:54 +0100

> As, currently, only team can signal NOTIFY_BAD on mtu change, it's
> really easy to implement. What do you think?

Looks great, and I agree that RTNL should solve all the other issues.