Re: [net-next v3] bridge: trigger RTM_NEWLINK when interface is modified by bridge ioctl

[David Miller]

From: Vincent Bernat 
Date: Thu, 21 Sep 2017 12:05:25 +0200

> Currently, there is a difference in netlink events received when an
> interface is modified through bridge ioctl() or through netlink. This
> patch generates additional events when an interface is added to or
> removed from a bridge via ioctl().
...
> Signed-off-by: Vincent Bernat 

Applied.

Re: [net-next v3] bridge: trigger RTM_NEWLINK when interface is modified by bridge ioctl

[David Ahern]

On 9/21/17 11:20 AM, Roopa Prabhu wrote:
> this patch seems fine...but ideally I would have assumed
> netdev_upper_dev_unlink which
> is eventually called in both paths would generate the RTN_NEWLINK
> IFF_MASTER in response
> to the NETDEV_CHANGEUPPER notifier. If we add it there now, it might
> need some more fixes
> to not generate redundant notifications for the netlink case.

Agreed and it is another pandora's box

Re: [net-next v3] bridge: trigger RTM_NEWLINK when interface is modified by bridge ioctl

[Roopa Prabhu]

On Thu, Sep 21, 2017 at 9:43 AM, David Ahern  wrote:
> On 9/21/17 4:05 AM, Vincent Bernat wrote:
>> Currently, there is a difference in netlink events received when an
>> interface is modified through bridge ioctl() or through netlink. This
>> patch generates additional events when an interface is added to or
>> removed from a bridge via ioctl().
>>
>> When adding then removing an interface from a bridge with netlink, we
>> get:
>>
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>>
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> Deleted 5: dummy1:  mtu 1500 master bridge0 
>> state UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 qdisc noqueue state 
>> UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>>
>> When using ioctl():
>>
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>>
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> Deleted 5: dummy1:  mtu 1500 master bridge0 
>> state UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 qdisc noqueue state 
>> UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>>
>> Without this patch, the last netlink notification is not sent.
>>
>> Signed-off-by: Vincent Bernat 
>> ---
>>  net/bridge/br_ioctl.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
>> index 7970f8540cbb..66cd98772051 100644
>> --- a/net/bridge/br_ioctl.c
>> +++ b/net/bridge/br_ioctl.c
>> @@ -102,6 +102,9 @@ static int add_del_if(struct net_bridge *br, int 
>> ifindex, int isadd)
>>   else
>>   ret = br_del_if(br, dev);
>>
>> + if (!ret)
>> + rtmsg_ifinfo(RTM_NEWLINK, dev, IFF_MASTER, GFP_KERNEL);
>> +
>>   return ret;
>>  }
>>
>>
>
> Agreed that this is needed for userspace to know about the master change
> when done through ioctl. The bridge code is emitting a lot of what
> appears to be redundant messages for both paths (netlink and ioctl).
>
> Reviewed-by: David Ahern 
>


this patch seems fine...but ideally I would have assumed
netdev_upper_dev_unlink which
is eventually called in both paths would generate the RTN_NEWLINK
IFF_MASTER in response
to the NETDEV_CHANGEUPPER notifier. If we add it there now, it might
need some more fixes
to not generate redundant notifications for the netlink case.

Re: [net-next v3] bridge: trigger RTM_NEWLINK when interface is modified by bridge ioctl

[David Ahern]

On 9/21/17 4:05 AM, Vincent Bernat wrote:
> Currently, there is a difference in netlink events received when an
> interface is modified through bridge ioctl() or through netlink. This
> patch generates additional events when an interface is added to or
> removed from a bridge via ioctl().
> 
> When adding then removing an interface from a bridge with netlink, we
> get:
> 
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> Deleted 5: dummy1:  mtu 1500 master bridge0 
> state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 qdisc noqueue state UNKNOWN 
> group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 
> When using ioctl():
> 
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> Deleted 5: dummy1:  mtu 1500 master bridge0 
> state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 qdisc noqueue state UNKNOWN 
> group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 
> Without this patch, the last netlink notification is not sent.
> 
> Signed-off-by: Vincent Bernat 
> ---
>  net/bridge/br_ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
> index 7970f8540cbb..66cd98772051 100644
> --- a/net/bridge/br_ioctl.c
> +++ b/net/bridge/br_ioctl.c
> @@ -102,6 +102,9 @@ static int add_del_if(struct net_bridge *br, int ifindex, 
> int isadd)
>   else
>   ret = br_del_if(br, dev);
>  
> + if (!ret)
> + rtmsg_ifinfo(RTM_NEWLINK, dev, IFF_MASTER, GFP_KERNEL);
> +
>   return ret;
>  }
>  
> 

Agreed that this is needed for userspace to know about the master change
when done through ioctl. The bridge code is emitting a lot of what
appears to be redundant messages for both paths (netlink and ioctl).

Reviewed-by: David Ahern 

Re: [net-next v3] bridge: trigger RTM_NEWLINK when interface is modified by bridge ioctl

[Vincent Bernat]

 ❦ 21 septembre 2017 08:15 -0700, Stephen Hemminger 
 :

>> Currently, there is a difference in netlink events received when an
>> interface is modified through bridge ioctl() or through netlink. This
>> patch generates additional events when an interface is added to or
>> removed from a bridge via ioctl().
>> 
>> When adding then removing an interface from a bridge with netlink, we
>> get:
>> 
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> Deleted 5: dummy1:  mtu 1500 master bridge0 
>> state UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 qdisc noqueue state 
>> UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 
>> When using ioctl():
>> 
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 
>> 5: dummy1:  mtu 1500 qdisc noqueue master 
>> bridge0 state UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 5: dummy1:  mtu 1500 master bridge0 state 
>> UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> Deleted 5: dummy1:  mtu 1500 master bridge0 
>> state UNKNOWN
>> link/ether 9e:da:60:ee:cf:c8
>> 5: dummy1:  mtu 1500 qdisc noqueue state 
>> UNKNOWN group default
>> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
>> 
>> Without this patch, the last netlink notification is not sent.
>> 
>> Signed-off-by: Vincent Bernat 
>
> This makes sense, you should probably add a Fixes: tag to help maintainers
> of long term stable kernels.
>
> Reviewed-by: Stephen Hemminger 

I wouldn't know which commit would be fixed since this is not a
regression, just a behavior difference.
-- 
Make sure special cases are truly special.
- The Elements of Programming Style (Kernighan & Plauger)

Re: [net-next v3] bridge: trigger RTM_NEWLINK when interface is modified by bridge ioctl

[Stephen Hemminger]

On Thu, 21 Sep 2017 12:05:25 +0200
Vincent Bernat  wrote:

> Currently, there is a difference in netlink events received when an
> interface is modified through bridge ioctl() or through netlink. This
> patch generates additional events when an interface is added to or
> removed from a bridge via ioctl().
> 
> When adding then removing an interface from a bridge with netlink, we
> get:
> 
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> Deleted 5: dummy1:  mtu 1500 master bridge0 
> state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 qdisc noqueue state UNKNOWN 
> group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 
> When using ioctl():
> 
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 
> 5: dummy1:  mtu 1500 qdisc noqueue master 
> bridge0 state UNKNOWN group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> Deleted 5: dummy1:  mtu 1500 master bridge0 
> state UNKNOWN
> link/ether 9e:da:60:ee:cf:c8
> 5: dummy1:  mtu 1500 qdisc noqueue state UNKNOWN 
> group default
> link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
> 
> Without this patch, the last netlink notification is not sent.
> 
> Signed-off-by: Vincent Bernat 

This makes sense, you should probably add a Fixes: tag to help maintainers
of long term stable kernels.

Reviewed-by: Stephen Hemminger 

[net-next v3] bridge: trigger RTM_NEWLINK when interface is modified by bridge ioctl

[Vincent Bernat]

Currently, there is a difference in netlink events received when an
interface is modified through bridge ioctl() or through netlink. This
patch generates additional events when an interface is added to or
removed from a bridge via ioctl().

When adding then removing an interface from a bridge with netlink, we
get:

5: dummy1:  mtu 1500 qdisc noqueue master bridge0 
state UNKNOWN group default
link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
link/ether 9e:da:60:ee:cf:c8
5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
link/ether 9e:da:60:ee:cf:c8
5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
link/ether 9e:da:60:ee:cf:c8
5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
link/ether 9e:da:60:ee:cf:c8
5: dummy1:  mtu 1500 qdisc noqueue master bridge0 
state UNKNOWN group default
link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff

5: dummy1:  mtu 1500 qdisc noqueue master bridge0 
state UNKNOWN group default
link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
link/ether 9e:da:60:ee:cf:c8
Deleted 5: dummy1:  mtu 1500 master bridge0 state 
UNKNOWN
link/ether 9e:da:60:ee:cf:c8
5: dummy1:  mtu 1500 qdisc noqueue state UNKNOWN 
group default
link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff

When using ioctl():

5: dummy1:  mtu 1500 qdisc noqueue master bridge0 
state UNKNOWN group default
link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
link/ether 9e:da:60:ee:cf:c8
5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
link/ether 9e:da:60:ee:cf:c8
5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
link/ether 9e:da:60:ee:cf:c8
5: dummy1:  mtu 1500 qdisc noqueue master bridge0 
state UNKNOWN group default
link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff

5: dummy1:  mtu 1500 qdisc noqueue master bridge0 
state UNKNOWN group default
link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff
5: dummy1:  mtu 1500 master bridge0 state UNKNOWN
link/ether 9e:da:60:ee:cf:c8
Deleted 5: dummy1:  mtu 1500 master bridge0 state 
UNKNOWN
link/ether 9e:da:60:ee:cf:c8
5: dummy1:  mtu 1500 qdisc noqueue state UNKNOWN 
group default
link/ether 9e:da:60:ee:cf:c8 brd ff:ff:ff:ff:ff:ff

Without this patch, the last netlink notification is not sent.

Signed-off-by: Vincent Bernat 
---
 net/bridge/br_ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index 7970f8540cbb..66cd98772051 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -102,6 +102,9 @@ static int add_del_if(struct net_bridge *br, int ifindex, 
int isadd)
else
ret = br_del_if(br, dev);
 
+   if (!ret)
+   rtmsg_ifinfo(RTM_NEWLINK, dev, IFF_MASTER, GFP_KERNEL);
+
return ret;
 }
 
-- 
2.14.1