| Message ID | 1314803731-1222-1-git-send-email-jpirko@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 31 Aug 2011 17:15:31 +0200 Jiri Pirko <jpirko@redhat.com> wrote: > In some situations, like when the device is used as slave device in > bond/br/etc it is not nice if someone closes the device. This allows > it's masters to forbid this closure. > > Signed-off-by: Jiri Pirko <jpirko@redhat.com> I don't think this is necessary, for bridging case. bridging handles it. And bonding should as well. It is a good way to test STP etc. Is this a case of "you really shouldn't do this", or "don't do this it will crash"? In general Linux allows the former and uses references to prevent the later. -- 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 Wed, 2011-08-31 at 17:15 +0200, Jiri Pirko wrote: > In some situations, like when the device is used as slave device in > bond/br/etc it is not nice if someone closes the device. This allows > it's masters to forbid this closure. No it doesn't. [...] > @@ -1269,9 +1282,12 @@ static int dev_close_many(struct list_head *head) > struct net_device *dev, *tmp; > LIST_HEAD(tmp_list); > > - list_for_each_entry_safe(dev, tmp, head, unreg_list) > + list_for_each_entry_safe(dev, tmp, head, unreg_list) { > if (!(dev->flags & IFF_UP)) > list_move(&dev->unreg_list, &tmp_list); > + else > + __dev_pre_close(dev); > + } > > __dev_close_many(head); The return value is ignored here. And this is called from dev_close(), where you are adding the notification as well. So the notifier will usually be called twice. [...] > @@ -1397,6 +1418,7 @@ rollback: > break; > > if (dev->flags & IFF_UP) { > + nb->notifier_call(nb, NETDEV_PRE_DOWN, dev); > nb->notifier_call(nb, NETDEV_GOING_DOWN, dev); > nb->notifier_call(nb, NETDEV_DOWN, dev); > } [...] The return value has to be ignored here. Not sure it makes any sense to call the notifier at all. Ben.
On Wed, 31 Aug 2011 16:25:51 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote: > On Wed, 2011-08-31 at 17:15 +0200, Jiri Pirko wrote: > > In some situations, like when the device is used as slave device in > > bond/br/etc it is not nice if someone closes the device. This allows > > it's masters to forbid this closure. > > No it doesn't. > > [...] > > @@ -1269,9 +1282,12 @@ static int dev_close_many(struct list_head *head) > > struct net_device *dev, *tmp; > > LIST_HEAD(tmp_list); > > > > - list_for_each_entry_safe(dev, tmp, head, unreg_list) > > + list_for_each_entry_safe(dev, tmp, head, unreg_list) { > > if (!(dev->flags & IFF_UP)) > > list_move(&dev->unreg_list, &tmp_list); > > + else > > + __dev_pre_close(dev); > > + } > > > > __dev_close_many(head); > > The return value is ignored here. > > And this is called from dev_close(), where you are adding the > notification as well. So the notifier will usually be called twice. > > [...] > > @@ -1397,6 +1418,7 @@ rollback: > > break; > > > > if (dev->flags & IFF_UP) { > > + nb->notifier_call(nb, NETDEV_PRE_DOWN, dev); > > nb->notifier_call(nb, NETDEV_GOING_DOWN, dev); > > nb->notifier_call(nb, NETDEV_DOWN, dev); > > } > [...] > > The return value has to be ignored here. Not sure it makes any sense to > call the notifier at all. > > Ben. > Also we need to allow rmmod'ing a network device even it is part of a bridge and that implicitly calls close. -- 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
Wed, Aug 31, 2011 at 05:25:51PM CEST, bhutchings@solarflare.com wrote: >On Wed, 2011-08-31 at 17:15 +0200, Jiri Pirko wrote: >> In some situations, like when the device is used as slave device in >> bond/br/etc it is not nice if someone closes the device. This allows >> it's masters to forbid this closure. > >No it doesn't. It does > >[...] >> @@ -1269,9 +1282,12 @@ static int dev_close_many(struct list_head *head) >> struct net_device *dev, *tmp; >> LIST_HEAD(tmp_list); >> >> - list_for_each_entry_safe(dev, tmp, head, unreg_list) >> + list_for_each_entry_safe(dev, tmp, head, unreg_list) { >> if (!(dev->flags & IFF_UP)) >> list_move(&dev->unreg_list, &tmp_list); >> + else >> + __dev_pre_close(dev); >> + } >> >> __dev_close_many(head); > >The return value is ignored here. That's intended. The reason is this is called from rollback_registered_many - refuse should be ignored in that case > >And this is called from dev_close(), where you are adding the >notification as well. So the notifier will usually be called twice. > Indeed. Anyway I thought about it and we probably do not need this patch as Stephen said. >[...] >> @@ -1397,6 +1418,7 @@ rollback: >> break; >> >> if (dev->flags & IFF_UP) { >> + nb->notifier_call(nb, NETDEV_PRE_DOWN, dev); >> nb->notifier_call(nb, NETDEV_GOING_DOWN, dev); >> nb->notifier_call(nb, NETDEV_DOWN, dev); >> } >[...] > >The return value has to be ignored here. Not sure it makes any sense to >call the notifier at all. > >Ben. > >-- >Ben Hutchings, Staff Engineer, Solarflare >Not speaking for my employer; that's the marketing department's job. >They asked us to note that Solarflare product names are trademarked. > -- 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
Wed, Aug 31, 2011 at 05:53:38PM CEST, shemminger@vyatta.com wrote: >On Wed, 31 Aug 2011 16:25:51 +0100 >Ben Hutchings <bhutchings@solarflare.com> wrote: > >> On Wed, 2011-08-31 at 17:15 +0200, Jiri Pirko wrote: >> > In some situations, like when the device is used as slave device in >> > bond/br/etc it is not nice if someone closes the device. This allows >> > it's masters to forbid this closure. >> >> No it doesn't. >> >> [...] >> > @@ -1269,9 +1282,12 @@ static int dev_close_many(struct list_head *head) >> > struct net_device *dev, *tmp; >> > LIST_HEAD(tmp_list); >> > >> > - list_for_each_entry_safe(dev, tmp, head, unreg_list) >> > + list_for_each_entry_safe(dev, tmp, head, unreg_list) { >> > if (!(dev->flags & IFF_UP)) >> > list_move(&dev->unreg_list, &tmp_list); >> > + else >> > + __dev_pre_close(dev); >> > + } >> > >> > __dev_close_many(head); >> >> The return value is ignored here. >> >> And this is called from dev_close(), where you are adding the >> notification as well. So the notifier will usually be called twice. >> >> [...] >> > @@ -1397,6 +1418,7 @@ rollback: >> > break; >> > >> > if (dev->flags & IFF_UP) { >> > + nb->notifier_call(nb, NETDEV_PRE_DOWN, dev); >> > nb->notifier_call(nb, NETDEV_GOING_DOWN, dev); >> > nb->notifier_call(nb, NETDEV_DOWN, dev); >> > } >> [...] >> >> The return value has to be ignored here. Not sure it makes any sense to >> call the notifier at all. >> >> Ben. >> > >Also we need to allow rmmod'ing a network device even it is >part of a bridge and that implicitly >calls close. this is not a problem because when it's called from rollback_registered_many, return value if pre_down is ignored. -- 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/include/linux/netdevice.h b/include/linux/netdevice.h index dad7e4d..b8047d3 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1559,6 +1559,7 @@ struct packet_type { #define NETDEV_RELEASE 0x0012 #define NETDEV_NOTIFY_PEERS 0x0013 #define NETDEV_JOIN 0x0014 +#define NETDEV_PRE_DOWN 0x0015 extern int register_netdevice_notifier(struct notifier_block *nb); extern int unregister_netdevice_notifier(struct notifier_block *nb); diff --git a/net/core/dev.c b/net/core/dev.c index 11b0fc7..d252a7e 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1253,11 +1253,24 @@ static int __dev_close_many(struct list_head *head) return 0; } +static int __dev_pre_close(struct net_device *dev) +{ + int err; + + err = call_netdevice_notifiers(NETDEV_PRE_DOWN, dev); + if (err) + return notifier_to_errno(err); + return 0; +} + static int __dev_close(struct net_device *dev) { int retval; LIST_HEAD(single); + retval = __dev_pre_close(dev); + if (retval) + return retval; list_add(&dev->unreg_list, &single); retval = __dev_close_many(&single); list_del(&single); @@ -1269,9 +1282,12 @@ static int dev_close_many(struct list_head *head) struct net_device *dev, *tmp; LIST_HEAD(tmp_list); - list_for_each_entry_safe(dev, tmp, head, unreg_list) + list_for_each_entry_safe(dev, tmp, head, unreg_list) { if (!(dev->flags & IFF_UP)) list_move(&dev->unreg_list, &tmp_list); + else + __dev_pre_close(dev); + } __dev_close_many(head); @@ -1289,21 +1305,26 @@ static int dev_close_many(struct list_head *head) * dev_close - shutdown an interface. * @dev: device to shutdown * - * This function moves an active device into down state. A - * %NETDEV_GOING_DOWN is sent to the netdev notifier chain. The device - * is then deactivated and finally a %NETDEV_DOWN is sent to the notifier - * chain. + * This function moves an active device into down state. + * A %NETDEV_PRE_DOWN and %NETDEV_GOING_DOWN is sent to the netdev + * notifier chain. The device is then deactivated and finally + * a %NETDEV_DOWN is sent to the notifier chain. */ int dev_close(struct net_device *dev) { + int retval = 0; + if (dev->flags & IFF_UP) { LIST_HEAD(single); + retval = __dev_pre_close(dev); + if (retval) + return retval; list_add(&dev->unreg_list, &single); dev_close_many(&single); list_del(&single); } - return 0; + return retval; } EXPORT_SYMBOL(dev_close); @@ -1397,6 +1418,7 @@ rollback: break; if (dev->flags & IFF_UP) { + nb->notifier_call(nb, NETDEV_PRE_DOWN, dev); nb->notifier_call(nb, NETDEV_GOING_DOWN, dev); nb->notifier_call(nb, NETDEV_DOWN, dev); } diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 39f8dd6..34f5b32 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1997,6 +1997,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi case NETDEV_UP: case NETDEV_DOWN: case NETDEV_PRE_UP: + case NETDEV_PRE_DOWN: case NETDEV_POST_INIT: case NETDEV_REGISTER: case NETDEV_CHANGE:
In some situations, like when the device is used as slave device in bond/br/etc it is not nice if someone closes the device. This allows it's masters to forbid this closure. Signed-off-by: Jiri Pirko <jpirko@redhat.com> --- include/linux/netdevice.h | 1 + net/core/dev.c | 34 ++++++++++++++++++++++++++++------ net/core/rtnetlink.c | 1 + 3 files changed, 30 insertions(+), 6 deletions(-)