From: Veaceslav Falico <vfalico@redhat•com>
To: David Miller <davem@davemloft•net>
Cc: netdev@vger•kernel.org, jiri@resnulli•us, edumazet@google•com,
alexander.h.duyck@intel•com, nicolas.dichtel@6wind•com
Subject: Re: [PATCH net-next] net: make dev_set_mtu() honor notification return code
Date: Tue, 14 Jan 2014 13:13:54 +0100 [thread overview]
Message-ID: <20140114121354.GI4132@redhat.com> (raw)
In-Reply-To: <20140113.151855.1046745397621207165.davem@davemloft.net>
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.
>
next prev parent reply other threads:[~2014-01-14 12:17 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-10 12:48 [PATCH net-next] net: make dev_set_mtu() honor notification return code Veaceslav Falico
2014-01-10 15:36 ` Alexander Duyck
2014-01-10 15:47 ` Veaceslav Falico
2014-01-13 23:18 ` David Miller
2014-01-14 12:13 ` Veaceslav Falico [this message]
2014-01-15 21:48 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140114121354.GI4132@redhat.com \
--to=vfalico@redhat$(echo .)com \
--cc=alexander.h.duyck@intel$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nicolas.dichtel@6wind$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox