Message-ID: <74433b12-78a9-1d7a-d169-a5fcdb4a5850@blackwall.org>
Date:   Wed, 19 Oct 2022 16:15:16 +0300
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Ido Schimmel <idosch@...dia.com>, netdev@...r.kernel.org,
        bridge@...ts.linux-foundation.org
Cc:     davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        edumazet@...gle.com, roopa@...dia.com, mlxsw@...dia.com
Subject: Re: [RFC PATCH net-next 00/19] bridge: mcast: Extensions for EVPN

On 18/10/2022 15:04, Ido Schimmel wrote:
> Posting as RFC to show the whole picture. Will split into smaller
> submissions for v1 and add selftests.
> 
> tl;dr
> =====
> 
> This patchset creates feature parity between user space and the kernel
> and allows the former to install and replace MDB port group entries with
> a source list and associated filter mode. This is required for EVPN use
> cases where multicast state is not derived from snooped IGMP/MLD
> packets, but instead derived from EVPN routes exchanged by the control
> plane in user space.
> 
> Background
> ==========
> 
> IGMPv3 [1] and MLDv2 [2] differ from earlier versions of the protocols
> in that they add support for source-specific multicast. That is, hosts
> can advertise interest in listening to a particular multicast address
> only from specific source addresses or from all sources except for
> specific source addresses.
> 
> In kernel 5.10 [3][4], the bridge driver gained the ability to snoop
> IGMPv3/MLDv2 packets and install corresponding MDB port group entries.
> For example, a snooped IGMPv3 Membership Report that contains a single
> MODE_IS_EXCLUDE record for group 239.10.10.10 with sources 192.0.2.1,
> 192.0.2.2, 192.0.2.20 and 192.0.2.21 would trigger the creation of these
> entries:
> 
>  # bridge -d mdb show
>  dev br0 port veth1 grp 239.10.10.10 src 192.0.2.21 temp filter_mode include proto kernel  blocked
>  dev br0 port veth1 grp 239.10.10.10 src 192.0.2.20 temp filter_mode include proto kernel  blocked
>  dev br0 port veth1 grp 239.10.10.10 src 192.0.2.2 temp filter_mode include proto kernel  blocked
>  dev br0 port veth1 grp 239.10.10.10 src 192.0.2.1 temp filter_mode include proto kernel  blocked
>  dev br0 port veth1 grp 239.10.10.10 temp filter_mode exclude source_list 192.0.2.21/0.00,192.0.2.20/0.00,192.0.2.2/0.00,192.0.2.1/0.00 proto kernel
> 
> While the kernel can install and replace entries with a filter mode and
> source list, user space cannot. It can only add EXCLUDE entries with an
> empty source list, which is sufficient for IGMPv2/MLDv1, but not for
> IGMPv3/MLDv2.
> 
> Use cases where the multicast state is not derived from snooped packets,
> but instead derived from routes exchanged by the user space control
> plane require feature parity between user space and the kernel in terms
> of MDB configuration. Such a use case is detailed in the next section.
> 
> Motivation
> ==========
> 
> RFC 7432 [5] defines a "MAC/IP Advertisement route" (type 2) [6] that
> allows NVE switches in the EVPN network to advertise and learn
> reachability information for unicast MAC addresses. Traffic destined to
> a unicast MAC address can therefore be selectively forwarded to a single
> NVE switch behind which the MAC is located.
> 
> The same is not true for IP multicast traffic. Such traffic is simply
> flooded as BUM to all NVE switches in the broadcast domain (BD),
> regardless if a switch has interested receivers for the multicast stream
> or not. This is especially problematic for overlay networks that make
> heavy use of multicast.
> 
> The issue is addressed by RFC 9251 [7] that defines a "Selective
> Multicast Ethernet Tag Route" (type 6) [8] which allows NVE switches in
> the EVPN network to advertise multicast streams that they are interested
> in. This is done by having each switch suppress IGMP/MLD packets from
> being transmitted to the NVE network and instead communicate the
> information over BGP to other switches.
> 
> As far as the bridge driver is concerned, the above means that the
> multicast state (i.e., {multicast address, group timer, filter-mode,
> (source records)}) for the VXLAN bridge port is not populated by the
> kernel from snooped IGMP/MLD packets (they are suppressed), but instead
> by user space. Specifically, by the routing daemon that is exchanging
> EVPN routes with other NVE switches.
> 
> Changes are obviously also required in the VXLAN driver, but they are
> the subject of future patchsets. See the "Future work" section.
> 
> Implementation
> ==============
> 
> The user interface is extended to allow user space to specify the filter
> mode of the MDB port group entry and its source list. Replace support is
> also added so that user space would not need to remove an entry and
> re-add it only to edit its source list or filter mode, as that would
> result in packet loss. Example usage:
> 
>  # bridge mdb replace dev br0 port dummy10 grp 239.1.1.1 permanent \
> 	source_list 192.0.2.1,192.0.2.3 filter_mode exclude proto zebra
>  # bridge -d -s mdb show
>  dev br0 port dummy10 grp 239.1.1.1 src 192.0.2.3 permanent filter_mode include proto zebra  blocked    0.00
>  dev br0 port dummy10 grp 239.1.1.1 src 192.0.2.1 permanent filter_mode include proto zebra  blocked    0.00
>  dev br0 port dummy10 grp 239.1.1.1 permanent filter_mode exclude source_list 192.0.2.3/0.00,192.0.2.1/0.00 proto zebra     0.00
> 
> The netlink interface is extended with a few new attributes in the
> RTM_NEWMDB request message:
> 
> [ struct nlmsghdr ]
> [ struct br_port_msg ]
> [ MDBA_SET_ENTRY ]
> 	struct br_mdb_entry
> [ MDBA_SET_ENTRY_ATTRS ]
> 	[ MDBE_ATTR_SOURCE ]
> 		struct in_addr / struct in6_addr
> 	[ MDBE_ATTR_SRC_LIST ]		// new
> 		[ MDBE_SRC_LIST_ENTRY ]
> 			[ MDBE_SRCATTR_ADDRESS ]
> 				struct in_addr / struct in6_addr
> 		[ ...]
> 	[ MDBE_ATTR_GROUP_MODE ]	// new
> 		u8
> 	[ MDBE_ATTR_RTPORT ]		// new
> 		u8
> 
> No changes are required in RTM_NEWMDB responses and notifications, as
> all the information can already be dumped by the kernel today.
> 
> Testing
> =======
> 
> Tested with existing bridge multicast selftests: bridge_igmp.sh,
> bridge_mdb_port_down.sh, bridge_mdb.sh, bridge_mld.sh,
> bridge_vlan_mcast.sh.
> 
> Will add dedicated selftests in v1.
> 
> Patchset overview
> =================
> 
> Patches #1-#8 are non-functional preparations aimed at making it easier
> to add additional netlink attributes later on. They create an MDB
> configuration structure into which netlink messages are parsed into.
> The structure is then passed in the entry creation / deletion call chain
> instead of passing the netlink attributes themselves. The same pattern
> is used by other rtnetlink objects such as routes and nexthops.
> 
> I initially tried to extend the current code, but it proved to be too
> difficult, which is why I decided to refactor it to the extensible and
> familiar pattern used by other rtnetlink objects.
> 
> The plan is to submit these patches separately for v1.
> 
> Patches #9-#15 are an additional set of non-functional preparations for
> the core changes in the last patches.
> 
> Patches #16-#17 allow user space to install (*, G) entries with a source
> list and associated filter mode. Specifically, patch #16 adds the
> necessary kernel plumbing and patch #17 exposes the new functionality to
> user space via a few new attributes.
> 
> Patch #18 allows user space to specify the routing protocol of new MDB
> port group entries so that a routing daemon could differentiate between
> entries installed by it and those installed by an administrator.
> 
> Patch #19 allows user space to replace MDB port group entries. This is
> useful, for example, when user space wants to add a new source to a
> source list. Instead of deleting a (*, G) entry and re-adding it with an
> extended source list (which would result in packet loss), user space can
> simply replace the current entry.
> 
> Future work
> ===========
> 
> The VXLAN driver will need to be extended with an MDB so that it could
> selectively forward IP multicast traffic to NVE switches with interested
> receivers instead of simply flooding it to all switches as BUM.
> 
> The idea is to reuse the existing MDB interface for the VXLAN driver in
> a similar way to how the FDB interface is shared between the bridge and
> VXLAN drivers.
> 
> From command line perspective, configuration will look as follows:
> 
>  # bridge mdb add dev br0 port vxlan0 grp 239.1.1.1 permanent \
> 	filter_mode exclude source_list 198.50.100.1,198.50.100.2
> 
>  # bridge mdb add dev vxlan0 port vxlan0 grp 239.1.1.1 permanent \
> 	filter_mode include source_list 198.50.100.3,198.50.100.4 \
> 	dst 192.0.2.1 dst_port 4789 src_vni 2
> 
>  # bridge mdb add dev vxlan0 port vxlan0 grp 239.1.1.1 permanent \
> 	filter_mode exclude source_list 198.50.100.1,198.50.100.2 \
> 	dst 192.0.2.2 dst_port 4789 src_vni 2
> 
> Where the first command is enabled by this set, but the next two will be
> the subject of future work.
> 
> From netlink perspective, the existing PF_BRIDGE/RTM_*MDB messages will
> be extended to the VXLAN driver. This means that a few new attributes
> will be added (e.g., 'MDBE_ATTR_SRC_VNI') and that the handlers for
> these messages will need to move to net/core/rtnetlink.c. The rtnetlink
> code will call into the appropriate driver based on the ifindex
> specified in the ancillary header.
> 
> iproute2 patches can be found here [9].
> 
> [1] https://datatracker.ietf.org/doc/html/rfc3376
> [2] https://www.rfc-editor.org/rfc/rfc3810
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6af52ae2ed14a6bc756d5606b29097dfd76740b8
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68d4fd30c83b1b208e08c954cd45e6474b148c87
> [5] https://datatracker.ietf.org/doc/html/rfc7432
> [6] https://datatracker.ietf.org/doc/html/rfc7432#section-7.2
> [7] https://datatracker.ietf.org/doc/html/rfc9251
> [8] https://datatracker.ietf.org/doc/html/rfc9251#section-9.1
> [9] https://github.com/idosch/iproute2/commits/submit/mdb_rfc_v1
> 
> Ido Schimmel (19):
>   bridge: mcast: Centralize netlink attribute parsing
>   bridge: mcast: Remove redundant checks
>   bridge: mcast: Use MDB configuration structure where possible
>   bridge: mcast: Propagate MDB configuration structure further
>   bridge: mcast: Use MDB group key from configuration structure
>   bridge: mcast: Remove br_mdb_parse()
>   bridge: mcast: Move checks out of critical section
>   bridge: mcast: Remove redundant function arguments
>   bridge: mcast: Do not derive entry type from its filter mode
>   bridge: mcast: Split (*, G) and (S, G) addition into different
>     functions
>   bridge: mcast: Place netlink policy before validation functions
>   bridge: mcast: Add a centralized error path
>   bridge: mcast: Expose br_multicast_new_group_src()
>   bridge: mcast: Add a flag for user installed source entries
>   bridge: mcast: Avoid arming group timer when (S, G) corresponds to a
>     source
>   bridge: mcast: Add support for (*, G) with a source list and filter
>     mode
>   bridge: mcast: Allow user space to add (*, G) with a source list and
>     filter mode
>   bridge: mcast: Allow user space to specify MDB entry routing protocol
>   bridge: mcast: Support replacement of MDB port group entries
> 
>  include/uapi/linux/if_bridge.h |  21 +
>  net/bridge/br_mdb.c            | 821 ++++++++++++++++++++++++---------
>  net/bridge/br_multicast.c      |   5 +-
>  net/bridge/br_private.h        |  21 +
>  4 files changed, 649 insertions(+), 219 deletions(-)
> 

Good work, the set looks good to me. It's a natural extension to allow user-space to
manipulate such mcast groups. As we discussed privately it's only missing the selftests. :)

Cheers,
 Nik

Powered by blists - more mailing lists