[PATCH net] rtnl: don't send rtnl msg for unregistered iface

[PATCH net] rtnl: don't send rtnl msg for unregistered iface

[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]

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

Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface

[Jiri Pirko]

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.

Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface

[Nicolas Dichtel]

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

Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface

[Nicolas Dichtel]

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

Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface

[Jiri Pirko]

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.

Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface

[Andy Gospodarek]

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.

Re: [PATCH net] rtnl: don't send rtnl msg for unregistered iface

[Nicolas Dichtel]

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.

[PATCH net v2] rtnl/bond: don't send rtnl msg for unregistered iface

[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

Re: [PATCH net v2] rtnl/bond: don't send rtnl msg for unregistered iface

[Nicolas Dichtel]

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.

[PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface

[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

Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface

[Jiri Pirko]

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
>

Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface

[Nicolas Dichtel]

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.

Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface

[Jiri Pirko]

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.

[PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface

[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

Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface

[Jiri Pirko]

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 :/

Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface

[Nicolas Dichtel]

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?

Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface

[David Miller]

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.

Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface

[Kristian Evensen]

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

Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface

[Nicolas Dichtel]

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

Re: [PATCH net v4] rtnl/bond: don't send rtnl msg for unregistered iface

[Nicolas Dichtel]

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

[PATCH linux-3.18.y] rtnl: restore notifications for deleted interfaces

[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

Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface

[Andy Gospodarek]

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
> >

Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface

[Nicolas Dichtel]

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"?

Re: [PATCH net v3] rtnl/bond: don't send rtnl msg for unregistered iface

[Andy Gospodarek]

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.