Re: [RFC PATCH] net: bridge: multicast: add support for L2 entries

[Nikolay Aleksandrov]

On Wed, 2020-10-21 at 09:17 +0000, Nikolay Aleksandrov wrote:
> On Sat, 2020-10-17 at 21:41 +0300, 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,
> > 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@xxxxxxxxxx>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> > ---
> > 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.
> 

Oh, one more thing, I don't think we validate that the dst mac that's being
added is actually a multicast one.

> >  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);
> >  	}
> >  }
> >