Linux Ethernet Bridging - Jul 2019 - [Bridge] [PATCH] net: bridge: Allow bridge to joing multicast groups

Hi Allan,
On 29/07/2019 15:14, Allan W. Nielsen wrote:
> Hi Nikolay,
> 
> First of all, as mentioned further down in this thread, I realized that our
> implementation of the multicast floodmasks does not align with the existing
SW
> implementation. We will change this, such that all multicast packets goes
to the
> SW bridge.
> 
> This changes things a bit, not that much.
> 
> I actually think you summarized the issue we have (after changing to
multicast
> flood-masks) right here:
> 
> The 07/26/2019 12:26, Nikolay Aleksandrov wrote:
>>>> Actually you mentioned non-IP traffic, so the querier stuff is
not a problem. This
>>>> traffic will always be flooded by the bridge (and also a copy
will be locally sent up).
>>>> Thus only the flooding may need to be controlled.
> 
> This seems to be exactly what we need.
> 
> Assuming we have a SW bridge (br0) with 4 slave interfaces (eth0-3). We use
this
> on a network where we want to limit the flooding of frames with dmac
> 01:21:6C:00:00:01 (which is non IP traffic) to eth0 and eth1.
> 
> One way of doing this could potentially be to support the following
command:
> 
> bridge fdb add    01:21:6C:00:00:01 port eth0
> bridge fdb append 01:21:6C:00:00:01 port eth1
> 
> On 25/07/2019 16:06, Nikolay Aleksandrov wrote:
>>>>>>>>  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.
> This is true, and this should have been addressed in the patch, we were too
> focused on setting up the offload patch in the driver, and forgot to do the
SW
> implementation.
> 
> Do you see any issues in supporting this flag, and updating the SW
> forwarding in br_handle_frame_finish such that it can support/allow a FDB
entry
> to be a multicast?
> 
Yes, all of the multicast code is handled differently, it doesn't go through
the fdb
lookup or code at all. I don't see how you'll do a lookup in the fdb
table with a
multicast mac address, take a look at br_handle_frame_finish() and you'll
notice
that when a multicast dmac is detected then we use the bridge mcast code for
lookups
and forwarding. If you're trying to achieve Rx only on the bridge of these
then
why not just use Ido's tc suggestion or even the ip maddr add offload for
each port ?

If you add a multicast mac in the fdb (currently allowed, but has no effect) and
you
use dev_mc_add() as suggested that'd just be a hack to pass it down and it
is already
possible to achieve via other methods, no need to go through the bridge.
> /Allan
>

On 29/07/2019 15:22, Nikolay Aleksandrov wrote:
> Hi Allan,
> On 29/07/2019 15:14, Allan W. Nielsen wrote:
>> Hi Nikolay,
>>
>> First of all, as mentioned further down in this thread, I realized that
our
>> implementation of the multicast floodmasks does not align with the
existing SW
>> implementation. We will change this, such that all multicast packets
goes to the
>> SW bridge.
>>
>> This changes things a bit, not that much.
>>
>> I actually think you summarized the issue we have (after changing to
multicast
>> flood-masks) right here:
>>
>> The 07/26/2019 12:26, Nikolay Aleksandrov wrote:
>>>>> Actually you mentioned non-IP traffic, so the querier stuff
is not a problem. This
>>>>> traffic will always be flooded by the bridge (and also a
copy will be locally sent up).
>>>>> Thus only the flooding may need to be controlled.
>>
>> This seems to be exactly what we need.
>>
>> Assuming we have a SW bridge (br0) with 4 slave interfaces (eth0-3). We
use this
>> on a network where we want to limit the flooding of frames with dmac
>> 01:21:6C:00:00:01 (which is non IP traffic) to eth0 and eth1.
>>
>> One way of doing this could potentially be to support the following
command:
>>
>> bridge fdb add    01:21:6C:00:00:01 port eth0
>> bridge fdb append 01:21:6C:00:00:01 port eth1
>>
And the fdbs become linked lists ? So we'll increase the complexity for
something
that is already supported by ACLs (e.g. tc) and also bridge per-port multicast
flood flag ?

I'm sorry but that doesn't sound good to me for a case which is very
rare and
there are existing ways to solve without incurring performance hits or
increasing
code complexity.
>> On 25/07/2019 16:06, Nikolay Aleksandrov wrote:
>>>>>>>>>  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.
>> This is true, and this should have been addressed in the patch, we were
too
>> focused on setting up the offload patch in the driver, and forgot to do
the SW
>> implementation.
>>
>> Do you see any issues in supporting this flag, and updating the SW
>> forwarding in br_handle_frame_finish such that it can support/allow a
FDB entry
>> to be a multicast?
>>
> 
> Yes, all of the multicast code is handled differently, it doesn't go
through the fdb
> lookup or code at all. I don't see how you'll do a lookup in the
fdb table with a
> multicast mac address, take a look at br_handle_frame_finish() and
you'll notice
> that when a multicast dmac is detected then we use the bridge mcast code
for lookups
> and forwarding. If you're trying to achieve Rx only on the bridge of
these then
> why not just use Ido's tc suggestion or even the ip maddr add offload
for each port ?
> 
> If you add a multicast mac in the fdb (currently allowed, but has no
effect) and you
> use dev_mc_add() as suggested that'd just be a hack to pass it down and
it is already
> possible to achieve via other methods, no need to go through the bridge.
> 
>> /Allan
>>
>

The 07/29/2019 15:22, Nikolay Aleksandrov wrote:
> Yes, all of the multicast code is handled differently, it doesn't go
through the fdb
> lookup or code at all. I don't see how you'll do a lookup in the
fdb table with a
> multicast mac address, take a look at br_handle_frame_finish() and
you'll notice
> that when a multicast dmac is detected then we use the bridge mcast code
for lookups
> and forwarding.
Here is my thinking (needs much more elaboration, which will come if we do a
patch to test it out):


In br_pkt_type

Rename BR_PKT_MULTICAST to BR_PKT_MULTICAST_IP
Add a new type called BR_PKT_MULTICAST_L2

In br_handle_frame_finish

	if (is_multicast_ether_addr(dest)) {
		/* by definition the broadcast is also a multicast address */
		if (is_broadcast_ether_addr(dest)) {
			pkt_type = BR_PKT_BROADCAST;
			local_rcv = true;
		} else {
			pkt_type = BR_PKT_MULTICAST;
			if (br_multicast_rcv(br, p, skb, vid))
				goto drop;
		}
	}

Change the code above to detect if it is a BR_PKT_MULTICAST_IP or a
BR_PKT_MULTICAST_L2


In this section:

switch (pkt_type) {
....
}

if (dst) {
} else {
}

Add awareness to the BR_PKT_MULTICAST_L2 type, and allow it do forwarding
according to the static entry if it is there.
> If you're trying to achieve Rx only on the bridge of these then
> why not just use Ido's tc suggestion or even the ip maddr add offload
for each port ?
> 
> If you add a multicast mac in the fdb (currently allowed, but has no
effect) and you
> use dev_mc_add() as suggested that'd just be a hack to pass it down and
it is already
> possible to achieve via other methods, no need to go through the bridge.
Well, I wanted the SW bridge implementation to behave the same with an without
HW offload.

And also, I believe that is conceptually belongs to the MAC tables.

/Allan

On 29/07/2019 16:14, Allan W. Nielsen wrote:
> The 07/29/2019 15:22, Nikolay Aleksandrov wrote:
>> Yes, all of the multicast code is handled differently, it doesn't
go through the fdb
>> lookup or code at all. I don't see how you'll do a lookup in
the fdb table with a
>> multicast mac address, take a look at br_handle_frame_finish() and
you'll notice
>> that when a multicast dmac is detected then we use the bridge mcast
code for lookups
>> and forwarding.
> 
> Here is my thinking (needs much more elaboration, which will come if we do
a
> patch to test it out):
> 
> 
> In br_pkt_type
> 
> Rename BR_PKT_MULTICAST to BR_PKT_MULTICAST_IP
> Add a new type called BR_PKT_MULTICAST_L2
> 
> In br_handle_frame_finish
> 
> 	if (is_multicast_ether_addr(dest)) {
> 		/* by definition the broadcast is also a multicast address */
> 		if (is_broadcast_ether_addr(dest)) {
> 			pkt_type = BR_PKT_BROADCAST;
> 			local_rcv = true;
> 		} else {
> 			pkt_type = BR_PKT_MULTICAST;
> 			if (br_multicast_rcv(br, p, skb, vid))
> 				goto drop;
> 		}
> 	}
> 
> Change the code above to detect if it is a BR_PKT_MULTICAST_IP or a
> BR_PKT_MULTICAST_L2
> 
> 
> In this section:
> 
> switch (pkt_type) {
> ....
> }
> 
> if (dst) {
> } else {
> }
> 
> Add awareness to the BR_PKT_MULTICAST_L2 type, and allow it do forwarding
> according to the static entry if it is there.
> 
All of these add overhead to everyone - standard unicast and multicast
forwarding.
Also as mentioned in my second email the fdb would need changes which would
further
increase that overhead and also the code complexity for everyone for a use-case
which
is very rare when there are alternatives today which avoid all of that. Offload
tc rules
and add a rule to allow that traffic on ingress for particular ports, then
either use
the bridge multicast flood flag or tc again to restrict flood to other egress
ports.

Alternatively use ip maddr add which calls dev_mc_add() which is what the patch
was
trying to do in the first place to allow the Rx of these packets and then
control
the flooding via one of the above methods.

Alternatively offload nft and use that to control the traffic.

I'm sure there are more ways that I'm missing. :)

If you find a way to achieve this without incurring a performance hit or
significant
code complexity increase, and without breaking current use-cases (e.g.
unexpected default
forwarding behaviour changes) then please send a patch and we can discuss it
further with
all details present. People have provided enough alternatives which avoid all of
the
problems.
>> If you're trying to achieve Rx only on the bridge of these then
>> why not just use Ido's tc suggestion or even the ip maddr add
offload for each port ?
>>
>> If you add a multicast mac in the fdb (currently allowed, but has no
effect) and you
>> use dev_mc_add() as suggested that'd just be a hack to pass it down
and it is already
>> possible to achieve via other methods, no need to go through the
bridge.
> 
> Well, I wanted the SW bridge implementation to behave the same with an
without
> HW offload.
> 
> And also, I believe that is conceptually belongs to the MAC tables.
> 
> /Allan
>