[PATCH net-next 0/4] bridge: mcast: Preparations for VXLAN MDB

[PATCH net-next 0/4] bridge: mcast: Preparations for VXLAN MDB

[Ido Schimmel]

This patchset contains small preparations for VXLAN MDB that were split
from this RFC [1]. Tested using existing bridge MDB forwarding
selftests.

[1] https://lore.kernel.org/netdev/20230204170801.3897900-1-idosch@nvidia.com/

Ido Schimmel (4):
  bridge: mcast: Use correct define in MDB dump
  bridge: mcast: Remove pointless sequence generation counter assignment
  bridge: mcast: Move validation to a policy
  selftests: forwarding: Add MDB dump test cases

 net/bridge/br_mdb.c                           | 49 +++++----
 .../selftests/net/forwarding/bridge_mdb.sh    | 99 +++++++++++++++++++
 2 files changed, 127 insertions(+), 21 deletions(-)

-- 
2.37.3

[PATCH net-next 1/4] bridge: mcast: Use correct define in MDB dump

[Ido Schimmel]

'MDB_PG_FLAGS_PERMANENT' and 'MDB_PERMANENT' happen to have the same
value, but the latter is uAPI and cannot change, so use it when dumping
an MDB entry.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/bridge/br_mdb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 9f22ebfdc518..13076206e497 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -259,7 +259,7 @@ static int __mdb_fill_info(struct sk_buff *skb,
 #endif
 	} else {
 		ether_addr_copy(e.addr.u.mac_addr, mp->addr.dst.mac_addr);
-		e.state = MDB_PG_FLAGS_PERMANENT;
+		e.state = MDB_PERMANENT;
 	}
 	e.addr.proto = mp->addr.proto;
 	nest_ent = nla_nest_start_noflag(skb,
-- 
2.37.3

[PATCH net-next 2/4] bridge: mcast: Remove pointless sequence generation counter assignment

[Ido Schimmel]

The purpose of the sequence generation counter in the netlink callback
is to identify if a multipart dump is consistent or not by calling
nl_dump_check_consistent() whenever a message is generated.

The function is not invoked by the MDB code, rendering the sequence
generation counter assignment pointless. Remove it.

Note that even if the function was invoked, we still could not
accurately determine if the dump is consistent or not, as there is no
sequence generation counter for MDB entries, unlike nexthop objects, for
example.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/bridge/br_mdb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 13076206e497..96f36febfb30 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -421,8 +421,6 @@ static int br_mdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 	rcu_read_lock();
 
-	cb->seq = net->dev_base_seq;
-
 	for_each_netdev_rcu(net, dev) {
 		if (netif_is_bridge_master(dev)) {
 			struct net_bridge *br = netdev_priv(dev);
-- 
2.37.3

[PATCH net-next 3/4] bridge: mcast: Move validation to a policy

[Ido Schimmel]

Future patches are going to move parts of the bridge MDB code to the
common rtnetlink code in preparation for VXLAN MDB support. To
facilitate code sharing between both drivers, move the validation of the
top level attributes in RTM_{NEW,DEL}MDB messages to a policy that will
eventually be moved to the rtnetlink code.

Use 'NLA_NESTED' for 'MDBA_SET_ENTRY_ATTRS' instead of
NLA_POLICY_NESTED() as this attribute is going to be validated using
different policies in the underlying drivers.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/bridge/br_mdb.c | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 96f36febfb30..25c48d81a597 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -683,51 +683,58 @@ static const struct nla_policy br_mdbe_attrs_pol[MDBE_ATTR_MAX + 1] = {
 	[MDBE_ATTR_RTPROT] = NLA_POLICY_MIN(NLA_U8, RTPROT_STATIC),
 };
 
-static bool is_valid_mdb_entry(struct br_mdb_entry *entry,
-			       struct netlink_ext_ack *extack)
+static int validate_mdb_entry(const struct nlattr *attr,
+			      struct netlink_ext_ack *extack)
 {
+	struct br_mdb_entry *entry = nla_data(attr);
+
+	if (nla_len(attr) != sizeof(struct br_mdb_entry)) {
+		NL_SET_ERR_MSG_MOD(extack, "Invalid MDBA_SET_ENTRY attribute length");
+		return -EINVAL;
+	}
+
 	if (entry->ifindex == 0) {
 		NL_SET_ERR_MSG_MOD(extack, "Zero entry ifindex is not allowed");
-		return false;
+		return -EINVAL;
 	}
 
 	if (entry->addr.proto == htons(ETH_P_IP)) {
 		if (!ipv4_is_multicast(entry->addr.u.ip4)) {
 			NL_SET_ERR_MSG_MOD(extack, "IPv4 entry group address is not multicast");
-			return false;
+			return -EINVAL;
 		}
 		if (ipv4_is_local_multicast(entry->addr.u.ip4)) {
 			NL_SET_ERR_MSG_MOD(extack, "IPv4 entry group address is local multicast");
-			return false;
+			return -EINVAL;
 		}
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (entry->addr.proto == htons(ETH_P_IPV6)) {
 		if (ipv6_addr_is_ll_all_nodes(&entry->addr.u.ip6)) {
 			NL_SET_ERR_MSG_MOD(extack, "IPv6 entry group address is link-local all nodes");
-			return false;
+			return -EINVAL;
 		}
 #endif
 	} else if (entry->addr.proto == 0) {
 		/* L2 mdb */
 		if (!is_multicast_ether_addr(entry->addr.u.mac_addr)) {
 			NL_SET_ERR_MSG_MOD(extack, "L2 entry group is not multicast");
-			return false;
+			return -EINVAL;
 		}
 	} else {
 		NL_SET_ERR_MSG_MOD(extack, "Unknown entry protocol");
-		return false;
+		return -EINVAL;
 	}
 
 	if (entry->state != MDB_PERMANENT && entry->state != MDB_TEMPORARY) {
 		NL_SET_ERR_MSG_MOD(extack, "Unknown entry state");
-		return false;
+		return -EINVAL;
 	}
 	if (entry->vid >= VLAN_VID_MASK) {
 		NL_SET_ERR_MSG_MOD(extack, "Invalid entry VLAN id");
-		return false;
+		return -EINVAL;
 	}
 
-	return true;
+	return 0;
 }
 
 static bool is_valid_mdb_source(struct nlattr *attr, __be16 proto,
@@ -1292,6 +1299,14 @@ static int br_mdb_config_attrs_init(struct nlattr *set_attrs,
 	return 0;
 }
 
+static const struct nla_policy mdba_policy[MDBA_SET_ENTRY_MAX + 1] = {
+	[MDBA_SET_ENTRY_UNSPEC] = { .strict_start_type = MDBA_SET_ENTRY_ATTRS + 1 },
+	[MDBA_SET_ENTRY] = NLA_POLICY_VALIDATE_FN(NLA_BINARY,
+						  validate_mdb_entry,
+						  sizeof(struct br_mdb_entry)),
+	[MDBA_SET_ENTRY_ATTRS] = { .type = NLA_NESTED },
+};
+
 static int br_mdb_config_init(struct net *net, const struct nlmsghdr *nlh,
 			      struct br_mdb_config *cfg,
 			      struct netlink_ext_ack *extack)
@@ -1302,7 +1317,7 @@ static int br_mdb_config_init(struct net *net, const struct nlmsghdr *nlh,
 	int err;
 
 	err = nlmsg_parse_deprecated(nlh, sizeof(*bpm), tb,
-				     MDBA_SET_ENTRY_MAX, NULL, extack);
+				     MDBA_SET_ENTRY_MAX, mdba_policy, extack);
 	if (err)
 		return err;
 
@@ -1344,14 +1359,8 @@ static int br_mdb_config_init(struct net *net, const struct nlmsghdr *nlh,
 		NL_SET_ERR_MSG_MOD(extack, "Missing MDBA_SET_ENTRY attribute");
 		return -EINVAL;
 	}
-	if (nla_len(tb[MDBA_SET_ENTRY]) != sizeof(struct br_mdb_entry)) {
-		NL_SET_ERR_MSG_MOD(extack, "Invalid MDBA_SET_ENTRY attribute length");
-		return -EINVAL;
-	}
 
 	cfg->entry = nla_data(tb[MDBA_SET_ENTRY]);
-	if (!is_valid_mdb_entry(cfg->entry, extack))
-		return -EINVAL;
 
 	if (cfg->entry->ifindex != cfg->br->dev->ifindex) {
 		struct net_device *pdev;
-- 
2.37.3

[PATCH net-next 4/4] selftests: forwarding: Add MDB dump test cases

[Ido Schimmel]

The kernel maintains three markers for the MDB dump:

1. The last bridge device from which the MDB was dumped.
2. The last MDB entry from which the MDB was dumped.
3. The last port-group entry that was dumped.

Add test cases for large scale MDB dump to make sure that all the
configured entries are dumped and that the markers are used correctly.

Specifically, create 2 bridges with 32 ports and add 256 MDB entries in
which all the ports are member of. Test that each bridge reports 8192
(256 * 32) permanent entries. Do that with IPv4, IPv6 and L2 MDB
entries.

On my system, MDB dump of the above is contained in about 50 netlink
messages.

Example output:

 # ./bridge_mdb.sh
 [...]
 INFO: # Large scale dump tests
 TEST: IPv4 large scale dump tests                                   [ OK ]
 TEST: IPv6 large scale dump tests                                   [ OK ]
 TEST: L2 large scale dump tests                                     [ OK ]
 [...]

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh    | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index b48867d8cadf..ae3f9462a2b6 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -742,10 +742,109 @@ cfg_test_port()
 	cfg_test_port_l2
 }
 
+ipv4_grps_get()
+{
+	local max_grps=$1; shift
+	local i
+
+	for i in $(seq 0 $((max_grps - 1))); do
+		echo "239.1.1.$i"
+	done
+}
+
+ipv6_grps_get()
+{
+	local max_grps=$1; shift
+	local i
+
+	for i in $(seq 0 $((max_grps - 1))); do
+		echo "ff0e::$(printf %x $i)"
+	done
+}
+
+l2_grps_get()
+{
+	local max_grps=$1; shift
+	local i
+
+	for i in $(seq 0 $((max_grps - 1))); do
+		echo "01:00:00:00:00:$(printf %02x $i)"
+	done
+}
+
+cfg_test_dump_common()
+{
+	local name=$1; shift
+	local fn=$1; shift
+	local max_bridges=2
+	local max_grps=256
+	local max_ports=32
+	local num_entries
+	local batch_file
+	local grp
+	local i j
+
+	RET=0
+
+	# Create net devices.
+	for i in $(seq 1 $max_bridges); do
+		ip link add name br-test${i} up type bridge vlan_filtering 1 \
+			mcast_snooping 1
+		for j in $(seq 1 $max_ports); do
+			ip link add name br-test${i}-du${j} up \
+				master br-test${i} type dummy
+		done
+	done
+
+	# Create batch file with MDB entries.
+	batch_file=$(mktemp)
+	for i in $(seq 1 $max_bridges); do
+		for j in $(seq 1 $max_ports); do
+			for grp in $($fn $max_grps); do
+				echo "mdb add dev br-test${i} \
+					port br-test${i}-du${j} grp $grp \
+					permanent vid 1" >> $batch_file
+			done
+		done
+	done
+
+	# Program the batch file and check for expected number of entries.
+	bridge -b $batch_file
+	for i in $(seq 1 $max_bridges); do
+		num_entries=$(bridge mdb show dev br-test${i} | \
+			grep "permanent" | wc -l)
+		[[ $num_entries -eq $((max_grps * max_ports)) ]]
+		check_err $? "Wrong number of entries in br-test${i}"
+	done
+
+	# Cleanup.
+	rm $batch_file
+	for i in $(seq 1 $max_bridges); do
+		ip link del dev br-test${i}
+		for j in $(seq $max_ports); do
+			ip link del dev br-test${i}-du${j}
+		done
+	done
+
+	log_test "$name large scale dump tests"
+}
+
+# Check large scale dump.
+cfg_test_dump()
+{
+	echo
+	log_info "# Large scale dump tests"
+
+	cfg_test_dump_common "IPv4" ipv4_grps_get
+	cfg_test_dump_common "IPv6" ipv6_grps_get
+	cfg_test_dump_common "L2" l2_grps_get
+}
+
 cfg_test()
 {
 	cfg_test_host
 	cfg_test_port
+	cfg_test_dump
 }
 
 __fwd_test_host_ip()
-- 
2.37.3

Re: [PATCH net-next 1/4] bridge: mcast: Use correct define in MDB dump

[Nikolay Aleksandrov]

On 2/9/23 09:18, Ido Schimmel wrote:
> 'MDB_PG_FLAGS_PERMANENT' and 'MDB_PERMANENT' happen to have the same
> value, but the latter is uAPI and cannot change, so use it when dumping
> an MDB entry.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>   net/bridge/br_mdb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 9f22ebfdc518..13076206e497 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -259,7 +259,7 @@ static int __mdb_fill_info(struct sk_buff *skb,
>   #endif
>   	} else {
>   		ether_addr_copy(e.addr.u.mac_addr, mp->addr.dst.mac_addr);
> -		e.state = MDB_PG_FLAGS_PERMANENT;
> +		e.state = MDB_PERMANENT;
>   	}
>   	e.addr.proto = mp->addr.proto;
>   	nest_ent = nla_nest_start_noflag(skb,

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH net-next 2/4] bridge: mcast: Remove pointless sequence generation counter assignment

[Nikolay Aleksandrov]

On 2/9/23 09:18, Ido Schimmel wrote:
> The purpose of the sequence generation counter in the netlink callback
> is to identify if a multipart dump is consistent or not by calling
> nl_dump_check_consistent() whenever a message is generated.
> 
> The function is not invoked by the MDB code, rendering the sequence
> generation counter assignment pointless. Remove it.
> 
> Note that even if the function was invoked, we still could not
> accurately determine if the dump is consistent or not, as there is no
> sequence generation counter for MDB entries, unlike nexthop objects, for
> example.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>   net/bridge/br_mdb.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 13076206e497..96f36febfb30 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -421,8 +421,6 @@ static int br_mdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
>   
>   	rcu_read_lock();
>   
> -	cb->seq = net->dev_base_seq;
> -
>   	for_each_netdev_rcu(net, dev) {
>   		if (netif_is_bridge_master(dev)) {
>   			struct net_bridge *br = netdev_priv(dev);


Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH net-next 3/4] bridge: mcast: Move validation to a policy

[Nikolay Aleksandrov]

On 2/9/23 09:18, Ido Schimmel wrote:
> Future patches are going to move parts of the bridge MDB code to the
> common rtnetlink code in preparation for VXLAN MDB support. To
> facilitate code sharing between both drivers, move the validation of the
> top level attributes in RTM_{NEW,DEL}MDB messages to a policy that will
> eventually be moved to the rtnetlink code.
> 
> Use 'NLA_NESTED' for 'MDBA_SET_ENTRY_ATTRS' instead of
> NLA_POLICY_NESTED() as this attribute is going to be validated using
> different policies in the underlying drivers.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>   net/bridge/br_mdb.c | 45 +++++++++++++++++++++++++++------------------
>   1 file changed, 27 insertions(+), 18 deletions(-)
> 


Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH net-next 4/4] selftests: forwarding: Add MDB dump test cases

[Nikolay Aleksandrov]

On 2/9/23 09:18, Ido Schimmel wrote:
> The kernel maintains three markers for the MDB dump:
> 
> 1. The last bridge device from which the MDB was dumped.
> 2. The last MDB entry from which the MDB was dumped.
> 3. The last port-group entry that was dumped.
> 
> Add test cases for large scale MDB dump to make sure that all the
> configured entries are dumped and that the markers are used correctly.
> 
> Specifically, create 2 bridges with 32 ports and add 256 MDB entries in
> which all the ports are member of. Test that each bridge reports 8192
> (256 * 32) permanent entries. Do that with IPv4, IPv6 and L2 MDB
> entries.
> 
> On my system, MDB dump of the above is contained in about 50 netlink
> messages.
> 
> Example output:
> 
>   # ./bridge_mdb.sh
>   [...]
>   INFO: # Large scale dump tests
>   TEST: IPv4 large scale dump tests                                   [ OK ]
>   TEST: IPv6 large scale dump tests                                   [ OK ]
>   TEST: L2 large scale dump tests                                     [ OK ]
>   [...]
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> ---
>   .../selftests/net/forwarding/bridge_mdb.sh    | 99 +++++++++++++++++++
>   1 file changed, 99 insertions(+)
> 

Nice!
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH net-next 3/4] bridge: mcast: Move validation to a policy

[Jakub Kicinski]

On Thu,  9 Feb 2023 09:18:51 +0200 Ido Schimmel wrote:
> +	if (nla_len(attr) != sizeof(struct br_mdb_entry)) {
> +		NL_SET_ERR_MSG_MOD(extack, "Invalid MDBA_SET_ENTRY attribute length");
> +		return -EINVAL;

Well, you're just moving it, but NL_SET_ERR_MSG_ATTR() would be better.
We shouldn't be adding _MOD() in the core implementation of the family.

Re: [PATCH net-next 0/4] bridge: mcast: Preparations for VXLAN MDB

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  9 Feb 2023 09:18:48 +0200 you wrote:
> This patchset contains small preparations for VXLAN MDB that were split
> from this RFC [1]. Tested using existing bridge MDB forwarding
> selftests.
> 
> [1] https://lore.kernel.org/netdev/20230204170801.3897900-1-idosch@nvidia.com/
> 
> Ido Schimmel (4):
>   bridge: mcast: Use correct define in MDB dump
>   bridge: mcast: Remove pointless sequence generation counter assignment
>   bridge: mcast: Move validation to a policy
>   selftests: forwarding: Add MDB dump test cases
> 
> [...]

Here is the summary with links:
  - [net-next,1/4] bridge: mcast: Use correct define in MDB dump
    https://git.kernel.org/netdev/net-next/c/ccd7f25b5b04
  - [net-next,2/4] bridge: mcast: Remove pointless sequence generation counter assignment
    https://git.kernel.org/netdev/net-next/c/7ea829664d3c
  - [net-next,3/4] bridge: mcast: Move validation to a policy
    https://git.kernel.org/netdev/net-next/c/170afa71e3a2
  - [net-next,4/4] selftests: forwarding: Add MDB dump test cases
    https://git.kernel.org/netdev/net-next/c/049139126ec7

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 3/4] bridge: mcast: Move validation to a policy

[Ido Schimmel]

On Fri, Feb 10, 2023 at 07:20:57PM -0800, Jakub Kicinski wrote:
> On Thu,  9 Feb 2023 09:18:51 +0200 Ido Schimmel wrote:
> > +	if (nla_len(attr) != sizeof(struct br_mdb_entry)) {
> > +		NL_SET_ERR_MSG_MOD(extack, "Invalid MDBA_SET_ENTRY attribute length");
> > +		return -EINVAL;
> 
> Well, you're just moving it, but NL_SET_ERR_MSG_ATTR() would be better.
> We shouldn't be adding _MOD() in the core implementation of the family.

Assuming you are referring to this code when it's moved to
net/core/rtnetlink.c, then I already removed the _MOD() suffix from it
(as visible in the RFC). I can squash this as well:

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index c9f878a28d32..491e4231b3dd 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -6138,7 +6138,7 @@ static int rtnl_validate_mdb_entry(const struct nlattr *attr,
        struct br_mdb_entry *entry = nla_data(attr);
 
        if (nla_len(attr) != sizeof(struct br_mdb_entry)) {
-               NL_SET_ERR_MSG(extack, "Invalid MDBA_SET_ENTRY attribute length");
+               NL_SET_ERR_MSG_ATTR(extack, attr, "Invalid attribute length");
                return -EINVAL;
        }