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 <niko...@nvidia.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 <niko...@nvidia.com>
> > Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.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.
>
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(ð, 0, sizeof(eth));
> > eth.h_proto = htons(proto);
> >
> > - ret = br_multicast_querier_exists(br, ð);
> > + ret = br_multicast_querier_exists(br, ð, 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);
> > }
> > }
> >
