Re: [PATCH v3 net-next] net: bridge: mcast: add support for raw L2 multicast groups

[Nikolay Aleksandrov]

On Wed, 2020-10-28 at 12:54 +0200, Vladimir Oltean wrote:
> From: Nikolay Aleksandrov <nikolay@xxxxxxxxxx>
> 
> Extend the bridge multicast control and data path to configure routes
> for L2 (non-IP) multicast groups.
> 
> The uapi struct br_mdb_entry union u is extended with another variant,
> mac_addr, which does not change the structure size, and which is valid
> when the proto field is zero.
> 
> To be compatible with the forwarding code that is already in place,
> which acts as an IGMP/MLD snooping bridge with querier capabilities, we
> need to declare that for L2 MDB entries (for which there exists no such
> thing as IGMP/MLD snooping/querying), that there is always a querier.
> Otherwise, these entries would be flooded to all bridge ports and not
> just to those that are members of the L2 multicast group.
> 
> Needless to say, only permanent L2 multicast groups can be installed on
> a bridge port.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@xxxxxxxxxx>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> ---
> Changes in v3:
> - Removed some noise in the diff.
> 
> Changes in v2:
> - Removed redundant MDB_FLAGS_L2 (we are simply signalling an L2 entry
>   through proto == 0)
> - Moved mac_addr inside union dst of struct br_ip.
> - Validation that L2 multicast address is indeed multicast
> 
>  include/linux/if_bridge.h      |  1 +
>  include/uapi/linux/if_bridge.h |  1 +
>  net/bridge/br_device.c         |  2 +-
>  net/bridge/br_input.c          |  2 +-
>  net/bridge/br_mdb.c            | 24 ++++++++++++++++++++++--
>  net/bridge/br_multicast.c      |  9 +++++++--
>  net/bridge/br_private.h        | 10 ++++++++--
>  7 files changed, 41 insertions(+), 8 deletions(-)
>
[snip]
> @@ -857,6 +872,11 @@ static int br_mdb_add_group(struct net_bridge *br, struct net_bridge_port *port,
>  			return err;
>  	}
>  
> +	if (entry->state != MDB_PERMANENT && br_group_is_l2(&mp->addr)) {
> +		NL_SET_ERR_MSG_MOD(extack, "Only permanent L2 entries allowed");
> +		return -EINVAL;
> +	}
> +

Sorry, but I didn't notice this earlier. We need to check for this error before
creating the mdb group otherwise we can end up with empty groups that can't be
deleted due to errors. I.e. it must be before the br_multicast_new_group() call.

The rest looks good to me, thanks!

>  	/* host join */
>  	if (!port) {
>  		if (mp->host_joined) {
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index eae898c3cff7..98de0acb0307 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -179,7 +179,8 @@ struct net_bridge_mdb_entry *br_mdb_get(struct net_bridge *br,