| Message ID | 1431354346-4476-1-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
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 -- 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
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 -- 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
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. -- 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 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. -- 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
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. -- 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/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; }
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(+)