network/brvlan: remove unnecessary bridge vlan IDs

When an interface is being reconfigured with different bridge vlan
settings, this makes old vlan IDs on the interface removed.

This also makes the PVID= setting support negative boolean value, e.g. "no",
in which case, the currently assigned PVID (typically, assigned by the
kernel when an interface is joined to a bridge) is dropped.
This feature is requested by #15291.

Note, if a .network file has no settings about bridge vlan, networkd
keeps the currently assigned vlan IDs. That's intended, to make not
break existing setups.
When a .network file has only PVID=no line in [BridgeVLAN] section, then
all assigned vlan IDs are removed.

Fixes #29975.
Closes #15291.

diff --git a/man/systemd.network.xml b/man/systemd.network.xml
index 886258fbd047ac9e87aa3205cd4fea74282e9de5..045ad271f8cb4b73e8f4c7b95f1d5922d972879b 100644 (file)
--- a/man/systemd.network.xml
+++ b/man/systemd.network.xml
@@ -5880,9 +5880,11 @@ ServerAddress=192.168.0.1/24</programlisting>
         <varlistentry>
           <term><varname>PVID=</varname></term>
           <listitem>
-            <para>The Port VLAN ID specified here is assigned to all untagged frames at ingress.
-            <varname>PVID=</varname> can be used only once. Configuring <varname>PVID=</varname> implicates the use of
-            <varname>VLAN=</varname> above and will enable the VLAN ID for ingress as well.</para>
+            <para>The port VLAN ID specified here is assigned to all untagged frames at ingress. Takes an
+            VLAN ID or negative boolean value (e.g. <literal>no</literal>). When false, the currently
+            assigned port VLAN ID will be dropped. Configuring <varname>PVID=</varname> implicates the use of
+            <varname>VLAN=</varname> setting in the above and will enable the VLAN ID for ingress as well.
+            Defaults to unset, and will keep the assigned port VLAN ID if exists.</para>
 
             <xi:include href="version-info.xml" xpointer="v231"/>
           </listitem>

diff --git a/src/network/networkd-bridge-vlan.c b/src/network/networkd-bridge-vlan.c
index c38ae16a47ddcee45d684e39ea50b060184cb309..d84b6b34f6376841a9dfc8f940345ae7e6413799 100644 (file)
--- a/src/network/networkd-bridge-vlan.c
+++ b/src/network/networkd-bridge-vlan.c
@@ -72,18 +72,42 @@ static int add_range(sd_netlink_message *m, uint16_t begin, uint16_t end, bool u
         return 0;
 }
 
+static uint16_t link_get_pvid(Link *link, bool *ret_untagged) {
+        assert(link);
+        assert(link->network);
+
+        if (vlanid_is_valid(link->network->bridge_vlan_pvid)) {
+                if (ret_untagged)
+                        *ret_untagged = is_bit_set(link->network->bridge_vlan_pvid,
+                                                   link->network->bridge_vlan_untagged_bitmap);
+                return link->network->bridge_vlan_pvid;
+        }
+
+        if (link->network->bridge_vlan_pvid == BRIDGE_VLAN_KEEP_PVID) {
+                if (ret_untagged)
+                        *ret_untagged = link->bridge_vlan_pvid_is_untagged;
+                return link->bridge_vlan_pvid;
+        }
+
+        if (ret_untagged)
+                *ret_untagged = false;
+        return UINT16_MAX;
+}
+
 static int bridge_vlan_append_set_info(Link *link, sd_netlink_message *m) {
-        uint16_t begin = UINT16_MAX;
-        bool untagged;
+        uint16_t pvid, begin = UINT16_MAX;
+        bool untagged, pvid_is_untagged;
         int r;
 
         assert(link);
         assert(link->network);
         assert(m);
 
+        pvid = link_get_pvid(link, &pvid_is_untagged);
+
         for (uint16_t k = 0; k < BRIDGE_VLAN_BITMAP_MAX; k++) {
 
-                if (k == link->network->bridge_vlan_pvid) {
+                if (k == pvid) {
                         /* PVID needs to be sent alone. Finish previous bits. */
                         if (begin != UINT16_MAX) {
                                 assert(begin < k);
@@ -95,8 +119,7 @@ static int bridge_vlan_append_set_info(Link *link, sd_netlink_message *m) {
                                 begin = UINT16_MAX;
                         }
 
-                        untagged = is_bit_set(k, link->network->bridge_vlan_untagged_bitmap);
-                        r = add_single(m, k, untagged, /* is_pvid = */ true);
+                        r = add_single(m, pvid, pvid_is_untagged, /* is_pvid = */ true);
                         if (r < 0)
                                 return r;
 
@@ -150,7 +173,48 @@ static int bridge_vlan_append_set_info(Link *link, sd_netlink_message *m) {
         return 0;
 }
 
-int bridge_vlan_set_message(Link *link, sd_netlink_message *m) {
+static int bridge_vlan_append_del_info(Link *link, sd_netlink_message *m) {
+        uint16_t pvid, begin = UINT16_MAX;
+        int r;
+
+        assert(link);
+        assert(link->network);
+        assert(m);
+
+        pvid = link_get_pvid(link, NULL);
+
+        for (uint16_t k = 0; k < BRIDGE_VLAN_BITMAP_MAX; k++) {
+
+                if (k == pvid ||
+                    !is_bit_set(k, link->bridge_vlan_bitmap) ||
+                    is_bit_set(k, link->network->bridge_vlan_bitmap)) {
+                        /* This bit is not necessary to be removed. Finish previous bits. */
+                        if (begin != UINT16_MAX) {
+                                assert(begin < k);
+
+                                r = add_range(m, begin, k - 1, /* untagged = */ false);
+                                if (r < 0)
+                                        return r;
+
+                                begin = UINT16_MAX;
+                        }
+
+                        continue;
+                }
+
+                if (begin != UINT16_MAX)
+                        continue;
+
+                /* This is the starting point of a new bit sequence. Save the position. */
+                begin = k;
+        }
+
+        /* No pending bit sequence. */
+        assert(begin == UINT16_MAX);
+        return 0;
+}
+
+int bridge_vlan_set_message(Link *link, sd_netlink_message *m, bool is_set) {
         int r;
 
         assert(link);
@@ -171,7 +235,10 @@ int bridge_vlan_set_message(Link *link, sd_netlink_message *m) {
                         return r;
         }
 
-        r = bridge_vlan_append_set_info(link, m);
+        if (is_set)
+                r = bridge_vlan_append_set_info(link, m);
+        else
+                r = bridge_vlan_append_del_info(link, m);
         if (r < 0)
                 return r;
 
@@ -275,7 +342,12 @@ int config_parse_bridge_vlan_id(
         assert(rvalue);
 
         if (isempty(rvalue)) {
-                *id = UINT16_MAX;
+                *id = BRIDGE_VLAN_KEEP_PVID;
+                return 0;
+        }
+
+        if (parse_boolean(rvalue) == 0) {
+                *id = BRIDGE_VLAN_REMOVE_PVID;
                 return 0;
         }
 

diff --git a/src/network/networkd-bridge-vlan.h b/src/network/networkd-bridge-vlan.h
index 43d365e7457e3dfdcbd0b748b3b9b58e94a017e2..0366cc6feea33c4915f5383fe3705794c65180b6 100644 (file)
--- a/src/network/networkd-bridge-vlan.h
+++ b/src/network/networkd-bridge-vlan.h
@@ -6,20 +6,26 @@
 ***/
 
 #include <inttypes.h>
+#include <stdbool.h>
 
 #include "sd-netlink.h"
 
 #include "conf-parser.h"
+#include "vlan-util.h"
 
 #define BRIDGE_VLAN_BITMAP_MAX 4096
 #define BRIDGE_VLAN_BITMAP_LEN (BRIDGE_VLAN_BITMAP_MAX / 32)
 
+#define BRIDGE_VLAN_KEEP_PVID   UINT16_MAX
+#define BRIDGE_VLAN_REMOVE_PVID (UINT16_MAX - 1)
+assert_cc(BRIDGE_VLAN_REMOVE_PVID > VLANID_MAX);
+
 typedef struct Link Link;
 typedef struct Network Network;
 
 void network_adjust_bridge_vlan(Network *network);
 
-int bridge_vlan_set_message(Link *link, sd_netlink_message *m);
+int bridge_vlan_set_message(Link *link, sd_netlink_message *m, bool is_set);
 
 int link_update_bridge_vlan(Link *link, sd_netlink_message *m);
 

diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h
index a6c2f2d72026c1db7250dcdac925e6a1d9c7e525..7458ea93bd0824d7b8cb651f237f7e75676f4f59 100644 (file)
--- a/src/network/networkd-link.h
+++ b/src/network/networkd-link.h
@@ -155,6 +155,7 @@ typedef struct Link {
         bool activated:1;
         bool master_set:1;
         bool stacked_netdevs_created:1;
+        bool bridge_vlan_set:1;
 
         sd_dhcp_server *dhcp_server;
 

diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c
index 654fd7bb9eefd09d53dbedcf00c0c0d8ea0ed285..d4401757bd28900bf19d72fad6c8e5a6b06f0d66 100644 (file)
--- a/src/network/networkd-network.c
+++ b/src/network/networkd-network.c
@@ -450,7 +450,7 @@ int network_load_one(Manager *manager, OrderedHashmap **networks, const char *fi
                 .priority = LINK_BRIDGE_PORT_PRIORITY_INVALID,
                 .multicast_router = _MULTICAST_ROUTER_INVALID,
 
-                .bridge_vlan_pvid = UINT16_MAX,
+                .bridge_vlan_pvid = BRIDGE_VLAN_KEEP_PVID,
 
                 .lldp_mode = LLDP_MODE_ROUTERS_ONLY,
                 .lldp_multicast_mode = _SD_LLDP_MULTICAST_MODE_INVALID,

diff --git a/src/network/networkd-queue.c b/src/network/networkd-queue.c
index 90caefbc72c82143c467feaf5a2179ee75772930..cc439a74a83bc5b747ae86407277e44c12dbf17b 100644 (file)
--- a/src/network/networkd-queue.c
+++ b/src/network/networkd-queue.c
@@ -303,7 +303,8 @@ static const char *const request_type_table[_REQUEST_TYPE_MAX] = {
         [REQUEST_TYPE_SET_LINK_ADDRESS_GENERATION_MODE] = "IPv6LL address generation mode",
         [REQUEST_TYPE_SET_LINK_BOND]                    = "bond configurations",
         [REQUEST_TYPE_SET_LINK_BRIDGE]                  = "bridge configurations",
-        [REQUEST_TYPE_SET_LINK_BRIDGE_VLAN]             = "bridge VLAN configurations",
+        [REQUEST_TYPE_SET_LINK_BRIDGE_VLAN]             = "bridge VLAN configurations (step 1)",
+        [REQUEST_TYPE_DEL_LINK_BRIDGE_VLAN]             = "bridge VLAN configurations (step 2)",
         [REQUEST_TYPE_SET_LINK_CAN]                     = "CAN interface configurations",
         [REQUEST_TYPE_SET_LINK_FLAGS]                   = "link flags",
         [REQUEST_TYPE_SET_LINK_GROUP]                   = "interface group",

diff --git a/src/network/networkd-queue.h b/src/network/networkd-queue.h
index e58d1be66f5542ea12a96fd3e44d4d8bd6033142..d6f5de421e2a125d50611383d6a7ac224e3b24e2 100644 (file)
--- a/src/network/networkd-queue.h
+++ b/src/network/networkd-queue.h
@@ -37,6 +37,7 @@ typedef enum RequestType {
         REQUEST_TYPE_SET_LINK_BOND,                    /* Setting bond configs. */
         REQUEST_TYPE_SET_LINK_BRIDGE,                  /* Setting bridge configs. */
         REQUEST_TYPE_SET_LINK_BRIDGE_VLAN,             /* Setting bridge VLAN configs. */
+        REQUEST_TYPE_DEL_LINK_BRIDGE_VLAN,             /* Removing bridge VLAN configs. */
         REQUEST_TYPE_SET_LINK_CAN,                     /* Setting CAN interface configs. */
         REQUEST_TYPE_SET_LINK_FLAGS,                   /* Setting IFF_NOARP or friends. */
         REQUEST_TYPE_SET_LINK_GROUP,                   /* Setting interface group. */

diff --git a/src/network/networkd-setlink.c b/src/network/networkd-setlink.c
index 3e1d904836f5bdd1b32b94379a2a6488e87f92ba..2b37c86d2351a791aca2cf26d12a01f2579c725f 100644 (file)
--- a/src/network/networkd-setlink.c
+++ b/src/network/networkd-setlink.c
@@ -103,6 +103,19 @@ static int link_set_bridge_handler(sd_netlink *rtnl, sd_netlink_message *m, Requ
 }
 
 static int link_set_bridge_vlan_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, void *userdata) {
+        int r;
+
+        assert(link);
+
+        r = set_link_handler_internal(rtnl, m, req, link, /* ignore = */ false, NULL);
+        if (r <= 0)
+                return r;
+
+        link->bridge_vlan_set = true;
+        return 0;
+}
+
+static int link_del_bridge_vlan_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, void *userdata) {
         return set_link_handler_internal(rtnl, m, req, link, /* ignore = */ false, NULL);
 }
 
@@ -326,7 +339,12 @@ static int link_configure_fill_message(
                         return r;
                 break;
         case REQUEST_TYPE_SET_LINK_BRIDGE_VLAN:
-                r = bridge_vlan_set_message(link, req);
+                r = bridge_vlan_set_message(link, req, /* is_set = */ true);
+                if (r < 0)
+                        return r;
+                break;
+        case REQUEST_TYPE_DEL_LINK_BRIDGE_VLAN:
+                r = bridge_vlan_set_message(link, req, /* is_set = */ false);
                 if (r < 0)
                         return r;
                 break;
@@ -410,6 +428,8 @@ static int link_configure(Link *link, Request *req) {
                 r = sd_rtnl_message_new_link(link->manager->rtnl, &m, RTM_NEWLINK, link->master_ifindex);
         else if (IN_SET(req->type, REQUEST_TYPE_SET_LINK_CAN, REQUEST_TYPE_SET_LINK_IPOIB))
                 r = sd_rtnl_message_new_link(link->manager->rtnl, &m, RTM_NEWLINK, link->ifindex);
+        else if (req->type == REQUEST_TYPE_DEL_LINK_BRIDGE_VLAN)
+                r = sd_rtnl_message_new_link(link->manager->rtnl, &m, RTM_DELLINK, link->ifindex);
         else
                 r = sd_rtnl_message_new_link(link->manager->rtnl, &m, RTM_SETLINK, link->ifindex);
         if (r < 0)
@@ -460,9 +480,11 @@ static int link_is_ready_to_set_link(Link *link, Request *req) {
 
                 if (link->network->keep_master && link->master_ifindex <= 0 && !streq_ptr(link->kind, "bridge"))
                         return false;
-
                 break;
 
+        case REQUEST_TYPE_DEL_LINK_BRIDGE_VLAN:
+                return link->bridge_vlan_set;
+
         case REQUEST_TYPE_SET_LINK_CAN:
                 /* Do not check link->set_flgas_messages here, as it is ok even if link->flags
                  * is outdated, and checking the counter causes a deadlock. */
@@ -684,11 +706,14 @@ int link_request_to_set_bridge(Link *link) {
 }
 
 int link_request_to_set_bridge_vlan(Link *link) {
+        int r;
+
         assert(link);
         assert(link->network);
 
         /* If nothing configured, use the default vlan ID. */
-        if (memeqzero(link->network->bridge_vlan_bitmap, BRIDGE_VLAN_BITMAP_LEN * sizeof(uint32_t)))
+        if (memeqzero(link->network->bridge_vlan_bitmap, BRIDGE_VLAN_BITMAP_LEN * sizeof(uint32_t)) &&
+            link->network->bridge_vlan_pvid == BRIDGE_VLAN_KEEP_PVID)
                 return 0;
 
         if (!link->network->bridge && !streq_ptr(link->kind, "bridge")) {
@@ -704,9 +729,21 @@ int link_request_to_set_bridge_vlan(Link *link) {
                         return 0;
         }
 
-        return link_request_set_link(link, REQUEST_TYPE_SET_LINK_BRIDGE_VLAN,
-                                     link_set_bridge_vlan_handler,
-                                     NULL);
+        link->bridge_vlan_set = false;
+
+        r = link_request_set_link(link, REQUEST_TYPE_SET_LINK_BRIDGE_VLAN,
+                                  link_set_bridge_vlan_handler,
+                                  NULL);
+        if (r < 0)
+                return r;
+
+        r = link_request_set_link(link, REQUEST_TYPE_DEL_LINK_BRIDGE_VLAN,
+                                  link_del_bridge_vlan_handler,
+                                  NULL);
+        if (r < 0)
+                return r;
+
+        return 0;
 }
 
 int link_request_to_set_can(Link *link) {