Horatiu Vultur
2019-Jul-25 11:44 UTC
[Bridge] [PATCH] net: bridge: Allow bridge to joing multicast groups
There is no way to configure the bridge, to receive only specific link layer multicast addresses. From the description of the command 'bridge fdb append' is supposed to do that, but there was no way to notify the network driver that the bridge joined a group, because LLADDR was added to the unicast netdev_hw_addr_list. Therefore update fdb_add_entry to check if the NLM_F_APPEND flag is set and if the source is NULL, which represent the bridge itself. Then add address to multicast netdev_hw_addr_list for each bridge interfaces. And then the .ndo_set_rx_mode function on the driver is called. To notify the driver that the list of multicast mac addresses changed. Signed-off-by: Horatiu Vultur <horatiu.vultur at microchip.com> --- net/bridge/br_fdb.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index b1d3248..d93746d 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -175,6 +175,29 @@ static void fdb_add_hw_addr(struct net_bridge *br, const unsigned char *addr) } } +static void fdb_add_hw_maddr(struct net_bridge *br, const unsigned char *addr) +{ + int err; + struct net_bridge_port *p; + + ASSERT_RTNL(); + + list_for_each_entry(p, &br->port_list, list) { + if (!br_promisc_port(p)) { + err = dev_mc_add(p->dev, addr); + if (err) + goto undo; + } + } + + return; +undo: + list_for_each_entry_continue_reverse(p, &br->port_list, list) { + if (!br_promisc_port(p)) + dev_mc_del(p->dev, addr); + } +} + /* When a static FDB entry is deleted, the HW address from that entry is * also removed from the bridge private HW address list and updates all * the ports with needed information. @@ -192,13 +215,27 @@ static void fdb_del_hw_addr(struct net_bridge *br, const unsigned char *addr) } } +static void fdb_del_hw_maddr(struct net_bridge *br, const unsigned char *addr) +{ + struct net_bridge_port *p; + + ASSERT_RTNL(); + + list_for_each_entry(p, &br->port_list, list) { + if (!br_promisc_port(p)) + dev_mc_del(p->dev, addr); + } +} + static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f, bool swdev_notify) { trace_fdb_delete(br, f); - if (f->is_static) + if (f->is_static) { fdb_del_hw_addr(br, f->key.addr.addr); + fdb_del_hw_maddr(br, f->key.addr.addr); + } hlist_del_init_rcu(&f->fdb_node); rhashtable_remove_fast(&br->fdb_hash_tbl, &f->rhnode, @@ -843,13 +880,19 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, fdb->is_local = 1; if (!fdb->is_static) { fdb->is_static = 1; - fdb_add_hw_addr(br, addr); + if (flags & NLM_F_APPEND && !source) + fdb_add_hw_maddr(br, addr); + else + fdb_add_hw_addr(br, addr); } } else if (state & NUD_NOARP) { fdb->is_local = 0; if (!fdb->is_static) { fdb->is_static = 1; - fdb_add_hw_addr(br, addr); + if (flags & NLM_F_APPEND && !source) + fdb_add_hw_maddr(br, addr); + else + fdb_add_hw_addr(br, addr); } } else { fdb->is_local = 0; -- 2.7.4
Nikolay Aleksandrov
2019-Jul-25 13:06 UTC
[Bridge] [PATCH] net: bridge: Allow bridge to joing multicast groups
On 25/07/2019 14:44, Horatiu Vultur wrote:> There is no way to configure the bridge, to receive only specific link > layer multicast addresses. From the description of the command 'bridge > fdb append' is supposed to do that, but there was no way to notify the > network driver that the bridge joined a group, because LLADDR was added > to the unicast netdev_hw_addr_list. > > Therefore update fdb_add_entry to check if the NLM_F_APPEND flag is set > and if the source is NULL, which represent the bridge itself. Then add > address to multicast netdev_hw_addr_list for each bridge interfaces. > And then the .ndo_set_rx_mode function on the driver is called. To notify > the driver that the list of multicast mac addresses changed. > > Signed-off-by: Horatiu Vultur <horatiu.vultur at microchip.com> > --- > net/bridge/br_fdb.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 46 insertions(+), 3 deletions(-) >Hi, I'm sorry but this patch is wrong on many levels, some notes below. In general NLM_F_APPEND is only used in vxlan, the bridge does not handle that flag at all. FDB is only for *unicast*, nothing is joined and no multicast should be used with fdbs. MDB is used for multicast handling, but both of these are used for forwarding. The reason the static fdbs are added to the filter is for non-promisc ports, so they can receive traffic destined for these FDBs for forwarding. If you'd like to join any multicast group please use the standard way, if you'd like to join it only on a specific port - join it only on that port (or ports) and the bridge and you'll have the effect that you're describing. What do you mean there's no way ? In addition you're allowing a mix of mcast functions to be called with unicast addresses and vice versa, it is not that big of a deal because the kernel will simply return an error but still makes no sense. Nacked-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index b1d3248..d93746d 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -175,6 +175,29 @@ static void fdb_add_hw_addr(struct net_bridge *br, const unsigned char *addr) > } > } > > +static void fdb_add_hw_maddr(struct net_bridge *br, const unsigned char *addr) > +{ > + int err; > + struct net_bridge_port *p; > + > + ASSERT_RTNL(); > + > + list_for_each_entry(p, &br->port_list, list) { > + if (!br_promisc_port(p)) { > + err = dev_mc_add(p->dev, addr); > + if (err) > + goto undo; > + } > + } > + > + return; > +undo: > + list_for_each_entry_continue_reverse(p, &br->port_list, list) { > + if (!br_promisc_port(p)) > + dev_mc_del(p->dev, addr); > + } > +} > + > /* When a static FDB entry is deleted, the HW address from that entry is > * also removed from the bridge private HW address list and updates all > * the ports with needed information. > @@ -192,13 +215,27 @@ static void fdb_del_hw_addr(struct net_bridge *br, const unsigned char *addr) > } > } > > +static void fdb_del_hw_maddr(struct net_bridge *br, const unsigned char *addr) > +{ > + struct net_bridge_port *p; > + > + ASSERT_RTNL(); > + > + list_for_each_entry(p, &br->port_list, list) { > + if (!br_promisc_port(p)) > + dev_mc_del(p->dev, addr); > + } > +} > + > static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f, > bool swdev_notify) > { > trace_fdb_delete(br, f); > > - if (f->is_static) > + if (f->is_static) { > fdb_del_hw_addr(br, f->key.addr.addr); > + fdb_del_hw_maddr(br, f->key.addr.addr);Walking over all ports again for each static delete is a no-go.> + } > > hlist_del_init_rcu(&f->fdb_node); > rhashtable_remove_fast(&br->fdb_hash_tbl, &f->rhnode, > @@ -843,13 +880,19 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, > fdb->is_local = 1; > if (!fdb->is_static) { > fdb->is_static = 1; > - fdb_add_hw_addr(br, addr); > + if (flags & NLM_F_APPEND && !source) > + fdb_add_hw_maddr(br, addr); > + else > + fdb_add_hw_addr(br, addr); > } > } else if (state & NUD_NOARP) { > fdb->is_local = 0; > if (!fdb->is_static) { > fdb->is_static = 1; > - fdb_add_hw_addr(br, addr); > + if (flags & NLM_F_APPEND && !source) > + fdb_add_hw_maddr(br, addr); > + else > + fdb_add_hw_addr(br, addr); > } > } else { > fdb->is_local = 0; >
Vivien Didelot
2019-Aug-01 19:17 UTC
[Bridge] [PATCH] net: bridge: Allow bridge to joing multicast groups
Hi Horatiu, On Thu, 25 Jul 2019 13:44:04 +0200, Horatiu Vultur <horatiu.vultur at microchip.com> wrote:> There is no way to configure the bridge, to receive only specific link > layer multicast addresses. From the description of the command 'bridge > fdb append' is supposed to do that, but there was no way to notify the > network driver that the bridge joined a group, because LLADDR was added > to the unicast netdev_hw_addr_list. > > Therefore update fdb_add_entry to check if the NLM_F_APPEND flag is set > and if the source is NULL, which represent the bridge itself. Then add > address to multicast netdev_hw_addr_list for each bridge interfaces. > And then the .ndo_set_rx_mode function on the driver is called. To notify > the driver that the list of multicast mac addresses changed. > > Signed-off-by: Horatiu Vultur <horatiu.vultur at microchip.com> > --- > net/bridge/br_fdb.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 46 insertions(+), 3 deletions(-) > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index b1d3248..d93746d 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -175,6 +175,29 @@ static void fdb_add_hw_addr(struct net_bridge *br, const unsigned char *addr) > } > } > > +static void fdb_add_hw_maddr(struct net_bridge *br, const unsigned char *addr) > +{ > + int err; > + struct net_bridge_port *p; > + > + ASSERT_RTNL(); > + > + list_for_each_entry(p, &br->port_list, list) { > + if (!br_promisc_port(p)) { > + err = dev_mc_add(p->dev, addr); > + if (err) > + goto undo; > + } > + } > + > + return; > +undo: > + list_for_each_entry_continue_reverse(p, &br->port_list, list) { > + if (!br_promisc_port(p)) > + dev_mc_del(p->dev, addr); > + } > +} > + > /* When a static FDB entry is deleted, the HW address from that entry is > * also removed from the bridge private HW address list and updates all > * the ports with needed information. > @@ -192,13 +215,27 @@ static void fdb_del_hw_addr(struct net_bridge *br, const unsigned char *addr) > } > } > > +static void fdb_del_hw_maddr(struct net_bridge *br, const unsigned char *addr) > +{ > + struct net_bridge_port *p; > + > + ASSERT_RTNL(); > + > + list_for_each_entry(p, &br->port_list, list) { > + if (!br_promisc_port(p)) > + dev_mc_del(p->dev, addr); > + } > +} > + > static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f, > bool swdev_notify) > { > trace_fdb_delete(br, f); > > - if (f->is_static) > + if (f->is_static) { > fdb_del_hw_addr(br, f->key.addr.addr); > + fdb_del_hw_maddr(br, f->key.addr.addr); > + } > > hlist_del_init_rcu(&f->fdb_node); > rhashtable_remove_fast(&br->fdb_hash_tbl, &f->rhnode, > @@ -843,13 +880,19 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, > fdb->is_local = 1; > if (!fdb->is_static) { > fdb->is_static = 1; > - fdb_add_hw_addr(br, addr); > + if (flags & NLM_F_APPEND && !source) > + fdb_add_hw_maddr(br, addr); > + else > + fdb_add_hw_addr(br, addr); > } > } else if (state & NUD_NOARP) { > fdb->is_local = 0; > if (!fdb->is_static) { > fdb->is_static = 1; > - fdb_add_hw_addr(br, addr); > + if (flags & NLM_F_APPEND && !source) > + fdb_add_hw_maddr(br, addr); > + else > + fdb_add_hw_addr(br, addr); > } > } else { > fdb->is_local = 0; > -- > 2.7.4 >I'm a bit late in the conversation. Isn't this what you want? ip address add <multicast IPv4 address> dev br0 autojoin Thanks, Vivien