Re: [PATCH] net: bridge: Allow bridge to joing multicast groups

[Nikolay Aleksandrov]

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@xxxxxxxxxxxxx>
>> ---
>>  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 ?
> 
> 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@xxxxxxxxxxxxxxxxxxx>
> 
>> 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;
>>
>