Merge pull request #17059 from yuwata/network-mdb-follow-ups

network: follow ups for bridge MDB support

src/basic/in-addr-util.c

@@ -54,6 +54,16 @@ int in_addr_is_link_local(int family, const union in_addr_union *u) {
         return -EAFNOSUPPORT;
 }
 
+bool in6_addr_is_link_local_all_nodes(const struct in6_addr *a) {
+        assert(a);
+
+        /* ff02::1 */
+        return be32toh(a->s6_addr32[0]) == UINT32_C(0xff020000) &&
+                a->s6_addr32[1] == 0 &&
+                a->s6_addr32[2] == 0 &&
+                be32toh(a->s6_addr32[3]) == UINT32_C(0x00000001);
+}
+
 int in_addr_is_multicast(int family, const union in_addr_union *u) {
         assert(u);
 
@@ -66,6 +76,12 @@ int in_addr_is_multicast(int family, const union in_addr_union *u) {
         return -EAFNOSUPPORT;
 }
 
+bool in4_addr_is_local_multicast(const struct in_addr *a) {
+        assert(a);
+
+        return (be32toh(a->s_addr) & UINT32_C(0xffffff00)) == UINT32_C(0xe0000000);
+}
+
 bool in4_addr_is_localhost(const struct in_addr *a) {
         assert(a);
 

src/basic/in-addr-util.h

@@ -27,10 +27,12 @@ int in_addr_is_multicast(int family, const union in_addr_union *u);
 
 bool in4_addr_is_link_local(const struct in_addr *a);
 int in_addr_is_link_local(int family, const union in_addr_union *u);
+bool in6_addr_is_link_local_all_nodes(const struct in6_addr *a);
 
 bool in4_addr_is_localhost(const struct in_addr *a);
 int in_addr_is_localhost(int family, const union in_addr_union *u);
 
+bool in4_addr_is_local_multicast(const struct in_addr *a);
 bool in4_addr_is_non_local(const struct in_addr *a);
 
 bool in4_addr_equal(const struct in_addr *a, const struct in_addr *b);

src/network/networkd-link.c

@@ -1157,6 +1157,11 @@ void link_check_ready(Link *link) {
                 return;
         }
 
+        if (!link->bridge_mdb_configured) {
+                log_link_debug(link, "%s(): Bridge MDB is not configured.", __func__);
+                return;
+        }
+
         if (link_has_carrier(link) || !link->network->configure_without_carrier) {
                 bool has_ndisc_address = false;
                 NDiscAddress *n;
@@ -1254,30 +1259,6 @@ static int link_set_bridge_fdb(Link *link) {
         return 0;
 }
 
-static int link_set_bridge_mdb(Link *link) {
-        MdbEntry *mdb_entry;
-        int r;
-
-        if (!link->network)
-                return 0;
-
-        if (LIST_IS_EMPTY(link->network->static_mdb_entries))
-                return 0;
-
-        if (!link->network->bridge) {
-                log_link_error(link, "Cannot configure MDB entries on non-bridge port");
-                return 0;
-        }
-
-        LIST_FOREACH(static_mdb_entries, mdb_entry, link->network->static_mdb_entries) {
-                r = mdb_entry_configure(link, mdb_entry);
-                if (r < 0)
-                        return log_link_error_errno(link, r, "Failed to add entry to multicast group database: %m");
-        }
-
-        return 0;
-}
-
 static int static_address_ready_callback(Address *address) {
         Address *a;
         Link *link;
@@ -1410,6 +1391,10 @@ static int link_request_set_addresses(Link *link) {
         if (r < 0)
                 return r;
 
+        r = link_set_bridge_mdb(link);
+        if (r < 0)
+                return r;
+
         r = link_request_set_neighbors(link);
         if (r < 0)
                 return r;
@@ -3931,9 +3916,24 @@ static int link_carrier_gained(Link *link) {
         if (r < 0)
                 return r;
 
-        r = link_set_bridge_mdb(link);
-        if (r < 0)
-                return r;
+        if (!link->bridge_mdb_configured) {
+                r = link_set_bridge_mdb(link);
+                if (r < 0)
+                        return r;
+        }
+
+        if (streq_ptr(link->kind, "bridge")) {
+                Link *slave;
+
+                SET_FOREACH(slave, link->slaves) {
+                        if (slave->bridge_mdb_configured)
+                                continue;
+
+                        r = link_set_bridge_mdb(slave);
+                        if (r < 0)
+                                link_enter_failed(slave);
+                }
+        }
 
         return 0;
 }

src/network/networkd-link.h

@@ -85,6 +85,7 @@ typedef struct Link {
         unsigned tc_messages;
         unsigned sr_iov_messages;
         unsigned enslaving;
+        unsigned bridge_mdb_messages;
 
         Set *addresses;
         Set *addresses_foreign;
@@ -124,6 +125,7 @@ typedef struct Link {
         bool setting_mtu:1;
         bool setting_genmode:1;
         bool ipv6_mtu_set:1;
+        bool bridge_mdb_configured:1;
 
         LIST_HEAD(Address, pool_addresses);
 

src/network/networkd-mdb.c

@@ -1,14 +1,11 @@
 /* SPDX-License-Identifier: LGPL-2.1+ */
 
-#include <linux/if_bridge.h>
-#include <net/ethernet.h>
 #include <net/if.h>
 
-#include "alloc-util.h"
 #include "netlink-util.h"
 #include "networkd-manager.h"
 #include "networkd-mdb.h"
-#include "util.h"
+#include "string-util.h"
 #include "vlan-util.h"
 
 #define STATIC_MDB_ENTRIES_PER_NETWORK_MAX 1024U
@@ -75,82 +72,6 @@ static int mdb_entry_new_static(
         return 0;
 }
 
-/* parse the VLAN Id from config files. */
-int config_parse_mdb_vlan_id(
-                const char *unit,
-                const char *filename,
-                unsigned line,
-                const char *section,
-                unsigned section_line,
-                const char *lvalue,
-                int ltype,
-                const char *rvalue,
-                void *data,
-                void *userdata) {
-
-        Network *network = userdata;
-        _cleanup_(mdb_entry_free_or_set_invalidp) MdbEntry *mdb_entry = NULL;
-        int r;
-
-        assert(filename);
-        assert(section);
-        assert(lvalue);
-        assert(rvalue);
-        assert(data);
-
-        r = mdb_entry_new_static(network, filename, section_line, &mdb_entry);
-        if (r < 0)
-                return log_oom();
-
-        r = config_parse_vlanid(unit, filename, line, section,
-                                section_line, lvalue, ltype,
-                                rvalue, &mdb_entry->vlan_id, userdata);
-        if (r < 0)
-                return r;
-
-        mdb_entry = NULL;
-
-        return 0;
-}
-
-/* parse the multicast group from config files. */
-int config_parse_mdb_group_address(
-                const char *unit,
-                const char *filename,
-                unsigned line,
-                const char *section,
-                unsigned section_line,
-                const char *lvalue,
-                int ltype,
-                const char *rvalue,
-                void *data,
-                void *userdata) {
-
-        Network *network = userdata;
-        _cleanup_(mdb_entry_free_or_set_invalidp) MdbEntry *mdb_entry = NULL;
-        int r;
-
-        assert(filename);
-        assert(section);
-        assert(lvalue);
-        assert(rvalue);
-        assert(data);
-
-        r = mdb_entry_new_static(network, filename, section_line, &mdb_entry);
-        if (r < 0)
-                return log_oom();
-
-        r = in_addr_from_string_auto(rvalue, &mdb_entry->family, &mdb_entry->group_addr);
-        if (r < 0) {
-                log_syntax(unit, LOG_WARNING, filename, line, r, "Cannot parse multicast group address: %m");
-                return 0;
-        }
-
-        mdb_entry = NULL;
-
-        return 0;
-}
-
 /* remove and MDB entry. */
 MdbEntry *mdb_entry_free(MdbEntry *mdb_entry) {
         if (!mdb_entry)
@@ -174,6 +95,9 @@ static int set_mdb_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link)
         int r;
 
         assert(link);
+        assert(link->bridge_mdb_messages > 0);
+
+        link->bridge_mdb_messages--;
 
         if (IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER))
                 return 1;
@@ -185,23 +109,16 @@ static int set_mdb_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link)
                 return 1;
         }
 
+        if (link->bridge_mdb_messages == 0) {
+                link->bridge_mdb_configured = true;
+                link_check_ready(link);
+        }
+
         return 1;
 }
 
-int mdb_entry_verify(MdbEntry *mdb_entry) {
-        if (section_is_invalid(mdb_entry->section))
-                return -EINVAL;
-
-        if (in_addr_is_multicast(mdb_entry->family, &mdb_entry->group_addr) <= 0) {
-                log_error("No valid MulticastGroupAddress= assignment in this section");
-                return -EINVAL;
-        }
-
-        return 0;
-}
-
 /* send a request to the kernel to add an MDB entry */
-int mdb_entry_configure(Link *link, MdbEntry *mdb_entry) {
+static int mdb_entry_configure(Link *link, MdbEntry *mdb_entry) {
         _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL;
         struct br_mdb_entry entry;
         int r;
@@ -211,6 +128,14 @@ int mdb_entry_configure(Link *link, MdbEntry *mdb_entry) {
         assert(link->manager);
         assert(mdb_entry);
 
+        if (DEBUG_LOGGING) {
+                _cleanup_free_ char *a = NULL;
+
+                (void) in_addr_to_string(mdb_entry->family, &mdb_entry->group_addr, &a);
+                log_link_debug(link, "Configuring bridge MDB entry: MulticastGroupAddress=%s, VLANId=%u",
+                               strna(a), mdb_entry->vlan_id);
+        }
+
         entry = (struct br_mdb_entry) {
                 .state = MDB_PERMANENT,
                 .ifindex = link->ifindex,
@@ -250,3 +175,155 @@ int mdb_entry_configure(Link *link, MdbEntry *mdb_entry) {
 
         return 1;
 }
+
+int link_set_bridge_mdb(Link *link) {
+        MdbEntry *mdb_entry;
+        Link *master;
+        int r;
+
+        assert(link);
+
+        link->bridge_mdb_configured = false;
+
+        if (!link->network)
+                return 0;
+
+        if (!link->network->bridge) {
+                link->bridge_mdb_configured = true;
+                return 0;
+        }
+
+        if (!link_has_carrier(link))
+                return log_link_debug(link, "Link does not have carrier yet, setting MDB entries later.");
+
+        r = link_get(link->manager, link->network->bridge->ifindex, &master);
+        if (r < 0)
+                return log_link_error_errno(link, r, "Failed to get Link object for Bridge=%s", link->network->bridge->ifname);
+
+        if (!link_has_carrier(master))
+                return log_link_debug(link, "Bridge interface %s does not have carrier yet, setting MDB entries later.", link->network->bridge->ifname);
+
+        LIST_FOREACH(static_mdb_entries, mdb_entry, link->network->static_mdb_entries) {
+                r = mdb_entry_configure(link, mdb_entry);
+                if (r < 0)
+                        return log_link_error_errno(link, r, "Failed to add MDB entry to multicast group database: %m");
+
+                link->bridge_mdb_messages++;
+        }
+
+        if (link->bridge_mdb_messages == 0) {
+                link->bridge_mdb_configured = true;
+                link_check_ready(link);
+        }
+
+        return 0;
+}
+
+/* parse the VLAN Id from config files. */
+int config_parse_mdb_vlan_id(
+                const char *unit,
+                const char *filename,
+                unsigned line,
+                const char *section,
+                unsigned section_line,
+                const char *lvalue,
+                int ltype,
+                const char *rvalue,
+                void *data,
+                void *userdata) {
+
+        _cleanup_(mdb_entry_free_or_set_invalidp) MdbEntry *mdb_entry = NULL;
+        Network *network = userdata;
+        int r;
+
+        assert(filename);
+        assert(section);
+        assert(lvalue);
+        assert(rvalue);
+        assert(data);
+
+        r = mdb_entry_new_static(network, filename, section_line, &mdb_entry);
+        if (r < 0)
+                return log_oom();
+
+        r = config_parse_vlanid(unit, filename, line, section,
+                                section_line, lvalue, ltype,
+                                rvalue, &mdb_entry->vlan_id, userdata);
+        if (r < 0)
+                return r;
+
+        mdb_entry = NULL;
+
+        return 0;
+}
+
+/* parse the multicast group from config files. */
+int config_parse_mdb_group_address(
+                const char *unit,
+                const char *filename,
+                unsigned line,
+                const char *section,
+                unsigned section_line,
+                const char *lvalue,
+                int ltype,
+                const char *rvalue,
+                void *data,
+                void *userdata) {
+
+        _cleanup_(mdb_entry_free_or_set_invalidp) MdbEntry *mdb_entry = NULL;
+        Network *network = userdata;
+        int r;
+
+        assert(filename);
+        assert(section);
+        assert(lvalue);
+        assert(rvalue);
+        assert(data);
+
+        r = mdb_entry_new_static(network, filename, section_line, &mdb_entry);
+        if (r < 0)
+                return log_oom();
+
+        r = in_addr_from_string_auto(rvalue, &mdb_entry->family, &mdb_entry->group_addr);
+        if (r < 0) {
+                log_syntax(unit, LOG_WARNING, filename, line, r, "Cannot parse multicast group address: %m");
+                return 0;
+        }
+
+        mdb_entry = NULL;
+
+        return 0;
+}
+
+int mdb_entry_verify(MdbEntry *mdb_entry) {
+        if (section_is_invalid(mdb_entry->section))
+                return -EINVAL;
+
+        if (mdb_entry->family == AF_UNSPEC)
+                return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
+                                         "%s: [BridgeMDB] section without MulticastGroupAddress= field configured. "
+                                         "Ignoring [BridgeMDB] section from line %u.",
+                                         mdb_entry->section->filename, mdb_entry->section->line);
+
+        if (!in_addr_is_multicast(mdb_entry->family, &mdb_entry->group_addr))
+                return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
+                                         "%s: MulticastGroupAddress= is not a multicast address. "
+                                         "Ignoring [BridgeMDB] section from line %u.",
+                                         mdb_entry->section->filename, mdb_entry->section->line);
+
+        if (mdb_entry->family == AF_INET) {
+                if (in4_addr_is_local_multicast(&mdb_entry->group_addr.in))
+                        return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
+                                                 "%s: MulticastGroupAddress= is a local multicast address. "
+                                                 "Ignoring [BridgeMDB] section from line %u.",
+                                                 mdb_entry->section->filename, mdb_entry->section->line);
+        } else {
+                if (in6_addr_is_link_local_all_nodes(&mdb_entry->group_addr.in6))
+                        return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
+                                                 "%s: MulticastGroupAddress= is the multicast all nodes address. "
+                                                 "Ignoring [BridgeMDB] section from line %u.",
+                                                 mdb_entry->section->filename, mdb_entry->section->line);
+        }
+
+        return 0;
+}

src/network/networkd-mdb.h

@@ -24,7 +24,7 @@ struct MdbEntry {
 
 int mdb_entry_verify(MdbEntry *mdb_entry);
 MdbEntry *mdb_entry_free(MdbEntry *mdb_entry);
-int mdb_entry_configure(Link *link, MdbEntry *mdb_entry);
+int link_set_bridge_mdb(Link *link);
 
 DEFINE_NETWORK_SECTION_FUNCTIONS(MdbEntry, mdb_entry_free);
 

src/network/networkd-network.c

@@ -306,6 +306,14 @@ int network_verify(Network *network) {
                 if (section_is_invalid(fdb->section))
                         fdb_entry_free(fdb);
 
+        if (!LIST_IS_EMPTY(network->static_mdb_entries) && !network->bridge) {
+                log_warning("%s: Cannot configure MDB entries on non-bridge port, ignoring [BridgeMDB] sections.",
+                            network->filename);
+
+                while ((mdb = network->static_mdb_entries))
+                        mdb_entry_free(mdb);
+        }
+
         LIST_FOREACH_SAFE(static_mdb_entries, mdb, mdb_next, network->static_mdb_entries)
                 if (mdb_entry_verify(mdb) < 0)
                         mdb_entry_free(mdb);

test/test-network/conf/26-bridge-mdb-master.network

@@ -0,0 +1,5 @@
+[Match]
+Name=bridge99
+
+[Network]
+IPv6AcceptRA=false

test/test-network/conf/26-bridge-mdb-slave.network

@@ -0,0 +1,14 @@
+[Match]
+Name=test1
+
+[Network]
+IPv6AcceptRA=no
+Bridge=bridge99
+
+[BridgeMDB]
+VLANId=4064
+MulticastGroupAddress=ff02:aaaa:fee5:0000:0000:0000:0001:0003
+
+[BridgeMDB]
+VLANId=4065
+MulticastGroupAddress=224.0.1.1

test/test-network/systemd-networkd-tests.py

@@ -2870,6 +2870,8 @@ class NetworkdBridgeTests(unittest.TestCase, Utilities):
         '12-dummy.netdev',
         '26-bridge.netdev',
         '26-bridge-configure-without-carrier.network',
+        '26-bridge-mdb-master.network',
+        '26-bridge-mdb-slave.network',
         '26-bridge-slave-interface-1.network',
         '26-bridge-slave-interface-2.network',
         '26-bridge-vlan-master.network',
@@ -2910,6 +2912,17 @@ class NetworkdBridgeTests(unittest.TestCase, Utilities):
             self.assertRegex(output, f'{i}')
         self.assertNotRegex(output, '4095')
 
+    def test_bridge_mdb(self):
+        copy_unit_to_networkd_unit_path('11-dummy.netdev', '26-bridge-mdb-slave.network',
+                                        '26-bridge.netdev', '26-bridge-mdb-master.network')
+        start_networkd()
+        self.wait_online(['test1:enslaved', 'bridge99:degraded'])
+
+        output = check_output('bridge mdb show dev bridge99')
+        print(output)
+        self.assertRegex(output, 'dev bridge99 port test1 grp ff02:aaaa:fee5::1:3 permanent *vid 4064')
+        self.assertRegex(output, 'dev bridge99 port test1 grp 224.0.1.1 permanent *vid 4065')
+
     def test_bridge_property(self):
         copy_unit_to_networkd_unit_path('11-dummy.netdev', '12-dummy.netdev', '26-bridge.netdev',
                                         '26-bridge-slave-interface-1.network', '26-bridge-slave-interface-2.network',