David Ahern
2017-Sep-21 16:43 UTC
[Bridge] [net-next v3] bridge: trigger RTM_NEWLINK when interface is modified by bridge ioctl
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: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > Deleted 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > Deleted 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN > link/ether 9e:da:60:ee:cf:c8 > 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> 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 <vincent at bernat.im> > --- > 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 <dsahern at gmail.com>
Roopa Prabhu
2017-Sep-21 17:20 UTC
[Bridge] [net-next v3] bridge: trigger RTM_NEWLINK when interface is modified by bridge ioctl
On Thu, Sep 21, 2017 at 9:43 AM, David Ahern <dsahern at gmail.com> 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: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> Deleted 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> 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: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> Deleted 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN >> link/ether 9e:da:60:ee:cf:c8 >> 5: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> 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 <vincent at bernat.im> >> --- >> 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 <dsahern at gmail.com> >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.