Message-ID: <98ac64d9b048278d2296f5b0ff3320c70ea13c72.camel@nvidia.com>
Date:   Wed, 21 Oct 2020 09:17:07 +0000
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bridge@...ts.linux-foundation.org" 
        <bridge@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Roopa Prabhu <roopa@...dia.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "vladimir.oltean@....com" <vladimir.oltean@....com>,
        "kuba@...nel.org" <kuba@...nel.org>
CC:     "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "jiri@...lanox.com" <jiri@...lanox.com>,
        "idosch@...sch.org" <idosch@...sch.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>
Subject: Re: [RFC PATCH] net: bridge: multicast: add support for L2 entries

On Sat, 2020-10-17 at 21:41 +0300, Vladimir Oltean wrote:
> From: Nikolay Aleksandrov <nikolay@...dia.com>
> 
> 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,
> interpretation, mac_addr, which does not change the structure size, and
> which is valid when the MDB_FLAGS_L2 flag is found set.
> 
> 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@...dia.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> This patch is adapted from the version that Nikolay posted here:
> https://lore.kernel.org/netdev/20200708090454.zvb6o7jr2woirw3i@skbuf/
> 
> There, he marked the patch as "unfinished". I haven't made any major
> modifications to it, but I've tested it and it appears to work ok,
> including with offloading. Hence, I would appreciate some tips regarding
> things that might be missing.
> 

Hi,
I almost missed this one, thank you for fixing it up. I was wondering if we
can move br_ip's mac_addr in the "dst" union to save some space and reduce
ops when matching, since we're also matching on the protocol field. In general
do we need the ->l2 field at all, can we use proto == 0 ? In order to make it
more readable it can be in a helper with a descriptive name so we don't wonder
what proto == 0 meant later. A few more minor comments below.

>  include/linux/if_bridge.h      |  1 +
>  include/uapi/linux/if_bridge.h |  2 ++
>  net/bridge/br_device.c         |  2 +-
>  net/bridge/br_input.c          |  2 +-
>  net/bridge/br_mdb.c            | 24 ++++++++++++++++++++----
>  net/bridge/br_multicast.c      | 12 ++++++++++--
>  net/bridge/br_private.h        |  7 +++++--
>  7 files changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 556caed00258..b135ad714383 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -26,6 +26,7 @@ struct br_ip {
>  		struct in6_addr ip6;
>  #endif
>  	} dst;
> +	unsigned char	mac_addr[ETH_ALEN];
>  	__be16		proto;
>  	__u16           vid;
>  };
> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index 4c687686aa8f..a25f6f9aa8c3 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -520,12 +520,14 @@ struct br_mdb_entry {
>  #define MDB_FLAGS_FAST_LEAVE	(1 << 1)
>  #define MDB_FLAGS_STAR_EXCL	(1 << 2)
>  #define MDB_FLAGS_BLOCKED	(1 << 3)
> +#define MDB_FLAGS_L2		(1 << 5)

I think this should be 4.

>  	__u8 flags;
>  	__u16 vid;
>  	struct {
>  		union {
>  			__be32	ip4;
>  			struct in6_addr ip6;
> +			unsigned char mac_addr[ETH_ALEN];
>  		} u;
>  		__be16		proto;
>  	} addr;
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 6f742fee874a..06c28753b911 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -93,7 +93,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  		mdst = br_mdb_get(br, skb, vid);
>  		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> -		    br_multicast_querier_exists(br, eth_hdr(skb)))
> +		    br_multicast_querier_exists(br, eth_hdr(skb), mdst))
>  			br_multicast_flood(mdst, skb, false, true);
>  		else
>  			br_flood(br, skb, BR_PKT_MULTICAST, false, true);
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 59a318b9f646..d31b5c18c6a1 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -134,7 +134,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  	case BR_PKT_MULTICAST:
>  		mdst = br_mdb_get(br, skb, vid);
>  		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
> -		    br_multicast_querier_exists(br, eth_hdr(skb))) {
> +		    br_multicast_querier_exists(br, eth_hdr(skb), mdst)) {
>  			if ((mdst && mdst->host_joined) ||
>  			    br_multicast_is_router(br)) {
>  				local_rcv = true;
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index e15bab19a012..4decf3eb7001 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -66,6 +66,8 @@ static void __mdb_entry_fill_flags(struct br_mdb_entry *e, unsigned char flags)
>  		e->flags |= MDB_FLAGS_STAR_EXCL;
>  	if (flags & MDB_PG_FLAGS_BLOCKED)
>  		e->flags |= MDB_FLAGS_BLOCKED;
> +	if (flags & MDB_PG_FLAGS_L2)
> +		e->flags |= MDB_FLAGS_L2;
>  }
>  
>  static void __mdb_entry_to_br_ip(struct br_mdb_entry *entry, struct br_ip *ip,
> @@ -87,6 +89,8 @@ static void __mdb_entry_to_br_ip(struct br_mdb_entry *entry, struct br_ip *ip,
>  			ip->src.ip6 = nla_get_in6_addr(mdb_attrs[MDBE_ATTR_SOURCE]);
>  		break;
>  #endif
> +	default:
> +		ether_addr_copy(ip->mac_addr, entry->addr.u.mac_addr);
>  	}
>  
>  }
> @@ -174,9 +178,11 @@ static int __mdb_fill_info(struct sk_buff *skb,
>  	if (mp->addr.proto == htons(ETH_P_IP))
>  		e.addr.u.ip4 = mp->addr.dst.ip4;
>  #if IS_ENABLED(CONFIG_IPV6)
> -	if (mp->addr.proto == htons(ETH_P_IPV6))
> +	else if (mp->addr.proto == htons(ETH_P_IPV6))
>  		e.addr.u.ip6 = mp->addr.dst.ip6;
>  #endif
> +	else
> +		ether_addr_copy(e.addr.u.mac_addr, mp->addr.mac_addr);
>  	e.addr.proto = mp->addr.proto;
>  	nest_ent = nla_nest_start_noflag(skb,
>  					 MDBA_MDB_ENTRY_INFO);
> @@ -210,6 +216,8 @@ static int __mdb_fill_info(struct sk_buff *skb,
>  		}
>  		break;
>  #endif
> +	default:
> +		ether_addr_copy(e.addr.u.mac_addr, mp->addr.mac_addr);
>  	}
>  	if (p) {
>  		if (nla_put_u8(skb, MDBA_MDB_EATTR_RTPROT, p->rt_protocol))
> @@ -562,9 +570,12 @@ void br_mdb_notify(struct net_device *dev,
>  		if (mp->addr.proto == htons(ETH_P_IP))
>  			ip_eth_mc_map(mp->addr.dst.ip4, mdb.addr);
>  #if IS_ENABLED(CONFIG_IPV6)
> -		else
> +		else if (mp->addr.proto == htons(ETH_P_IPV6))
>  			ipv6_eth_mc_map(&mp->addr.dst.ip6, mdb.addr);
>  #endif
> +		else
> +			ether_addr_copy(mdb.addr, mp->addr.mac_addr);
> +
>  		mdb.obj.orig_dev = pg->key.port->dev;
>  		switch (type) {
>  		case RTM_NEWMDB:
> @@ -693,7 +704,7 @@ static bool is_valid_mdb_entry(struct br_mdb_entry *entry,
>  			return false;
>  		}
>  #endif
> -	} else {
> +	} else if (entry->addr.proto != 0) {
>  		NL_SET_ERR_MSG_MOD(extack, "Unknown entry protocol");
>  		return false;
>  	}
> @@ -857,6 +868,11 @@ static int br_mdb_add_group(struct net_bridge *br, struct net_bridge_port *port,
>  			return err;
>  	}
>  
> +	if (entry->state != MDB_PERMANENT && mp->l2) {
> +		NL_SET_ERR_MSG_MOD(extack, "Only permanent L2 entries allowed");
> +		return -EINVAL;
> +	}
> +
>  	/* host join */
>  	if (!port) {
>  		if (mp->host_joined) {
> @@ -891,7 +907,7 @@ static int br_mdb_add_group(struct net_bridge *br, struct net_bridge_port *port,
>  		return -ENOMEM;
>  	}
>  	rcu_assign_pointer(*pp, p);
> -	if (entry->state == MDB_TEMPORARY)
> +	if (entry->state == MDB_TEMPORARY && !mp->l2)
>  		mod_timer(&p->timer, now + br->multicast_membership_interval);
>  	br_mdb_notify(br->dev, mp, p, RTM_NEWMDB);
>  	/* if we are adding a new EXCLUDE port group (*,G) it needs to be also
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index eae898c3cff7..bc03057e7caf 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,
>  		break;
>  #endif
>  	default:
> -		return NULL;
> +		ip.proto = 0;
> +		ether_addr_copy(ip.mac_addr, eth_hdr(skb)->h_dest);
>  	}
>  
>  	return br_mdb_ip_get_rcu(br, &ip);
> @@ -1050,6 +1051,7 @@ struct net_bridge_mdb_entry *br_multicast_new_group(struct net_bridge *br,
>  
>  	mp->br = br;
>  	mp->addr = *group;
> +	mp->l2 = !!(group->proto == 0);
>  	mp->mcast_gc.destroy = br_multicast_destroy_mdb_entry;
>  	timer_setup(&mp->timer, br_multicast_group_expired, 0);
>  	err = rhashtable_lookup_insert_fast(&br->mdb_hash_tbl, &mp->rhnode,
> @@ -1169,6 +1171,8 @@ struct net_bridge_port_group *br_multicast_new_port_group(
>  	p->key.addr = *group;
>  	p->key.port = port;
>  	p->flags = flags;
> +	if (group->proto == htons(0))
> +		p->flags |= MDB_PG_FLAGS_L2;

Can we pass the flag from the caller? This kind of beats the purpose of
receiving the flags as an argument. :)

>  	p->filter_mode = filter_mode;
>  	p->rt_protocol = rt_protocol;
>  	p->mcast_gc.destroy = br_multicast_destroy_port_group;
> @@ -1203,6 +1207,10 @@ void br_multicast_host_join(struct net_bridge_mdb_entry *mp, bool notify)
>  		if (notify)
>  			br_mdb_notify(mp->br->dev, mp, NULL, RTM_NEWMDB);
>  	}
> +
> +	if (mp->l2)
> +		return;
> +
>  	mod_timer(&mp->timer, jiffies + mp->br->multicast_membership_interval);
>  }
>  
> @@ -3690,7 +3698,7 @@ bool br_multicast_has_querier_anywhere(struct net_device *dev, int proto)
>  	memset(&eth, 0, sizeof(eth));
>  	eth.h_proto = htons(proto);
>  
> -	ret = br_multicast_querier_exists(br, &eth);
> +	ret = br_multicast_querier_exists(br, &eth, NULL);
>  
>  unlock:
>  	rcu_read_unlock();
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 345118e35c42..63a98c1af351 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -215,6 +215,7 @@ struct net_bridge_fdb_entry {
>  #define MDB_PG_FLAGS_FAST_LEAVE	BIT(2)
>  #define MDB_PG_FLAGS_STAR_EXCL	BIT(3)
>  #define MDB_PG_FLAGS_BLOCKED	BIT(4)
> +#define MDB_PG_FLAGS_L2		BIT(5)
>  
>  #define PG_SRC_ENT_LIMIT	32
>  
> @@ -272,6 +273,7 @@ struct net_bridge_mdb_entry {
>  	struct net_bridge_port_group __rcu *ports;
>  	struct br_ip			addr;
>  	bool				host_joined;
> +	bool				l2;
>  
>  	struct timer_list		timer;
>  	struct hlist_node		mdb_node;
> @@ -871,7 +873,8 @@ __br_multicast_querier_exists(struct net_bridge *br,
>  }
>  
>  static inline bool br_multicast_querier_exists(struct net_bridge *br,
> -					       struct ethhdr *eth)
> +					       struct ethhdr *eth,
> +					       const struct net_bridge_mdb_entry *mdb)
>  {
>  	switch (eth->h_proto) {
>  	case (htons(ETH_P_IP)):
> @@ -883,7 +886,7 @@ static inline bool br_multicast_querier_exists(struct net_bridge *br,
>  			&br->ip6_other_query, true);
>  #endif
>  	default:
> -		return false;
> +		return !!(mdb && mdb->l2);
>  	}
>  }
>  

Powered by blists - more mailing lists