From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Cc: <roopa@cumulusnetworks.com>, <davem@davemloft.net>,
<bridge@lists.linux-foundation.org>, <netdev@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <allan.nielsen@microchip.com>
Subject: Re: [PATCH] net: bridge: Allow bridge to joing multicast groups
Date: Thu, 25 Jul 2019 16:21:03 +0200 [thread overview]
Message-ID: <20190725142101.65tusauc6fzxb2yp@soft-dev3.microsemi.net> (raw)
In-Reply-To: <eef063fe-fd3a-7e02-89c2-e40728a17578@cumulusnetworks.com>
Hi Nikolay,
The 07/25/2019 16:21, Nikolay Aleksandrov wrote:
> External E-Mail
>
>
> On 25/07/2019 16:06, Nikolay Aleksandrov wrote:
> > 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@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
>
> And obviously this is for the case where you're not enabling port promisc mode (non-default).
> In general you'll only need to join the group on the bridge to receive traffic for it
> or add it as an mdb entry to forward it.
>
> > have the effect that you're describing. What do you mean there's no way ?
Thanks for the explanation.
There are few things that are not 100% clear to me and maybe you can
explain them, not to go totally in the wrong direction. Currently I am
writing a network driver on which I added switchdev support. Then I was
looking for a way to configure the network driver to copy link layer
multicast address to the CPU port.
If I am using bridge mdb I can do it only for IP multicast addreses,
but how should I do it if I want non IP frames with link layer multicast
address to be copy to CPU? For example: all frames with multicast
address '01-21-6C-00-00-01' to be copy to CPU. What is the user space
command for that?
> >
> > 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@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;
> >>
> >
>
--
/Horatiu
next prev parent reply other threads:[~2019-07-25 14:21 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-25 11:44 [PATCH] net: bridge: Allow bridge to joing multicast groups Horatiu Vultur
2019-07-25 13:06 ` Nikolay Aleksandrov
2019-07-25 13:21 ` Nikolay Aleksandrov
2019-07-25 14:21 ` Horatiu Vultur [this message]
2019-07-26 8:41 ` Nikolay Aleksandrov
2019-07-26 9:26 ` Nikolay Aleksandrov
2019-07-26 12:02 ` Horatiu Vultur
2019-07-26 12:31 ` Nikolay Aleksandrov
2019-07-29 12:14 ` Allan W. Nielsen
2019-07-29 12:22 ` Nikolay Aleksandrov
2019-07-29 12:50 ` Nikolay Aleksandrov
2019-07-29 13:14 ` Allan W. Nielsen
2019-07-29 13:42 ` Nikolay Aleksandrov
2019-07-29 13:52 ` Allan W. Nielsen
2019-07-29 14:21 ` Nikolay Aleksandrov
2019-07-29 14:35 ` Allan W. Nielsen
2019-07-29 17:51 ` Ido Schimmel
2019-07-30 6:27 ` Allan W. Nielsen
2019-07-30 7:06 ` Ido Schimmel
2019-07-30 8:30 ` Allan W. Nielsen
2019-07-30 8:58 ` Nikolay Aleksandrov
2019-07-30 9:21 ` Allan W. Nielsen
2019-07-30 9:55 ` Nikolay Aleksandrov
2019-07-30 11:23 ` Allan W. Nielsen
2019-07-30 10:04 ` Ido Schimmel
2019-07-30 12:19 ` Allan W. Nielsen
2019-07-30 14:34 ` Andrew Lunn
2019-07-30 19:00 ` Allan W. Nielsen
2019-07-31 3:31 ` Andrew Lunn
2019-07-31 8:01 ` Allan W. Nielsen
2019-07-31 13:45 ` Andrew Lunn
2019-07-31 19:38 ` Allan W. Nielsen
2019-08-01 14:22 ` Allan W. Nielsen
2019-07-26 13:46 ` Andrew Lunn
2019-07-26 19:50 ` Allan W. Nielsen
2019-07-27 3:02 ` Andrew Lunn
2019-07-28 19:15 ` Allan W. Nielsen
2019-07-28 23:07 ` Andrew Lunn
2019-07-29 6:09 ` Ido Schimmel
2019-07-29 12:43 ` Allan W. Nielsen
2019-08-01 19:17 ` Vivien Didelot
2019-08-01 19:48 ` Horatiu Vultur
2019-08-01 20:08 ` Vivien Didelot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190725142101.65tusauc6fzxb2yp@soft-dev3.microsemi.net \
--to=horatiu.vultur@microchip.com \
--cc=allan.nielsen@microchip.com \
--cc=bridge@lists.linux-foundation.org \
--cc=davem@davemloft.net \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nikolay@cumulusnetworks.com \
--cc=roopa@cumulusnetworks.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).