Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Ido Schimmel <idosch@mellanox.com>]

On Thu, Jan 17, 2019 at 11:17:57AM -0800, Florian Fainelli wrote:
> On 1/17/19 6:05 AM, Ido Schimmel wrote:
> > On Wed, Jan 16, 2019 at 12:00:57PM -0800, Florian Fainelli wrote:
> >> In order for bridge port members to get a chance to implement unicast
> >> and multicast address filtering correctly, which would matter for e.g:
> >> switch network devices, synchronize the UC and MC lists down to the
> >> individual bridge port members using switchdev HOST_MDB objects such
> >> that this does not impact drivers that already have a ndo_set_rx_mode()
> >> operation which likely already operate in promiscuous mode.
> >>
> >> When the bridge has multicast snooping enabled, proper HOST_MDB
> >> notifications will be sent through br_mdb_notify() already.
> > 
> > I don't understand the change. HOST_MDB is used to notify underlying
> > drivers about MDB entries that should be configured to locally receive
> > packets. This is triggered by the transmission of an IGMP report through
> > the bridge, for example.
> > 
> > It seems that you're trying to overload HOST_MDB with multicast address
> > filtering on bridge ports?
> 
> I don't really think this is an abuse of HOST_MDB, since in case the
> bridge has multicast_snooping enabled, and there is e.g: a multicast
> application bound to the bridge master device, we would get those
> notifications through HOST_MDB already. This is the same use case that I
> am addressing here, ndo_set_rx_mode() learns about local multicast
> addresses that should be programmed, which means there is a multicast
> application listening on the bridge master device itself.
> 
> The problem that I want to solve is that with Broadcom b53/bcm_sf2
> switches, we cannot easily filter/flood multicast for the CPU/management
> port.
> 
> We have per-port controls for MC/IPMC flooding, and we also have a
> separate control for CPU/management port receiving multicast. If either
> of these two bits/settings are configured, then the CPU port will always
> receive multicast, even when we should be filtering it in HW. The only
> way to perform selective reception of multicast to the CPU port is to
> program a corresponding MDB entry.
> 
> >Why are you performing this filtering?
> 
> If I do not filter, then non-bridged ports on which there is no
> multicast application bound to would be passing up multicast traffic all
> the way to the CPU port, which then has to be dropped in software. This
> is not acceptable IMHO because it is a deviation from how a standalone
> NIC supporting multicast filtering would operate.
> 
> > Shouldn't you allow all MAC addresses to ingress?
> 
> I do allow all MC addresses to ingress on the front-panel switch ports
> (while honoring the multicast_snooping setting), but we have no control
> over what the CPU/management port should be doing.
> 
> As I wrote earlier, if we flood to the CPU/management port, because
> there is at least one switch device port, in the bridge, and that bridge
> has multicast_snooping disabled, then this could break filtering for
> other, non-bridged ports. That is really not acceptable IMHO.

When multicast snooping is disabled every multicast packet should be
flooded to the CPU port. Not only addresses configured on the bridge
device's address list. Check br_handle_frame_finish(), it sets
'local_rcv' to 'true'.

> 
> The reason why I chose switchdev HOST_MDB notification here are two fold:
> 
> - this is the same use case as with multicast_snooping=1 and we target
> the CPU port within DSA to resolve that use case, so from the switch
> driver perspective, there is no difference in the context
> 
> - this does not impact network device drivers that have a
> ndo_set_rx_mode() and somehow decide to support things through that API
> since those would typically have a switchdev_port_attr_set() callback
> 
> Let me know if this is not clear.
> -- 
> Florian