* [PATCH net] rtnl: don't send rtnl msg for unregistered iface @ 2015-05-11 14:25 Nicolas Dichtel 2015-05-11 15:37 ` Jiri Pirko 0 siblings, 1 reply; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-11 14:25 UTC (permalink / raw) To: davem; +Cc: netdev, j.vosburgh, vfalico, gospo, Nicolas Dichtel, Jiri Pirko Before the patch, the command 'ip link add bond2 type bond mode 802.3ad' causes the kernel to send a rtnl message for the bond2 interface, with an ifindex 0. 'ip monitor' shows: 0: bond2: <BROADCAST,MULTICAST,MASTER> mtu 1500 state DOWN group default link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff 9: bond2@NONE: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN group default link/ether ea:3e:1f:53:92:7b brd ff:ff:ff:ff:ff:ff [snip] Fixes: d4261e565000 ("bonding: create netlink event when bonding option is changed") CC: Jiri Pirko <jiri@resnulli.us> Reported-by: Julien Meunier <julien.meunier@6wind.com> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- net/core/rtnetlink.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 837d30b5ffed..721ca1b0e734 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -3273,6 +3273,8 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi case NETDEV_BONDING_INFO: break; default: + if (!dev->ifindex) + break; rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL); break; } -- 2.2.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface 2015-05-11 14:25 [PATCH net] rtnl: don't send rtnl msg for unregistered iface Nicolas Dichtel @ 2015-05-11 15:37 ` Jiri Pirko 2015-05-11 16:04 ` Nicolas Dichtel 0 siblings, 1 reply; 24+ messages in thread From: Jiri Pirko @ 2015-05-11 15:37 UTC (permalink / raw) To: Nicolas Dichtel; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Mon, May 11, 2015 at 04:25:46PM CEST, nicolas.dichtel@6wind.com wrote: >Before the patch, the command 'ip link add bond2 type bond mode 802.3ad' >causes the kernel to send a rtnl message for the bond2 interface, with an >ifindex 0. > >'ip monitor' shows: >0: bond2: <BROADCAST,MULTICAST,MASTER> mtu 1500 state DOWN group default > link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff >9: bond2@NONE: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN group default > link/ether ea:3e:1f:53:92:7b brd ff:ff:ff:ff:ff:ff >[snip] > >Fixes: d4261e565000 ("bonding: create netlink event when bonding option is changed") >CC: Jiri Pirko <jiri@resnulli.us> >Reported-by: Julien Meunier <julien.meunier@6wind.com> >Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >--- > net/core/rtnetlink.c | 2 ++ > 1 file changed, 2 insertions(+) > >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >index 837d30b5ffed..721ca1b0e734 100644 >--- a/net/core/rtnetlink.c >+++ b/net/core/rtnetlink.c >@@ -3273,6 +3273,8 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi > case NETDEV_BONDING_INFO: > break; > default: >+ if (!dev->ifindex) >+ break; I don't think this is the correct way to fix this. How ifindex can be 0 here? Ifindex is set in register_netdevice and looking at bond_create, I don't see any call to __bond_opt_set before that. But since it apparently is, the ordering should be changed so register_netdevice is called first. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface 2015-05-11 15:37 ` Jiri Pirko @ 2015-05-11 16:04 ` Nicolas Dichtel 2015-05-11 16:15 ` Nicolas Dichtel 0 siblings, 1 reply; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-11 16:04 UTC (permalink / raw) To: Jiri Pirko; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Le 11/05/2015 17:37, Jiri Pirko a écrit : [snip] > > I don't think this is the correct way to fix this. > > How ifindex can be 0 here? Ifindex is set in register_netdevice and > looking at bond_create, I don't see any call to __bond_opt_set before > that. But since it apparently is, the ordering should be changed so > register_netdevice is called first. bond_newlink => bond_changelink => __bond_opt_set and then back to bond_newlink => register_netdevice Regards, Nicolas ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface 2015-05-11 16:04 ` Nicolas Dichtel @ 2015-05-11 16:15 ` Nicolas Dichtel 2015-05-11 17:53 ` Jiri Pirko 0 siblings, 1 reply; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-11 16:15 UTC (permalink / raw) To: Jiri Pirko; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Le 11/05/2015 18:04, Nicolas Dichtel a écrit : > Le 11/05/2015 17:37, Jiri Pirko a écrit : > [snip] >> >> I don't think this is the correct way to fix this. >> >> How ifindex can be 0 here? Ifindex is set in register_netdevice and >> looking at bond_create, I don't see any call to __bond_opt_set before >> that. But since it apparently is, the ordering should be changed so >> register_netdevice is called first. I also don't see why we would prevent to register a bonding interface directly with the right mode. > bond_newlink => bond_changelink => __bond_opt_set > and then back to bond_newlink => register_netdevice > > > Regards, > Nicolas ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface 2015-05-11 16:15 ` Nicolas Dichtel @ 2015-05-11 17:53 ` Jiri Pirko 2015-05-11 22:15 ` Andy Gospodarek ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Jiri Pirko @ 2015-05-11 17:53 UTC (permalink / raw) To: Nicolas Dichtel; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Mon, May 11, 2015 at 06:15:00PM CEST, nicolas.dichtel@6wind.com wrote: >Le 11/05/2015 18:04, Nicolas Dichtel a écrit : >>Le 11/05/2015 17:37, Jiri Pirko a écrit : >>[snip] >>> >>>I don't think this is the correct way to fix this. >>> >>>How ifindex can be 0 here? Ifindex is set in register_netdevice and >>>looking at bond_create, I don't see any call to __bond_opt_set before >>>that. But since it apparently is, the ordering should be changed so >>>register_netdevice is called first. >I also don't see why we would prevent to register a bonding interface directly >with the right mode. > >>bond_newlink => bond_changelink => __bond_opt_set >>and then back to bond_newlink => register_netdevice I see it now. Why not to do register first, changelink later? Or, change __bond_opt_set to call call_netdevice_notifiers only in case dev->reg_state == NETREG_REGISTERED? Looking at the other places this check happens, looks like a little helper like "netdev_check_registered" might be convenient. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface 2015-05-11 17:53 ` Jiri Pirko @ 2015-05-11 22:15 ` Andy Gospodarek 2015-05-12 8:43 ` Nicolas Dichtel 2015-05-12 15:03 ` [PATCH net v2] rtnl/bond: " Nicolas Dichtel 2 siblings, 0 replies; 24+ messages in thread From: Andy Gospodarek @ 2015-05-11 22:15 UTC (permalink / raw) To: Jiri Pirko; +Cc: Nicolas Dichtel, davem, netdev, j.vosburgh, vfalico On Mon, May 11, 2015 at 07:53:55PM +0200, Jiri Pirko wrote: > Mon, May 11, 2015 at 06:15:00PM CEST, nicolas.dichtel@6wind.com wrote: > >Le 11/05/2015 18:04, Nicolas Dichtel a écrit : > >>Le 11/05/2015 17:37, Jiri Pirko a écrit : > >>[snip] > >>> > >>>I don't think this is the correct way to fix this. > >>> > >>>How ifindex can be 0 here? Ifindex is set in register_netdevice and > >>>looking at bond_create, I don't see any call to __bond_opt_set before > >>>that. But since it apparently is, the ordering should be changed so > >>>register_netdevice is called first. > >I also don't see why we would prevent to register a bonding interface directly > >with the right mode. > > > >>bond_newlink => bond_changelink => __bond_opt_set > >>and then back to bond_newlink => register_netdevice > > I see it now. Why not to do register first, changelink later? > Or, change __bond_opt_set to call call_netdevice_notifiers only in case > dev->reg_state == NETREG_REGISTERED? Looking at the other places this This is probably an excellent approach. > check happens, looks like a little helper like "netdev_check_registered" > might be convenient. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface 2015-05-11 17:53 ` Jiri Pirko 2015-05-11 22:15 ` Andy Gospodarek @ 2015-05-12 8:43 ` Nicolas Dichtel 2015-05-12 15:03 ` [PATCH net v2] rtnl/bond: " Nicolas Dichtel 2 siblings, 0 replies; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-12 8:43 UTC (permalink / raw) To: Jiri Pirko; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Le 11/05/2015 19:53, Jiri Pirko a écrit : [snip] > Or, change __bond_opt_set to call call_netdevice_notifiers only in case > dev->reg_state == NETREG_REGISTERED? Looking at the other places this Why not. It will fix this bug, but I tend to put something more general in rtnetlink to avoid such bug in the future. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net v2] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-11 17:53 ` Jiri Pirko 2015-05-11 22:15 ` Andy Gospodarek 2015-05-12 8:43 ` Nicolas Dichtel @ 2015-05-12 15:03 ` Nicolas Dichtel 2015-05-12 15:06 ` Nicolas Dichtel 2 siblings, 1 reply; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-12 15:03 UTC (permalink / raw) To: jiri; +Cc: davem, netdev, j.vosburgh, vfalico, gospo, Nicolas Dichtel Before the patch, the command 'ip link add bond2 type bond mode 802.3ad' causes the kernel to send a rtnl message for the bond2 interface, with an ifindex 0. 'ip monitor' shows: 0: bond2: <BROADCAST,MULTICAST,MASTER> mtu 1500 state DOWN group default link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff 9: bond2@NONE: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN group default link/ether ea:3e:1f:53:92:7b brd ff:ff:ff:ff:ff:ff [snip] First, nobody seems to care about NETDEV_CHANGEINFODATA (commit d4261e565000 says that it was put only to notify userspace). Hence, we can call directly rtmsg_ifinfo(). Secondly, prevent in rtnetlink_event() to send notifications to userspace about unregistered interfaces. Fixes: d4261e565000 ("bonding: create netlink event when bonding option is changed") CC: Jiri Pirko <jiri@resnulli.us> Reported-by: Julien Meunier <julien.meunier@6wind.com> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- v2: check dev->reg_state instead of ifindex call rtmsg_ifinfo() directly in __bond_opt_set() There is about 20 occurences of "dev->reg_state .= NETREG_REGISTERED". I will send a patch on net-next to introduce an helper for this when net will be merged with this patch in net-next. drivers/net/bonding/bond_options.c | 4 ++-- include/linux/netdevice.h | 3 +-- net/core/rtnetlink.c | 3 ++- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index 4df28943d222..882e0599e72b 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -624,8 +624,8 @@ int __bond_opt_set(struct bonding *bond, out: if (ret) bond_opt_error_interpret(bond, opt, ret, val); - else - call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev); + else if (bond->dev->reg_state == NETREG_REGISTERED) + rtmsg_ifinfo(RTM_NEWLINK, bond->dev, 0, GFP_KERNEL); return ret; } diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 1899c74a7127..c040d48925fd 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2069,8 +2069,7 @@ struct pcpu_sw_netstats { #define NETDEV_CHANGEUPPER 0x0015 #define NETDEV_RESEND_IGMP 0x0016 #define NETDEV_PRECHANGEMTU 0x0017 /* notify before mtu change happened */ -#define NETDEV_CHANGEINFODATA 0x0018 -#define NETDEV_BONDING_INFO 0x0019 +#define NETDEV_BONDING_INFO 0x0018 int register_netdevice_notifier(struct notifier_block *nb); int unregister_netdevice_notifier(struct notifier_block *nb); diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 837d30b5ffed..e6e4d7fc863e 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -3273,7 +3273,8 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi case NETDEV_BONDING_INFO: break; default: - rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL); + if (dev->reg_state == NETREG_REGISTERED) + rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL); break; } return NOTIFY_DONE; -- 2.2.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH net v2] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-12 15:03 ` [PATCH net v2] rtnl/bond: " Nicolas Dichtel @ 2015-05-12 15:06 ` Nicolas Dichtel 2015-05-12 15:17 ` [PATCH net v3] " Nicolas Dichtel 0 siblings, 1 reply; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-12 15:06 UTC (permalink / raw) To: jiri; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Le 12/05/2015 17:03, Nicolas Dichtel a écrit : [snip] > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -3273,7 +3273,8 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi > case NETDEV_BONDING_INFO: > break; > default: > - rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL); > + if (dev->reg_state == NETREG_REGISTERED) > + rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL); In fact, it's probably better to put this in rtmsg_ifinfo(). Will send a v3, sorry for the noise. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-12 15:06 ` Nicolas Dichtel @ 2015-05-12 15:17 ` Nicolas Dichtel 2015-05-12 15:58 ` Jiri Pirko 0 siblings, 1 reply; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-12 15:17 UTC (permalink / raw) To: davem; +Cc: netdev, j.vosburgh, vfalico, gospo, jiri, Nicolas Dichtel Before the patch, the command 'ip link add bond2 type bond mode 802.3ad' causes the kernel to send a rtnl message for the bond2 interface, with an ifindex 0. 'ip monitor' shows: 0: bond2: <BROADCAST,MULTICAST,MASTER> mtu 1500 state DOWN group default link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff 9: bond2@NONE: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN group default link/ether ea:3e:1f:53:92:7b brd ff:ff:ff:ff:ff:ff [snip] First, nobody seems to care about NETDEV_CHANGEINFODATA (commit d4261e565000 says that it was put only to notify userspace). Hence, we can call directly rtmsg_ifinfo(). Secondly, prevent in rtmsg_ifinfo() to send notifications to userspace about unregistered interfaces. Fixes: d4261e565000 ("bonding: create netlink event when bonding option is changed") CC: Jiri Pirko <jiri@resnulli.us> Reported-by: Julien Meunier <julien.meunier@6wind.com> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- v3: move reg_state check in rtmsg_ifinfo() v2: check dev->reg_state instead of ifindex call rtmsg_ifinfo() directly in __bond_opt_set() There is about 20 occurences of "dev->reg_state .= NETREG_REGISTERED". I will send a patch on net-next to introduce an helper for this when net will be merged with this patch in net-next. drivers/net/bonding/bond_options.c | 2 +- include/linux/netdevice.h | 3 +-- net/core/rtnetlink.c | 3 +++ 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index 4df28943d222..a2b72cc0e3e8 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -625,7 +625,7 @@ out: if (ret) bond_opt_error_interpret(bond, opt, ret, val); else - call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev); + rtmsg_ifinfo(RTM_NEWLINK, bond->dev, 0, GFP_KERNEL); return ret; } diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 1899c74a7127..c040d48925fd 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2069,8 +2069,7 @@ struct pcpu_sw_netstats { #define NETDEV_CHANGEUPPER 0x0015 #define NETDEV_RESEND_IGMP 0x0016 #define NETDEV_PRECHANGEMTU 0x0017 /* notify before mtu change happened */ -#define NETDEV_CHANGEINFODATA 0x0018 -#define NETDEV_BONDING_INFO 0x0019 +#define NETDEV_BONDING_INFO 0x0018 int register_netdevice_notifier(struct notifier_block *nb); int unregister_netdevice_notifier(struct notifier_block *nb); diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 837d30b5ffed..7b25f1ef3d75 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2415,6 +2415,9 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, { struct sk_buff *skb; + if (dev->reg_state != NETREG_REGISTERED) + return; + skb = rtmsg_ifinfo_build_skb(type, dev, change, flags); if (skb) rtmsg_ifinfo_send(skb, dev, flags); -- 2.2.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-12 15:17 ` [PATCH net v3] " Nicolas Dichtel @ 2015-05-12 15:58 ` Jiri Pirko 2015-05-12 16:10 ` Nicolas Dichtel 2015-05-12 17:12 ` [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface Andy Gospodarek 0 siblings, 2 replies; 24+ messages in thread From: Jiri Pirko @ 2015-05-12 15:58 UTC (permalink / raw) To: Nicolas Dichtel; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Tue, May 12, 2015 at 05:17:45PM CEST, nicolas.dichtel@6wind.com wrote: >Before the patch, the command 'ip link add bond2 type bond mode 802.3ad' >causes the kernel to send a rtnl message for the bond2 interface, with an >ifindex 0. > >'ip monitor' shows: >0: bond2: <BROADCAST,MULTICAST,MASTER> mtu 1500 state DOWN group default > link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff >9: bond2@NONE: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN group default > link/ether ea:3e:1f:53:92:7b brd ff:ff:ff:ff:ff:ff >[snip] > >First, nobody seems to care about NETDEV_CHANGEINFODATA (commit >d4261e565000 says that it was put only to notify userspace). Hence, we can >call directly rtmsg_ifinfo(). Please leave this notifier here. Will be needed in very near future for LAG offloading purposes. >Secondly, prevent in rtmsg_ifinfo() to send notifications to userspace >about unregistered interfaces. I'm not sure about this. Why caller of rtmsg_ifinfo don't take care of it? Seems more logical to me. > >Fixes: d4261e565000 ("bonding: create netlink event when bonding option is changed") >CC: Jiri Pirko <jiri@resnulli.us> >Reported-by: Julien Meunier <julien.meunier@6wind.com> >Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >--- > >v3: move reg_state check in rtmsg_ifinfo() > >v2: check dev->reg_state instead of ifindex > call rtmsg_ifinfo() directly in __bond_opt_set() > >There is about 20 occurences of "dev->reg_state .= NETREG_REGISTERED". >I will send a patch on net-next to introduce an helper for this when net >will be merged with this patch in net-next. > > drivers/net/bonding/bond_options.c | 2 +- > include/linux/netdevice.h | 3 +-- > net/core/rtnetlink.c | 3 +++ > 3 files changed, 5 insertions(+), 3 deletions(-) > >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c >index 4df28943d222..a2b72cc0e3e8 100644 >--- a/drivers/net/bonding/bond_options.c >+++ b/drivers/net/bonding/bond_options.c >@@ -625,7 +625,7 @@ out: > if (ret) > bond_opt_error_interpret(bond, opt, ret, val); > else >- call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev); >+ rtmsg_ifinfo(RTM_NEWLINK, bond->dev, 0, GFP_KERNEL); > > return ret; > } >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >index 1899c74a7127..c040d48925fd 100644 >--- a/include/linux/netdevice.h >+++ b/include/linux/netdevice.h >@@ -2069,8 +2069,7 @@ struct pcpu_sw_netstats { > #define NETDEV_CHANGEUPPER 0x0015 > #define NETDEV_RESEND_IGMP 0x0016 > #define NETDEV_PRECHANGEMTU 0x0017 /* notify before mtu change happened */ >-#define NETDEV_CHANGEINFODATA 0x0018 >-#define NETDEV_BONDING_INFO 0x0019 >+#define NETDEV_BONDING_INFO 0x0018 > > int register_netdevice_notifier(struct notifier_block *nb); > int unregister_netdevice_notifier(struct notifier_block *nb); >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >index 837d30b5ffed..7b25f1ef3d75 100644 >--- a/net/core/rtnetlink.c >+++ b/net/core/rtnetlink.c >@@ -2415,6 +2415,9 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, > { > struct sk_buff *skb; > >+ if (dev->reg_state != NETREG_REGISTERED) >+ return; >+ > skb = rtmsg_ifinfo_build_skb(type, dev, change, flags); > if (skb) > rtmsg_ifinfo_send(skb, dev, flags); >-- >2.2.2 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-12 15:58 ` Jiri Pirko @ 2015-05-12 16:10 ` Nicolas Dichtel 2015-05-12 16:14 ` Jiri Pirko 2015-05-12 17:12 ` [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface Andy Gospodarek 1 sibling, 1 reply; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-12 16:10 UTC (permalink / raw) To: Jiri Pirko; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Le 12/05/2015 17:58, Jiri Pirko a écrit : > Tue, May 12, 2015 at 05:17:45PM CEST, nicolas.dichtel@6wind.com wrote: [snip] >> First, nobody seems to care about NETDEV_CHANGEINFODATA (commit >> d4261e565000 says that it was put only to notify userspace). Hence, we can >> call directly rtmsg_ifinfo(). > > Please leave this notifier here. Will be needed in very near future for > LAG offloading purposes. Ok. > >> Secondly, prevent in rtmsg_ifinfo() to send notifications to userspace >> about unregistered interfaces. > > I'm not sure about this. Why caller of rtmsg_ifinfo don't take care of > it? Seems more logical to me. Simply to avoid code duplication and ensure the bug will not show up in the future. The existing bug shows that it's easy to forget that check. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-12 16:10 ` Nicolas Dichtel @ 2015-05-12 16:14 ` Jiri Pirko 2015-05-13 12:19 ` [PATCH net v4] " Nicolas Dichtel 0 siblings, 1 reply; 24+ messages in thread From: Jiri Pirko @ 2015-05-12 16:14 UTC (permalink / raw) To: Nicolas Dichtel; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Tue, May 12, 2015 at 06:10:56PM CEST, nicolas.dichtel@6wind.com wrote: >Le 12/05/2015 17:58, Jiri Pirko a écrit : >>Tue, May 12, 2015 at 05:17:45PM CEST, nicolas.dichtel@6wind.com wrote: >[snip] >>>First, nobody seems to care about NETDEV_CHANGEINFODATA (commit >>>d4261e565000 says that it was put only to notify userspace). Hence, we can >>>call directly rtmsg_ifinfo(). >> >>Please leave this notifier here. Will be needed in very near future for >>LAG offloading purposes. >Ok. > >> >>>Secondly, prevent in rtmsg_ifinfo() to send notifications to userspace >>>about unregistered interfaces. >> >>I'm not sure about this. Why caller of rtmsg_ifinfo don't take care of >>it? Seems more logical to me. >Simply to avoid code duplication and ensure the bug will not show up in the >future. The existing bug shows that it's easy to forget that check. I don't know, seems to me more like curing consequence, not the cause. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-12 16:14 ` Jiri Pirko @ 2015-05-13 12:19 ` Nicolas Dichtel 2015-05-13 12:24 ` Jiri Pirko ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-13 12:19 UTC (permalink / raw) To: jiri; +Cc: davem, netdev, j.vosburgh, vfalico, gospo, Nicolas Dichtel Before the patch, the command 'ip link add bond2 type bond mode 802.3ad' causes the kernel to send a rtnl message for the bond2 interface, with an ifindex 0. 'ip monitor' shows: 0: bond2: <BROADCAST,MULTICAST,MASTER> mtu 1500 state DOWN group default link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff 9: bond2@NONE: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN group default link/ether ea:3e:1f:53:92:7b brd ff:ff:ff:ff:ff:ff [snip] The patch fixes the spotted bug by checking in bond driver if the interface is registered before calling the notifier chain. It also adds a check in rtmsg_ifinfo() to prevent this kind of bug in the future. Fixes: d4261e565000 ("bonding: create netlink event when bonding option is changed") CC: Jiri Pirko <jiri@resnulli.us> Reported-by: Julien Meunier <julien.meunier@6wind.com> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- v4: keep the call to the notifier chain v3: move reg_state check in rtmsg_ifinfo() v2: check dev->reg_state instead of ifindex call rtmsg_ifinfo() directly in __bond_opt_set() There is about 20 occurences of "dev->reg_state .= NETREG_REGISTERED". I will send a patch on net-next to introduce an helper for this when net will be merged with this patch in net-next. drivers/net/bonding/bond_options.c | 2 +- net/core/rtnetlink.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index 4df28943d222..e8d3c1d35453 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -624,7 +624,7 @@ int __bond_opt_set(struct bonding *bond, out: if (ret) bond_opt_error_interpret(bond, opt, ret, val); - else + else if (bond->dev->reg_state == NETREG_REGISTERED) call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev); return ret; diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 837d30b5ffed..7b25f1ef3d75 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2415,6 +2415,9 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, { struct sk_buff *skb; + if (dev->reg_state != NETREG_REGISTERED) + return; + skb = rtmsg_ifinfo_build_skb(type, dev, change, flags); if (skb) rtmsg_ifinfo_send(skb, dev, flags); -- 2.2.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-13 12:19 ` [PATCH net v4] " Nicolas Dichtel @ 2015-05-13 12:24 ` Jiri Pirko 2015-05-13 12:36 ` Nicolas Dichtel 2015-05-18 2:44 ` David Miller 2015-07-13 14:11 ` Kristian Evensen 2 siblings, 1 reply; 24+ messages in thread From: Jiri Pirko @ 2015-05-13 12:24 UTC (permalink / raw) To: Nicolas Dichtel; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Wed, May 13, 2015 at 02:19:42PM CEST, nicolas.dichtel@6wind.com wrote: >Before the patch, the command 'ip link add bond2 type bond mode 802.3ad' >causes the kernel to send a rtnl message for the bond2 interface, with an >ifindex 0. > >'ip monitor' shows: >0: bond2: <BROADCAST,MULTICAST,MASTER> mtu 1500 state DOWN group default > link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff >9: bond2@NONE: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN group default > link/ether ea:3e:1f:53:92:7b brd ff:ff:ff:ff:ff:ff >[snip] > >The patch fixes the spotted bug by checking in bond driver if the interface >is registered before calling the notifier chain. >It also adds a check in rtmsg_ifinfo() to prevent this kind of bug in the >future. > >Fixes: d4261e565000 ("bonding: create netlink event when bonding option is changed") >CC: Jiri Pirko <jiri@resnulli.us> >Reported-by: Julien Meunier <julien.meunier@6wind.com> >Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >--- > >v4: keep the call to the notifier chain > >v3: move reg_state check in rtmsg_ifinfo() > >v2: check dev->reg_state instead of ifindex > call rtmsg_ifinfo() directly in __bond_opt_set() > >There is about 20 occurences of "dev->reg_state .= NETREG_REGISTERED". >I will send a patch on net-next to introduce an helper for this when net >will be merged with this patch in net-next. > > drivers/net/bonding/bond_options.c | 2 +- > net/core/rtnetlink.c | 3 +++ > 2 files changed, 4 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c >index 4df28943d222..e8d3c1d35453 100644 >--- a/drivers/net/bonding/bond_options.c >+++ b/drivers/net/bonding/bond_options.c >@@ -624,7 +624,7 @@ int __bond_opt_set(struct bonding *bond, > out: > if (ret) > bond_opt_error_interpret(bond, opt, ret, val); >- else >+ else if (bond->dev->reg_state == NETREG_REGISTERED) > call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev); > > return ret; >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >index 837d30b5ffed..7b25f1ef3d75 100644 >--- a/net/core/rtnetlink.c >+++ b/net/core/rtnetlink.c >@@ -2415,6 +2415,9 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, > { > struct sk_buff *skb; > >+ if (dev->reg_state != NETREG_REGISTERED) >+ return; >+ This is redundant now that you check it in __bond_opt_set. Sorry to be a pain Nicolas :/ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-13 12:24 ` Jiri Pirko @ 2015-05-13 12:36 ` Nicolas Dichtel 0 siblings, 0 replies; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-13 12:36 UTC (permalink / raw) To: Jiri Pirko; +Cc: davem, netdev, j.vosburgh, vfalico, gospo Le 13/05/2015 14:24, Jiri Pirko a écrit : > Wed, May 13, 2015 at 02:19:42PM CEST, nicolas.dichtel@6wind.com wrote: [snip] >> The patch fixes the spotted bug by checking in bond driver if the interface >> is registered before calling the notifier chain. >> It also adds a check in rtmsg_ifinfo() to prevent this kind of bug in the >> future. [snip] >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -2415,6 +2415,9 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, >> { >> struct sk_buff *skb; >> >> + if (dev->reg_state != NETREG_REGISTERED) >> + return; >> + > > This is redundant now that you check it in __bond_opt_set. As said in the commit log ;-) > > Sorry to be a pain Nicolas :/ > Yeah, I see that you don't want this check ;-) In fact, I don't really understand why because it does not harm and it prevents to have again the same bug. Does anyone else have an opinion on that? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-13 12:19 ` [PATCH net v4] " Nicolas Dichtel 2015-05-13 12:24 ` Jiri Pirko @ 2015-05-18 2:44 ` David Miller 2015-07-13 14:11 ` Kristian Evensen 2 siblings, 0 replies; 24+ messages in thread From: David Miller @ 2015-05-18 2:44 UTC (permalink / raw) To: nicolas.dichtel; +Cc: jiri, netdev, j.vosburgh, vfalico, gospo From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Wed, 13 May 2015 14:19:42 +0200 > Before the patch, the command 'ip link add bond2 type bond mode 802.3ad' > causes the kernel to send a rtnl message for the bond2 interface, with an > ifindex 0. > > 'ip monitor' shows: > 0: bond2: <BROADCAST,MULTICAST,MASTER> mtu 1500 state DOWN group default > link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff > 9: bond2@NONE: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN group default > link/ether ea:3e:1f:53:92:7b brd ff:ff:ff:ff:ff:ff > [snip] > > The patch fixes the spotted bug by checking in bond driver if the interface > is registered before calling the notifier chain. > It also adds a check in rtmsg_ifinfo() to prevent this kind of bug in the > future. > > Fixes: d4261e565000 ("bonding: create netlink event when bonding option is changed") > CC: Jiri Pirko <jiri@resnulli.us> > Reported-by: Julien Meunier <julien.meunier@6wind.com> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> Applied and queued up for -stable, thanks Nicolas. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-13 12:19 ` [PATCH net v4] " Nicolas Dichtel 2015-05-13 12:24 ` Jiri Pirko 2015-05-18 2:44 ` David Miller @ 2015-07-13 14:11 ` Kristian Evensen 2015-07-15 15:07 ` Nicolas Dichtel 2015-07-16 9:06 ` Nicolas Dichtel 2 siblings, 2 replies; 24+ messages in thread From: Kristian Evensen @ 2015-07-13 14:11 UTC (permalink / raw) To: Nicolas Dichtel Cc: jiri, David Miller, Network Development, j.vosburgh, vfalico, gospo Hello, I have a quick question about this patch. On Wed, May 13, 2015 at 2:19 PM, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 837d30b5ffed..7b25f1ef3d75 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -2415,6 +2415,9 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, > { > struct sk_buff *skb; > > + if (dev->reg_state != NETREG_REGISTERED) > + return; > + Is this check correct, or placed at the correct location? The reason I am asking is as follows. In rollback_registered_many(), dev->reg_state is set to NETREG_UNREGISTERING for devices that will be unregistered. When rtmsg_ifinfo_build_skb(RTM_DELLINK, ...) is called in the following loop in rollback_registered_many, this comparison will always be true and no DELLINK event generated. This change led to some applications I have not behaving as expected due to missing DELLINK when network devices are removed. I also see no DELLINK with ip mon link. Removing the check restores the old behavior (DELLINK events are generated). My machine is running 3.18.18, which includes this fix. Thanks in advance for any help, Kristian ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface 2015-07-13 14:11 ` Kristian Evensen @ 2015-07-15 15:07 ` Nicolas Dichtel 2015-07-16 9:06 ` Nicolas Dichtel 1 sibling, 0 replies; 24+ messages in thread From: Nicolas Dichtel @ 2015-07-15 15:07 UTC (permalink / raw) To: Kristian Evensen Cc: jiri, David Miller, Network Development, j.vosburgh, vfalico, gospo Le 13/07/2015 16:11, Kristian Evensen a écrit : > Hello, > > I have a quick question about this patch. > > On Wed, May 13, 2015 at 2:19 PM, Nicolas Dichtel > <nicolas.dichtel@6wind.com> wrote: >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> index 837d30b5ffed..7b25f1ef3d75 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -2415,6 +2415,9 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, >> { >> struct sk_buff *skb; >> >> + if (dev->reg_state != NETREG_REGISTERED) >> + return; >> + > > Is this check correct, or placed at the correct location? The reason I > am asking is as follows. In rollback_registered_many(), dev->reg_state > is set to NETREG_UNREGISTERING for devices that will be unregistered. > When rtmsg_ifinfo_build_skb(RTM_DELLINK, ...) is called in the > following loop in rollback_registered_many, this comparison will > always be true and no DELLINK event generated. > > This change led to some applications I have not behaving as expected > due to missing DELLINK when network devices are removed. I also see no > DELLINK with ip mon link. Removing the check restores the old behavior > (DELLINK events are generated). My machine is running 3.18.18, which > includes this fix. I didn't have time to investigate right now. Will do it tomorrow. Regards, Nicolas ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface 2015-07-13 14:11 ` Kristian Evensen 2015-07-15 15:07 ` Nicolas Dichtel @ 2015-07-16 9:06 ` Nicolas Dichtel 2015-07-16 9:31 ` [PATCH linux-3.18.y] rtnl: restore notifications for deleted interfaces Nicolas Dichtel 1 sibling, 1 reply; 24+ messages in thread From: Nicolas Dichtel @ 2015-07-16 9:06 UTC (permalink / raw) To: Kristian Evensen Cc: jiri, David Miller, Network Development, j.vosburgh, vfalico, gospo Le 13/07/2015 16:11, Kristian Evensen a écrit : > Hello, > > I have a quick question about this patch. > > On Wed, May 13, 2015 at 2:19 PM, Nicolas Dichtel > <nicolas.dichtel@6wind.com> wrote: >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> index 837d30b5ffed..7b25f1ef3d75 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -2415,6 +2415,9 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, >> { >> struct sk_buff *skb; >> >> + if (dev->reg_state != NETREG_REGISTERED) >> + return; >> + > > Is this check correct, or placed at the correct location? The reason I > am asking is as follows. In rollback_registered_many(), dev->reg_state > is set to NETREG_UNREGISTERING for devices that will be unregistered. > When rtmsg_ifinfo_build_skb(RTM_DELLINK, ...) is called in the > following loop in rollback_registered_many, this comparison will > always be true and no DELLINK event generated. > > This change led to some applications I have not behaving as expected > due to missing DELLINK when network devices are removed. I also see no > DELLINK with ip mon link. Removing the check restores the old behavior > (DELLINK events are generated). My machine is running 3.18.18, which > includes this fix. Ok, I see the problem. My patch depends on commit 395eea6ccf2b ("rtnetlink: delay RTM_DELLINK notification until after ndo_uninit()") which has been introduced in 3.19 (this patch is not backported in 3.18). After 3.19, rtmsg_ifinfo_build_skb() just builds a message (an skb), it never calls rtmsg_ifinfo(). After this call, rollback_registered_many() calls rtmsg_ifinfo_send() (which also never calls rtmsg_ifinfo()). I will submit a patch for the 3.18. Regards, Nicolas ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH linux-3.18.y] rtnl: restore notifications for deleted interfaces 2015-07-16 9:06 ` Nicolas Dichtel @ 2015-07-16 9:31 ` Nicolas Dichtel 0 siblings, 0 replies; 24+ messages in thread From: Nicolas Dichtel @ 2015-07-16 9:31 UTC (permalink / raw) To: sasha.levin, stable Cc: netdev, davem, kristian.evensen, jiri, j.vosburgh, vfalico, gospo, Nicolas Dichtel The commit 984ff7a3e060 is an upstream backport. In fact, it depends on commit 395eea6ccf2b ("rtnetlink: delay RTM_DELLINK notification until after ndo_uninit()") which has not been backported in 3.18.y. Before commit 395eea6ccf2b, rollback_registered_many() uses rtmsg_ifinfo(). The call to this function is done with dev->reg_state set to NETREG_UNREGISTERING, thus testing this reg_state in rtmsg_ifinfo() is wrong. This patch partially reverts commit 984ff7a3e060. Fixes: 984ff7a3e060 ("rtnl/bond: don't send rtnl msg for unregistered iface") Reported-by: Kristian Evensen <kristian.evensen@gmail.com> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- net/core/rtnetlink.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 24d3242f0e01..c522f7a00eab 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2229,9 +2229,6 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, int err = -ENOBUFS; size_t if_info_size; - if (dev->reg_state != NETREG_REGISTERED) - return; - skb = nlmsg_new((if_info_size = if_nlmsg_size(dev, 0)), flags); if (skb == NULL) goto errout; -- 2.4.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-12 15:58 ` Jiri Pirko 2015-05-12 16:10 ` Nicolas Dichtel @ 2015-05-12 17:12 ` Andy Gospodarek 2015-05-13 12:01 ` Nicolas Dichtel 1 sibling, 1 reply; 24+ messages in thread From: Andy Gospodarek @ 2015-05-12 17:12 UTC (permalink / raw) To: Jiri Pirko; +Cc: Nicolas Dichtel, davem, netdev, j.vosburgh, vfalico On Tue, May 12, 2015 at 05:58:42PM +0200, Jiri Pirko wrote: > Tue, May 12, 2015 at 05:17:45PM CEST, nicolas.dichtel@6wind.com wrote: > >Before the patch, the command 'ip link add bond2 type bond mode 802.3ad' > >causes the kernel to send a rtnl message for the bond2 interface, with an > >ifindex 0. > > > >'ip monitor' shows: > >0: bond2: <BROADCAST,MULTICAST,MASTER> mtu 1500 state DOWN group default > > link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff > >9: bond2@NONE: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN group default > > link/ether ea:3e:1f:53:92:7b brd ff:ff:ff:ff:ff:ff > >[snip] > > > >First, nobody seems to care about NETDEV_CHANGEINFODATA (commit > >d4261e565000 says that it was put only to notify userspace). Hence, we can > >call directly rtmsg_ifinfo(). > > Please leave this notifier here. Will be needed in very near future for > LAG offloading purposes. I agree with this. It is extremely useful for a variety of reasons (not just the offload case), so please to do not remove it. > > > > >Secondly, prevent in rtmsg_ifinfo() to send notifications to userspace > >about unregistered interfaces. > > I'm not sure about this. Why caller of rtmsg_ifinfo don't take care of > it? Seems more logical to me. > > > > > >Fixes: d4261e565000 ("bonding: create netlink event when bonding option is changed") > >CC: Jiri Pirko <jiri@resnulli.us> > >Reported-by: Julien Meunier <julien.meunier@6wind.com> > >Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > >--- > > > >v3: move reg_state check in rtmsg_ifinfo() > > > >v2: check dev->reg_state instead of ifindex > > call rtmsg_ifinfo() directly in __bond_opt_set() > > > >There is about 20 occurences of "dev->reg_state .= NETREG_REGISTERED". > >I will send a patch on net-next to introduce an helper for this when net > >will be merged with this patch in net-next. > > > > drivers/net/bonding/bond_options.c | 2 +- > > include/linux/netdevice.h | 3 +-- > > net/core/rtnetlink.c | 3 +++ > > 3 files changed, 5 insertions(+), 3 deletions(-) > > > >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c > >index 4df28943d222..a2b72cc0e3e8 100644 > >--- a/drivers/net/bonding/bond_options.c > >+++ b/drivers/net/bonding/bond_options.c > >@@ -625,7 +625,7 @@ out: > > if (ret) > > bond_opt_error_interpret(bond, opt, ret, val); > > else > >- call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev); > >+ rtmsg_ifinfo(RTM_NEWLINK, bond->dev, 0, GFP_KERNEL); > > > > return ret; > > } > >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > >index 1899c74a7127..c040d48925fd 100644 > >--- a/include/linux/netdevice.h > >+++ b/include/linux/netdevice.h > >@@ -2069,8 +2069,7 @@ struct pcpu_sw_netstats { > > #define NETDEV_CHANGEUPPER 0x0015 > > #define NETDEV_RESEND_IGMP 0x0016 > > #define NETDEV_PRECHANGEMTU 0x0017 /* notify before mtu change happened */ > >-#define NETDEV_CHANGEINFODATA 0x0018 > >-#define NETDEV_BONDING_INFO 0x0019 > >+#define NETDEV_BONDING_INFO 0x0018 > > > > int register_netdevice_notifier(struct notifier_block *nb); > > int unregister_netdevice_notifier(struct notifier_block *nb); > >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > >index 837d30b5ffed..7b25f1ef3d75 100644 > >--- a/net/core/rtnetlink.c > >+++ b/net/core/rtnetlink.c > >@@ -2415,6 +2415,9 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, > > { > > struct sk_buff *skb; > > > >+ if (dev->reg_state != NETREG_REGISTERED) > >+ return; > >+ > > skb = rtmsg_ifinfo_build_skb(type, dev, change, flags); > > if (skb) > > rtmsg_ifinfo_send(skb, dev, flags); > >-- > >2.2.2 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-12 17:12 ` [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface Andy Gospodarek @ 2015-05-13 12:01 ` Nicolas Dichtel 2015-05-13 14:43 ` Andy Gospodarek 0 siblings, 1 reply; 24+ messages in thread From: Nicolas Dichtel @ 2015-05-13 12:01 UTC (permalink / raw) To: Andy Gospodarek, Jiri Pirko; +Cc: davem, netdev, j.vosburgh, vfalico Le 12/05/2015 19:12, Andy Gospodarek a écrit : > On Tue, May 12, 2015 at 05:58:42PM +0200, Jiri Pirko wrote: >> Tue, May 12, 2015 at 05:17:45PM CEST, nicolas.dichtel@6wind.com wrote: [snip] >>> First, nobody seems to care about NETDEV_CHANGEINFODATA (commit >>> d4261e565000 says that it was put only to notify userspace). Hence, we can >>> call directly rtmsg_ifinfo(). >> >> Please leave this notifier here. Will be needed in very near future for >> LAG offloading purposes. > > I agree with this. It is extremely useful for a variety of reasons (not > just the offload case), so please to do not remove it. No problem, I will keep it. For my knowledge, can you explain these "variety of reasons"? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface 2015-05-13 12:01 ` Nicolas Dichtel @ 2015-05-13 14:43 ` Andy Gospodarek 0 siblings, 0 replies; 24+ messages in thread From: Andy Gospodarek @ 2015-05-13 14:43 UTC (permalink / raw) To: Nicolas Dichtel; +Cc: Jiri Pirko, davem, netdev, j.vosburgh, vfalico On Wed, May 13, 2015 at 02:01:05PM +0200, Nicolas Dichtel wrote: > Le 12/05/2015 19:12, Andy Gospodarek a écrit : > >On Tue, May 12, 2015 at 05:58:42PM +0200, Jiri Pirko wrote: > >>Tue, May 12, 2015 at 05:17:45PM CEST, nicolas.dichtel@6wind.com wrote: > [snip] > >>>First, nobody seems to care about NETDEV_CHANGEINFODATA (commit > >>>d4261e565000 says that it was put only to notify userspace). Hence, we can > >>>call directly rtmsg_ifinfo(). > >> > >>Please leave this notifier here. Will be needed in very near future for > >>LAG offloading purposes. > > > >I agree with this. It is extremely useful for a variety of reasons (not > >just the offload case), so please to do not remove it. > No problem, I will keep it. > > For my knowledge, can you explain these "variety of reasons"? I maybe 'variety' was too strong of a word. :) The offload case has uses for it in the future as well as the case where userspace deamons who are not sending the netlink message but are monitoring netlink messages as a way to track port status. Deamons that would help broker multi-host LAGs are one such example. Though it does look like the only event that will happen when NETDEV_CHANGEINFODATA is called is an RTM_NEWLINK, it will get used more in the future, so please keep it. ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2015-07-16 9:32 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-11 14:25 [PATCH net] rtnl: don't send rtnl msg for unregistered iface Nicolas Dichtel 2015-05-11 15:37 ` Jiri Pirko 2015-05-11 16:04 ` Nicolas Dichtel 2015-05-11 16:15 ` Nicolas Dichtel 2015-05-11 17:53 ` Jiri Pirko 2015-05-11 22:15 ` Andy Gospodarek 2015-05-12 8:43 ` Nicolas Dichtel 2015-05-12 15:03 ` [PATCH net v2] rtnl/bond: " Nicolas Dichtel 2015-05-12 15:06 ` Nicolas Dichtel 2015-05-12 15:17 ` [PATCH net v3] " Nicolas Dichtel 2015-05-12 15:58 ` Jiri Pirko 2015-05-12 16:10 ` Nicolas Dichtel 2015-05-12 16:14 ` Jiri Pirko 2015-05-13 12:19 ` [PATCH net v4] " Nicolas Dichtel 2015-05-13 12:24 ` Jiri Pirko 2015-05-13 12:36 ` Nicolas Dichtel 2015-05-18 2:44 ` David Miller 2015-07-13 14:11 ` Kristian Evensen 2015-07-15 15:07 ` Nicolas Dichtel 2015-07-16 9:06 ` Nicolas Dichtel 2015-07-16 9:31 ` [PATCH linux-3.18.y] rtnl: restore notifications for deleted interfaces Nicolas Dichtel 2015-05-12 17:12 ` [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface Andy Gospodarek 2015-05-13 12:01 ` Nicolas Dichtel 2015-05-13 14:43 ` Andy Gospodarek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.