From e00c81b1536cdfb72fa4776a7b0768b573c58009 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 25 Jul 2024 09:49:18 +0200 Subject: [PATCH 1/5] bridge: change the signature for nm_platform_link_set_bridge_vlans() Currently, nm_platform_link_set_bridge_vlans() accepts an array of pointers to vlan objects; to avoid multiple allocations, setting_vlans_to_platform() creates the array by piggybacking the actual data after the pointers array. In the next commits, the array will need to be manipulated and extended, which is difficult with the current structure. Instead, pass separately an array of objects and its size. --- src/core/devices/nm-device-bridge.c | 64 +++++++++++++------------- src/libnm-platform/nm-linux-platform.c | 19 ++++---- src/libnm-platform/nm-platform.c | 32 ++++++------- src/libnm-platform/nm-platform.h | 18 ++++---- 4 files changed, 70 insertions(+), 63 deletions(-) diff --git a/src/core/devices/nm-device-bridge.c b/src/core/devices/nm-device-bridge.c index b256056d43..cde94a4264 100644 --- a/src/core/devices/nm-device-bridge.c +++ b/src/core/devices/nm-device-bridge.c @@ -419,20 +419,18 @@ static const Option controller_options[] = { 0, }}; -static const NMPlatformBridgeVlan ** -setting_vlans_to_platform(GPtrArray *array) +static NMPlatformBridgeVlan * +setting_vlans_to_platform(GPtrArray *array, guint *out_len) { - NMPlatformBridgeVlan **arr; - NMPlatformBridgeVlan *p_data; - guint i; + NMPlatformBridgeVlan *arr; + guint i; - if (!array || !array->len) + if (!array || !array->len) { + *out_len = 0; return NULL; + } - G_STATIC_ASSERT_EXPR(_nm_alignof(NMPlatformBridgeVlan *) >= _nm_alignof(NMPlatformBridgeVlan)); - arr = g_malloc((sizeof(NMPlatformBridgeVlan *) * (array->len + 1)) - + (sizeof(NMPlatformBridgeVlan) * (array->len))); - p_data = (NMPlatformBridgeVlan *) &arr[array->len + 1]; + arr = g_new(NMPlatformBridgeVlan, array->len); for (i = 0; i < array->len; i++) { NMBridgeVlan *vlan = array->pdata[i]; @@ -440,16 +438,16 @@ setting_vlans_to_platform(GPtrArray *array) nm_bridge_vlan_get_vid_range(vlan, &vid_start, &vid_end); - p_data[i] = (NMPlatformBridgeVlan){ + arr[i] = (NMPlatformBridgeVlan){ .vid_start = vid_start, .vid_end = vid_end, .pvid = nm_bridge_vlan_is_pvid(vlan), .untagged = nm_bridge_vlan_is_untagged(vlan), }; - arr[i] = &p_data[i]; } - arr[i] = NULL; - return (const NMPlatformBridgeVlan **) arr; + + *out_len = array->len; + return arr; } static void @@ -639,15 +637,16 @@ is_bridge_pvid_changed(NMDevice *device, NMSettingBridge *s_bridge) static gboolean bridge_set_vlan_options(NMDevice *device, NMSettingBridge *s_bridge, gboolean is_reapply) { - NMDeviceBridge *self = NM_DEVICE_BRIDGE(device); - gconstpointer hwaddr; - size_t length; - gboolean enabled; - guint16 pvid; - NMPlatform *plat; - int ifindex; - gs_unref_ptrarray GPtrArray *vlans = NULL; - gs_free const NMPlatformBridgeVlan **plat_vlans = NULL; + NMDeviceBridge *self = NM_DEVICE_BRIDGE(device); + gconstpointer hwaddr; + size_t length; + gboolean enabled; + guint16 pvid; + NMPlatform *plat; + int ifindex; + gs_unref_ptrarray GPtrArray *vlans = NULL; + gs_free NMPlatformBridgeVlan *plat_vlans = NULL; + guint num_vlans; if (self->vlan_configured) return TRUE; @@ -664,7 +663,7 @@ bridge_set_vlan_options(NMDevice *device, NMSettingBridge *s_bridge, gboolean is .vlan_filtering_val = FALSE, .vlan_default_pvid_has = TRUE, .vlan_default_pvid_val = 1})); - nm_platform_link_set_bridge_vlans(plat, ifindex, FALSE, NULL); + nm_platform_link_set_bridge_vlans(plat, ifindex, FALSE, NULL, 0); return TRUE; } @@ -696,7 +695,7 @@ bridge_set_vlan_options(NMDevice *device, NMSettingBridge *s_bridge, gboolean is .vlan_default_pvid_val = 0})); /* Clear all existing VLANs */ - if (!nm_platform_link_set_bridge_vlans(plat, ifindex, FALSE, NULL)) + if (!nm_platform_link_set_bridge_vlans(plat, ifindex, FALSE, NULL, 0)) return FALSE; /* Now set the default PVID. After this point the kernel creates @@ -714,8 +713,9 @@ bridge_set_vlan_options(NMDevice *device, NMSettingBridge *s_bridge, gboolean is /* Create VLANs only after setting the default PVID, so that * any PVID VLAN overrides the bridge's default PVID. */ g_object_get(s_bridge, NM_SETTING_BRIDGE_VLANS, &vlans, NULL); - plat_vlans = setting_vlans_to_platform(vlans); - if (plat_vlans && !nm_platform_link_set_bridge_vlans(plat, ifindex, FALSE, plat_vlans)) + plat_vlans = setting_vlans_to_platform(vlans, &num_vlans); + if (plat_vlans + && !nm_platform_link_set_bridge_vlans(plat, ifindex, FALSE, plat_vlans, num_vlans)) return FALSE; nm_platform_link_set_bridge_info(plat, @@ -937,13 +937,14 @@ attach_port(NMDevice *device, bridge_set_vlan_options(device, s_bridge, FALSE); if (nm_setting_bridge_get_vlan_filtering(s_bridge)) { - gs_free const NMPlatformBridgeVlan **plat_vlans = NULL; - gs_unref_ptrarray GPtrArray *vlans = NULL; + gs_free NMPlatformBridgeVlan *plat_vlans = NULL; + gs_unref_ptrarray GPtrArray *vlans = NULL; + guint num_vlans; if (s_port) g_object_get(s_port, NM_SETTING_BRIDGE_PORT_VLANS, &vlans, NULL); - plat_vlans = setting_vlans_to_platform(vlans); + plat_vlans = setting_vlans_to_platform(vlans, &num_vlans); /* Since the link was just enportd, there are no existing VLANs * (except for the default one) and so there's no need to flush. */ @@ -952,7 +953,8 @@ attach_port(NMDevice *device, && !nm_platform_link_set_bridge_vlans(nm_device_get_platform(port), nm_device_get_ifindex(port), TRUE, - plat_vlans)) + plat_vlans, + num_vlans)) return FALSE; } diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 65f42d8a27..9a1920555d 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -9505,17 +9505,20 @@ nla_put_failure: } static gboolean -link_set_bridge_vlans(NMPlatform *platform, - int ifindex, - gboolean on_controller, - const NMPlatformBridgeVlan *const *vlans) +link_set_bridge_vlans(NMPlatform *platform, + int ifindex, + gboolean on_controller, + const NMPlatformBridgeVlan *vlans, + guint num_vlans) { nm_auto_nlmsg struct nl_msg *nlmsg = NULL; struct nlattr *list; struct bridge_vlan_info vinfo = {}; guint i; - nlmsg = _nl_msg_new_link_full(vlans ? RTM_SETLINK : RTM_DELLINK, + nm_assert(num_vlans == 0 || vlans); + + nlmsg = _nl_msg_new_link_full(num_vlans > 0 ? RTM_SETLINK : RTM_DELLINK, 0, ifindex, NULL, @@ -9533,10 +9536,10 @@ link_set_bridge_vlans(NMPlatform *platform, IFLA_BRIDGE_FLAGS, on_controller ? BRIDGE_FLAGS_CONTROLLER : BRIDGE_FLAGS_SELF); - if (vlans) { + if (num_vlans > 0) { /* Add VLANs */ - for (i = 0; vlans[i]; i++) { - const NMPlatformBridgeVlan *vlan = vlans[i]; + for (i = 0; i < num_vlans; i++) { + const NMPlatformBridgeVlan *vlan = &vlans[i]; gboolean is_range = vlan->vid_start != vlan->vid_end; vinfo.vid = vlan->vid_start; diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index f82de7f9d7..670f56bb2b 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -2070,10 +2070,11 @@ nm_platform_link_set_sriov_vfs(NMPlatform *self, int ifindex, const NMPlatformVF } gboolean -nm_platform_link_set_bridge_vlans(NMPlatform *self, - int ifindex, - gboolean on_controller, - const NMPlatformBridgeVlan *const *vlans) +nm_platform_link_set_bridge_vlans(NMPlatform *self, + int ifindex, + gboolean on_controller, + const NMPlatformBridgeVlan *vlans, + guint num_vlans) { guint i; _CHECK_SELF(self, klass, FALSE); @@ -2085,9 +2086,9 @@ nm_platform_link_set_bridge_vlans(NMPlatform *self, vlans ? "setting" : "clearing", on_controller ? "controller" : "self"); if (vlans) { - for (i = 0; vlans[i]; i++) { + for (i = 0; i < num_vlans; i++) { char sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE]; - const NMPlatformBridgeVlan *vlan = vlans[i]; + const NMPlatformBridgeVlan *vlan = &vlans[i]; _LOG3D("link: bridge VLAN %s", nm_platform_bridge_vlan_to_string(vlan, sbuf, sizeof(sbuf))); @@ -2095,7 +2096,7 @@ nm_platform_link_set_bridge_vlans(NMPlatform *self, } } - return klass->link_set_bridge_vlans(self, ifindex, on_controller, vlans); + return klass->link_set_bridge_vlans(self, ifindex, on_controller, vlans, num_vlans); } gboolean @@ -6148,9 +6149,9 @@ nm_platform_link_to_string(const NMPlatformLink *link, char *buf, gsize len) link->initialized ? " init" : " not-init", link->inet6_addr_gen_mode_inv ? " addrgenmode " : "", link->inet6_addr_gen_mode_inv ? nm_platform_link_inet6_addrgenmode2str( - _nm_platform_uint8_inv(link->inet6_addr_gen_mode_inv), - str_addrmode, - sizeof(str_addrmode)) + _nm_platform_uint8_inv(link->inet6_addr_gen_mode_inv), + str_addrmode, + sizeof(str_addrmode)) : "", str_address[0] ? " addr " : "", str_address[0] ? str_address : "", @@ -7386,12 +7387,11 @@ nm_platform_ip6_route_to_string(const NMPlatformIP6Route *route, char *buf, gsiz route->lock_mtu ? "lock " : "", route->mtu) : "", - route->rt_pref - ? nm_sprintf_buf( - str_pref, - " pref %s", - nm_icmpv6_router_pref_to_string(route->rt_pref, str_pref2, sizeof(str_pref2))) - : ""); + route->rt_pref ? nm_sprintf_buf( + str_pref, + " pref %s", + nm_icmpv6_router_pref_to_string(route->rt_pref, str_pref2, sizeof(str_pref2))) + : ""); return buf; } diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index b05b1297a5..7754fba953 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -1185,10 +1185,11 @@ typedef struct { gpointer callback_data, GCancellable *cancellable); gboolean (*link_set_sriov_vfs)(NMPlatform *self, int ifindex, const NMPlatformVF *const *vfs); - gboolean (*link_set_bridge_vlans)(NMPlatform *self, - int ifindex, - gboolean on_controller, - const NMPlatformBridgeVlan *const *vlans); + gboolean (*link_set_bridge_vlans)(NMPlatform *self, + int ifindex, + gboolean on_controller, + const NMPlatformBridgeVlan *vlans, + guint num_vlans); gboolean (*link_set_bridge_info)(NMPlatform *self, int ifindex, const NMPlatformLinkSetBridgeInfoData *bridge_info); @@ -2049,10 +2050,11 @@ void nm_platform_link_set_sriov_params_async(NMPlatform *self, gboolean nm_platform_link_set_sriov_vfs(NMPlatform *self, int ifindex, const NMPlatformVF *const *vfs); -gboolean nm_platform_link_set_bridge_vlans(NMPlatform *self, - int ifindex, - gboolean on_controller, - const NMPlatformBridgeVlan *const *vlans); +gboolean nm_platform_link_set_bridge_vlans(NMPlatform *self, + int ifindex, + gboolean on_controller, + const NMPlatformBridgeVlan *vlans, + guint num_vlans); gboolean nm_platform_link_set_bridge_info(NMPlatform *self, int ifindex, const NMPlatformLinkSetBridgeInfoData *bridge_info); From c5d1e35f993e8d3aca3aac23035663cf95df840b Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 25 Jul 2024 18:32:24 +0200 Subject: [PATCH 2/5] device: support reapplying bridge-port VLANs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For now, always reapply the VLANs unconditionally, even if they didn't change in kernel. To set again the VLANs on the port we need to clear all the existing one before. However, this deletes also the VLAN for the default-pvid on the bridge. Therefore, we need some additional logic to inject the default-pvid in the list of VLANs. Co-authored-by: Íñigo Huguet --- NEWS | 1 + src/core/devices/nm-device-bridge.c | 87 +++++++++++++++++++++++++++++ src/core/devices/nm-device-bridge.h | 2 + src/core/devices/nm-device.c | 2 + 4 files changed, 92 insertions(+) diff --git a/NEWS b/NEWS index 720370ee57..daf62fd5f7 100644 --- a/NEWS +++ b/NEWS @@ -23,6 +23,7 @@ USE AT YOUR OWN RISK. NOT RECOMMENDED FOR PRODUCTION USE! * ndisc: Support multiple gateways for a single network * wifi: Support configuring channel-width in AP mode * keyfile: Stop writing offensive terms into keyfiles +* Support reapplying the VLANs on bridge ports. ============================================= NetworkManager-1.48 diff --git a/src/core/devices/nm-device-bridge.c b/src/core/devices/nm-device-bridge.c index cde94a4264..7628e4600f 100644 --- a/src/core/devices/nm-device-bridge.c +++ b/src/core/devices/nm-device-bridge.c @@ -728,6 +728,93 @@ bridge_set_vlan_options(NMDevice *device, NMSettingBridge *s_bridge, gboolean is return TRUE; } +static NMPlatformBridgeVlan * +merge_bridge_vlan_default_pvid(NMPlatformBridgeVlan *vlans, guint *num_vlans, guint default_pvid) +{ + NMPlatformBridgeVlan *vlan; + gboolean has_pvid = FALSE; + guint i; + + for (i = 0; i < *num_vlans; i++) { + if (vlans[i].pvid) { + has_pvid = TRUE; + break; + } + } + + /* search if the list of VLANs already contains the default PVID */ + vlan = NULL; + for (i = 0; i < *num_vlans; i++) { + if (default_pvid >= vlans[i].vid_start && default_pvid <= vlans[i].vid_end) { + vlan = &vlans[i]; + break; + } + } + + if (!vlan) { + /* VLAN id not found, append the default PVID at the end. + * Set the PVID flag only if the port didn't have one. */ + vlans = g_realloc_n(vlans, *num_vlans + 1, sizeof(NMPlatformBridgeVlan)); + (*num_vlans)++; + vlans[*num_vlans - 1] = (NMPlatformBridgeVlan){ + .vid_start = default_pvid, + .vid_end = default_pvid, + .untagged = TRUE, + .pvid = !has_pvid, + }; + } + + return vlans; +} + +void +nm_device_reapply_bridge_port_vlans(NMDevice *device) +{ + NMSettingBridgePort *s_bridge_port; + NMDevice *controller; + NMSettingBridge *s_bridge; + gs_unref_ptrarray GPtrArray *tmp_vlans = NULL; + gs_free NMPlatformBridgeVlan *setting_vlans = NULL; + guint num_setting_vlans = 0; + NMPlatform *plat; + int ifindex; + + s_bridge_port = nm_device_get_applied_setting(device, NM_TYPE_SETTING_BRIDGE_PORT); + if (!s_bridge_port) + return; + + controller = nm_device_get_controller(device); + if (!controller) + return; + + s_bridge = nm_device_get_applied_setting(controller, NM_TYPE_SETTING_BRIDGE); + if (!s_bridge) + return; + + if (nm_setting_bridge_get_vlan_filtering(s_bridge)) { + g_object_get(s_bridge_port, NM_SETTING_BRIDGE_PORT_VLANS, &tmp_vlans, NULL); + setting_vlans = setting_vlans_to_platform(tmp_vlans, &num_setting_vlans); + + /* During a regular activation, we first set the default_pvid on the bridge + * (which creates the PVID VLAN on the port) and then add the VLANs on the port. + * This ensures that the PVID VLAN is inherited from the bridge, but it's + * overridden if the port specifies one. + * During a reapply on the port, we are not going to touch the bridge and + * so we need to merge manually the PVID from the bridge with the port VLANs. */ + setting_vlans = + merge_bridge_vlan_default_pvid(setting_vlans, + &num_setting_vlans, + nm_setting_bridge_get_vlan_default_pvid(s_bridge)); + } + + plat = nm_device_get_platform(device); + ifindex = nm_device_get_ifindex(device); + + nm_platform_link_set_bridge_vlans(plat, ifindex, TRUE, NULL, 0); + if (num_setting_vlans > 0) + nm_platform_link_set_bridge_vlans(plat, ifindex, TRUE, setting_vlans, num_setting_vlans); +} + static void _platform_lnk_bridge_init_from_setting(NMSettingBridge *s_bridge, NMPlatformLnkBridge *props) { diff --git a/src/core/devices/nm-device-bridge.h b/src/core/devices/nm-device-bridge.h index 6d9f1614c8..f9be7580bb 100644 --- a/src/core/devices/nm-device-bridge.h +++ b/src/core/devices/nm-device-bridge.h @@ -27,4 +27,6 @@ extern const NMBtVTableNetworkServer *nm_bt_vtable_network_server; void _nm_device_bridge_notify_unregister_bt_nap(NMDevice *device, const char *reason); +void nm_device_reapply_bridge_port_vlans(NMDevice *device); + #endif /* __NETWORKMANAGER_DEVICE_BRIDGE_H__ */ diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 2382f6588f..82141ce005 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -13881,6 +13881,8 @@ check_and_reapply_connection(NMDevice *self, if (priv->state >= NM_DEVICE_STATE_ACTIVATED) nm_device_update_metered(self); + nm_device_reapply_bridge_port_vlans(self); + sett_conn = nm_device_get_settings_connection(self); if (sett_conn) { nm_settings_connection_autoconnect_blocked_reason_set( From 7ae4660a77c686bb9238b9570ee93ebee82a972e Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 11 Jul 2024 09:03:05 +0200 Subject: [PATCH 3/5] platform: support reading bridge VLANs Add a function to read the list of bridge VLANs on an interface. --- src/libnm-platform/nm-linux-platform.c | 133 +++++++++++++++++++++++++ src/libnm-platform/nm-platform.c | 34 +++++++ src/libnm-platform/nm-platform.h | 8 ++ 3 files changed, 175 insertions(+) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 9a1920555d..83fc980748 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -9576,6 +9576,138 @@ nla_put_failure: g_return_val_if_reached(FALSE); } +typedef struct { + int ifindex; + GArray *vlans; +} BridgeVlanData; + +static int +get_bridge_vlans_cb(const struct nl_msg *msg, void *arg) +{ + static const struct nla_policy policy[] = { + [IFLA_AF_SPEC] = {.type = NLA_NESTED}, + }; + struct nlattr *tb[G_N_ELEMENTS(policy)]; + gboolean is_range = FALSE; + BridgeVlanData *data = arg; + struct ifinfomsg *ifinfo; + struct nlattr *attr; + int rem; + + if (nlmsg_parse_arr(nlmsg_hdr(msg), sizeof(struct ifinfomsg), tb, policy) < 0) + return NL_SKIP; + + ifinfo = NLMSG_DATA(nlmsg_hdr(msg)); + if (ifinfo->ifi_index != data->ifindex) + return NL_SKIP; + + if (!tb[IFLA_AF_SPEC]) + return NL_SKIP; + + nla_for_each_nested (attr, tb[IFLA_AF_SPEC], rem) { + struct bridge_vlan_info vlan_info; + NMPlatformBridgeVlan vlan = {}; + + if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO) + continue; + + if (!data->vlans) + data->vlans = g_array_new(0, FALSE, sizeof(NMPlatformBridgeVlan)); + + vlan_info = *nla_data_as(struct bridge_vlan_info, attr); + + if (is_range) { + nm_g_array_index(data->vlans, NMPlatformBridgeVlan, data->vlans->len - 1).vid_end = + vlan_info.vid; + is_range = FALSE; + continue; + } else { + vlan.vid_start = vlan_info.vid; + vlan.vid_end = vlan_info.vid; + vlan.untagged = vlan_info.flags & BRIDGE_VLAN_INFO_UNTAGGED; + vlan.pvid = vlan_info.flags & BRIDGE_VLAN_INFO_PVID; + + if (vlan_info.flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) + is_range = TRUE; + } + + g_array_append_val(data->vlans, vlan); + } + + return NL_OK; +} + +static gboolean +link_get_bridge_vlans(NMPlatform *platform, + int ifindex, + NMPlatformBridgeVlan **out_vlans, + guint *out_num_vlans) +{ + gboolean ret = FALSE; + nm_auto_nlmsg struct nl_msg *nlmsg = NULL; + struct nl_sock *sk = NULL; + BridgeVlanData data; + int nle; + + nlmsg = _nl_msg_new_link_full(RTM_GETLINK, NLM_F_DUMP, 0, NULL, AF_BRIDGE, 0, 0, 0); + if (!nlmsg) + g_return_val_if_reached(FALSE); + + nle = nl_socket_new(&sk, NETLINK_ROUTE, NL_SOCKET_FLAGS_DISABLE_MSG_PEEK, 0, 0); + if (nle < 0) { + _LOGD("get-bridge-vlan: error opening socket: %s (%d)", nm_strerror(nle), nle); + ret = FALSE; + goto err; + } + + NLA_PUT_U32(nlmsg, IFLA_EXT_MASK, RTEXT_FILTER_BRVLAN_COMPRESSED); + + nle = nl_send_auto(sk, nlmsg); + if (nle < 0) { + _LOGD("get-bridge-vlans: failed sending request: %s (%d)", nm_strerror(nle), nle); + ret = FALSE; + goto err; + } + + data = ((BridgeVlanData){ + .ifindex = ifindex, + }); + + do { + nle = nl_recvmsgs(sk, + &((const struct nl_cb){ + .valid_cb = get_bridge_vlans_cb, + .valid_arg = &data, + })); + } while (nle == -EAGAIN); + + if (nle < 0) { + _LOGD("get-bridge-vlan: recv failed: %s (%d)", nm_strerror(nle), nle); + ret = FALSE; + goto err; + } + + if (data.vlans) { + NM_SET_OUT(out_vlans, &nm_g_array_index(data.vlans, NMPlatformBridgeVlan, 0)); + NM_SET_OUT(out_num_vlans, data.vlans->len); + } else { + NM_SET_OUT(out_vlans, NULL); + NM_SET_OUT(out_num_vlans, 0); + } + + if (data.vlans) + g_array_free(data.vlans, !out_vlans); + + ret = TRUE; +err: + if (sk) + nl_socket_free(sk); + return ret; + +nla_put_failure: + g_return_val_if_reached(FALSE); +} + static gboolean link_set_bridge_info(NMPlatform *platform, int ifindex, @@ -11915,6 +12047,7 @@ nm_linux_platform_class_init(NMLinuxPlatformClass *klass) platform_class->link_set_sriov_params_async = link_set_sriov_params_async; platform_class->link_set_sriov_vfs = link_set_sriov_vfs; platform_class->link_set_bridge_vlans = link_set_bridge_vlans; + platform_class->link_get_bridge_vlans = link_get_bridge_vlans; platform_class->link_set_bridge_info = link_set_bridge_info; platform_class->link_get_physical_port_id = link_get_physical_port_id; diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index 670f56bb2b..8b9f203348 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -2099,6 +2099,40 @@ nm_platform_link_set_bridge_vlans(NMPlatform *self, return klass->link_set_bridge_vlans(self, ifindex, on_controller, vlans, num_vlans); } +gboolean +nm_platform_link_get_bridge_vlans(NMPlatform *self, + int ifindex, + NMPlatformBridgeVlan **out_vlans, + guint *out_num_vlans) +{ + char sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE]; + gboolean ret; + guint i; + + _CHECK_SELF(self, klass, FALSE); + + g_return_val_if_fail(ifindex > 0, FALSE); + g_return_val_if_fail(out_vlans, FALSE); + g_return_val_if_fail(out_num_vlans, FALSE); + + _LOG3D("link: getting bridge VLANs"); + + ret = klass->link_get_bridge_vlans(self, ifindex, out_vlans, out_num_vlans); + + if (_LOGD_ENABLED()) { + if (!ret) { + _LOG3D("link: failure while getting bridge vlans"); + } else { + for (i = 0; i < *out_num_vlans; i++) { + _LOG3D("link: bridge VLAN %s", + nm_platform_bridge_vlan_to_string(&(*out_vlans)[i], sbuf, sizeof(sbuf))); + } + } + } + + return ret; +} + gboolean nm_platform_link_set_bridge_info(NMPlatform *self, int ifindex, diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index 7754fba953..e5110ff117 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -1190,6 +1190,10 @@ typedef struct { gboolean on_controller, const NMPlatformBridgeVlan *vlans, guint num_vlans); + gboolean (*link_get_bridge_vlans)(NMPlatform *self, + int ifindex, + NMPlatformBridgeVlan **out_vlans, + guint *out_num_vlans); gboolean (*link_set_bridge_info)(NMPlatform *self, int ifindex, const NMPlatformLinkSetBridgeInfoData *bridge_info); @@ -2055,6 +2059,10 @@ gboolean nm_platform_link_set_bridge_vlans(NMPlatform *self, gboolean on_controller, const NMPlatformBridgeVlan *vlans, guint num_vlans); +gboolean nm_platform_link_get_bridge_vlans(NMPlatform *self, + int ifindex, + NMPlatformBridgeVlan **out_vlans, + guint *out_num_vlans); gboolean nm_platform_link_set_bridge_info(NMPlatform *self, int ifindex, const NMPlatformLinkSetBridgeInfoData *bridge_info); From 1c43fe52358508adefb5015be08dc8b246b256eb Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 26 Jul 2024 15:17:58 +0200 Subject: [PATCH 4/5] platform: add nmp_utils_bridge_normalized_vlans_equal() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a function to compare two arrays of NMPlatformBridgeVlan. It will be used in the next commit to compare the VLANs from platform to the ones we want to set. To compare in a performant way, the vlans need to be normalized (no duplicated VLANS, ranges into their minimal expression...). Add the function nmp_utils_bridge_vlan_normalize. Co-authored-by: Íñigo Huguet --- src/libnm-platform/nm-platform-utils.c | 83 +++++++ src/libnm-platform/nm-platform-utils.h | 7 + src/libnm-platform/nm-platform.h | 7 - src/libnm-platform/nmp-base.h | 9 + src/libnm-platform/tests/test-nm-platform.c | 237 ++++++++++++++++++++ 5 files changed, 336 insertions(+), 7 deletions(-) diff --git a/src/libnm-platform/nm-platform-utils.c b/src/libnm-platform/nm-platform-utils.c index 6f3ad05c94..3f70f5fe79 100644 --- a/src/libnm-platform/nm-platform-utils.c +++ b/src/libnm-platform/nm-platform-utils.c @@ -2275,6 +2275,89 @@ nmp_utils_lifetime_get(guint32 timestamp, /*****************************************************************************/ +static int +bridge_vlan_compare(gconstpointer a, gconstpointer b, gpointer user_data) +{ + const NMPlatformBridgeVlan *vlan_a = a; + const NMPlatformBridgeVlan *vlan_b = b; + + return (int) vlan_a->vid_start - (int) vlan_b->vid_start; +} + +/** + * nmp_utils_bridge_vlan_normalize: + * @vlans: the array of VLAN ranges + * @num_vlans: the number of VLAN ranges in the array. On return, it contains + * the new number. + * + * Sort the VLAN ranges and merge those that are contiguous or overlapping. It + * must not contain invalid data such as 2 overlapping ranges with different + * flags. + */ +void +nmp_utils_bridge_vlan_normalize(NMPlatformBridgeVlan *vlans, guint *num_vlans) +{ + guint i; + + if (*num_vlans <= 1) + return; + + g_qsort_with_data(vlans, *num_vlans, sizeof(NMPlatformBridgeVlan), bridge_vlan_compare, NULL); + + /* Merge VLAN ranges that are contiguous or overlap */ + i = 0; + while (i < *num_vlans - 1) { + guint j = i + 1; + gboolean can_merge = vlans[j].vid_start <= vlans[i].vid_end + 1 + && vlans[j].pvid == vlans[i].pvid + && vlans[j].untagged == vlans[i].untagged; + + if (can_merge) { + vlans[i].vid_end = NM_MAX(vlans[i].vid_end, vlans[j].vid_end); + for (; j < *num_vlans - 1; j++) + vlans[j] = vlans[j + 1]; + *num_vlans -= 1; + } else { + i++; + } + } +} + +/** + * nmp_utils_bridge_normalized_vlans_equal: + * @vlans_a: the first array of bridge VLANs + * @num_vlans_a: the number of elements of first array + * @vlans_b: the second array of bridge VLANs + * @num_vlans_b: the number of elements of second array + * + * Given two arrays of bridge VLAN ranges, compare if they are equal, + * i.e. if they represent the same set of VLANs with the same attributes. + * The input arrays must be normalized (sorted and without overlapping or + * duplicated ranges). Normalize with nmp_utils_bridge_vlan_normalize(). + */ +gboolean +nmp_utils_bridge_normalized_vlans_equal(const NMPlatformBridgeVlan *vlans_a, + guint num_vlans_a, + const NMPlatformBridgeVlan *vlans_b, + guint num_vlans_b) +{ + guint i; + + if (num_vlans_a != num_vlans_b) + return FALSE; + + for (i = 0; i < num_vlans_a; i++) { + if (vlans_a[i].vid_start != vlans_b[i].vid_start || vlans_a[i].vid_end != vlans_b[i].vid_end + || vlans_a[i].pvid != vlans_b[i].pvid || vlans_a[i].untagged != vlans_b[i].untagged) { + return FALSE; + } + } + + return TRUE; +} + +/*****************************************************************************/ + static const char * _trunk_first_line(char *str) { diff --git a/src/libnm-platform/nm-platform-utils.h b/src/libnm-platform/nm-platform-utils.h index 18fc615557..96ac22ef3d 100644 --- a/src/libnm-platform/nm-platform-utils.h +++ b/src/libnm-platform/nm-platform-utils.h @@ -99,4 +99,11 @@ guint32 nmp_utils_lifetime_get(guint32 timestamp, int nmp_utils_modprobe(GError **error, gboolean suppress_error_logging, const char *arg1, ...) G_GNUC_NULL_TERMINATED; +void nmp_utils_bridge_vlan_normalize(NMPlatformBridgeVlan *vlans, guint *num_vlans); + +gboolean nmp_utils_bridge_normalized_vlans_equal(const NMPlatformBridgeVlan *vlans_a, + guint num_vlans_a, + const NMPlatformBridgeVlan *vlans_b, + guint num_vlans_b); + #endif /* __NM_PLATFORM_UTILS_H__ */ diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index e5110ff117..e33be81356 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -741,13 +741,6 @@ typedef struct { gint8 trust; } NMPlatformVF; -typedef struct { - guint16 vid_start; - guint16 vid_end; - bool untagged : 1; - bool pvid : 1; -} NMPlatformBridgeVlan; - typedef struct { guint16 vlan_default_pvid_val; bool vlan_filtering_val : 1; diff --git a/src/libnm-platform/nmp-base.h b/src/libnm-platform/nmp-base.h index 70b5d1bc55..c7d487e23c 100644 --- a/src/libnm-platform/nmp-base.h +++ b/src/libnm-platform/nmp-base.h @@ -38,6 +38,15 @@ typedef enum { /*****************************************************************************/ +typedef struct { + guint16 vid_start; + guint16 vid_end; + bool untagged : 1; + bool pvid : 1; +} NMPlatformBridgeVlan; + +/*****************************************************************************/ + typedef struct { /* We don't want to include in header files, * thus create a ABI compatible version of struct ethtool_drvinfo.*/ diff --git a/src/libnm-platform/tests/test-nm-platform.c b/src/libnm-platform/tests/test-nm-platform.c index 5fc8a5dded..377078750f 100644 --- a/src/libnm-platform/tests/test-nm-platform.c +++ b/src/libnm-platform/tests/test-nm-platform.c @@ -190,6 +190,239 @@ test_nmp_link_mode_all_advertised_modes_bits(void) /*****************************************************************************/ +static void +test_nmp_utils_bridge_vlans_normalize(void) +{ + NMPlatformBridgeVlan vlans[10]; + NMPlatformBridgeVlan expect[10]; + guint vlans_len; + + /* Single one is unmodified */ + vlans[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 10, + .untagged = TRUE, + }; + expect[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 10, + .untagged = TRUE, + }; + vlans_len = 1; + nmp_utils_bridge_vlan_normalize(vlans, &vlans_len); + g_assert(vlans_len == 1); + g_assert(nmp_utils_bridge_normalized_vlans_equal(vlans, vlans_len, expect, vlans_len)); + + /* Not merged if flags are different */ + vlans[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 10, + .untagged = TRUE, + }; + vlans[1] = (NMPlatformBridgeVlan){ + .vid_start = 11, + .vid_end = 11, + .pvid = TRUE, + }; + vlans[2] = (NMPlatformBridgeVlan){ + .vid_start = 20, + .vid_end = 25, + }; + vlans[3] = (NMPlatformBridgeVlan){ + .vid_start = 26, + .vid_end = 30, + .untagged = TRUE, + }; + vlans[4] = (NMPlatformBridgeVlan){ + .vid_start = 40, + .vid_end = 40, + .untagged = TRUE, + }; + vlans[5] = (NMPlatformBridgeVlan){ + .vid_start = 40, + .vid_end = 40, + .untagged = TRUE, + .pvid = TRUE, + }; + expect[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 10, + .untagged = TRUE, + }; + expect[1] = (NMPlatformBridgeVlan){ + .vid_start = 11, + .vid_end = 11, + .pvid = TRUE, + }; + expect[2] = (NMPlatformBridgeVlan){ + .vid_start = 20, + .vid_end = 25, + }; + expect[3] = (NMPlatformBridgeVlan){ + .vid_start = 26, + .vid_end = 30, + .untagged = TRUE, + }; + expect[4] = (NMPlatformBridgeVlan){ + .vid_start = 40, + .vid_end = 40, + .untagged = TRUE, + }; + expect[5] = (NMPlatformBridgeVlan){ + .vid_start = 40, + .vid_end = 40, + .untagged = TRUE, + .pvid = TRUE, + }; + vlans_len = 6; + nmp_utils_bridge_vlan_normalize(vlans, &vlans_len); + g_assert(vlans_len == 6); + g_assert(nmp_utils_bridge_normalized_vlans_equal(vlans, vlans_len, expect, vlans_len)); + + /* Overlapping and contiguous ranges are merged */ + vlans[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 10, + .untagged = TRUE, + }; + vlans[1] = (NMPlatformBridgeVlan){ + .vid_start = 11, + .vid_end = 20, + .untagged = TRUE, + }; + vlans[2] = (NMPlatformBridgeVlan){ + .vid_start = 19, + .vid_end = 30, + .untagged = TRUE, + }; + expect[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 30, + .untagged = TRUE, + }; + vlans_len = 3; + nmp_utils_bridge_vlan_normalize(vlans, &vlans_len); + g_assert(vlans_len == 1); + g_assert(nmp_utils_bridge_normalized_vlans_equal(vlans, vlans_len, expect, vlans_len)); + + vlans[0] = (NMPlatformBridgeVlan){ + .vid_start = 20, + .vid_end = 20, + }; + vlans[1] = (NMPlatformBridgeVlan){ + .vid_start = 4, + .vid_end = 4, + .pvid = TRUE, + }; + vlans[2] = (NMPlatformBridgeVlan){ + .vid_start = 33, + .vid_end = 33, + }; + vlans[3] = (NMPlatformBridgeVlan){ + .vid_start = 100, + .vid_end = 100, + .untagged = TRUE, + }; + vlans[4] = (NMPlatformBridgeVlan){ + .vid_start = 34, + .vid_end = 40, + }; + vlans[5] = (NMPlatformBridgeVlan){ + .vid_start = 21, + .vid_end = 32, + }; + expect[0] = (NMPlatformBridgeVlan){ + .vid_start = 4, + .vid_end = 4, + .pvid = TRUE, + }; + expect[1] = (NMPlatformBridgeVlan){ + .vid_start = 20, + .vid_end = 40, + }; + expect[2] = (NMPlatformBridgeVlan){ + .vid_start = 100, + .vid_end = 100, + .untagged = TRUE, + }; + vlans_len = 6; + nmp_utils_bridge_vlan_normalize(vlans, &vlans_len); + g_assert(vlans_len == 3); + g_assert(nmp_utils_bridge_normalized_vlans_equal(vlans, vlans_len, expect, vlans_len)); +} + +static void +test_nmp_utils_bridge_normalized_vlans_equal(void) +{ + NMPlatformBridgeVlan a[10]; + NMPlatformBridgeVlan b[10]; + + /* Both empty */ + g_assert(nmp_utils_bridge_normalized_vlans_equal(NULL, 0, NULL, 0)); + g_assert(nmp_utils_bridge_normalized_vlans_equal(a, 0, b, 0)); + g_assert(nmp_utils_bridge_normalized_vlans_equal(a, 0, NULL, 0)); + g_assert(nmp_utils_bridge_normalized_vlans_equal(NULL, 0, b, 0)); + + /* One empty, other not */ + a[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 10, + .untagged = TRUE, + }; + g_assert(!nmp_utils_bridge_normalized_vlans_equal(a, 1, NULL, 0)); + g_assert(!nmp_utils_bridge_normalized_vlans_equal(NULL, 0, a, 1)); + + /* Equal range + VLAN */ + a[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 10, + .untagged = TRUE, + }; + a[1] = (NMPlatformBridgeVlan){ + .vid_start = 11, + .vid_end = 11, + .pvid = TRUE, + }; + b[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 10, + .untagged = TRUE, + }; + b[1] = (NMPlatformBridgeVlan){ + .vid_start = 11, + .vid_end = 11, + .pvid = TRUE, + }; + g_assert(nmp_utils_bridge_normalized_vlans_equal(a, 2, b, 2)); + g_assert(nmp_utils_bridge_normalized_vlans_equal(b, 2, a, 2)); + + /* Different flag */ + b[1].pvid = FALSE; + g_assert(!nmp_utils_bridge_normalized_vlans_equal(a, 2, b, 2)); + g_assert(!nmp_utils_bridge_normalized_vlans_equal(b, 2, a, 2)); + + /* Different ranges */ + a[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 30, + .untagged = TRUE, + }; + b[0] = (NMPlatformBridgeVlan){ + .vid_start = 1, + .vid_end = 29, + .untagged = TRUE, + }; + g_assert(!nmp_utils_bridge_normalized_vlans_equal(a, 1, b, 1)); + g_assert(!nmp_utils_bridge_normalized_vlans_equal(b, 1, a, 1)); + + b[0].vid_start = 2; + b[0].vid_end = 30; + g_assert(!nmp_utils_bridge_normalized_vlans_equal(a, 1, b, 1)); + g_assert(!nmp_utils_bridge_normalized_vlans_equal(b, 1, a, 1)); +} + +/*****************************************************************************/ + static void test_nmpclass_consistency(void) { @@ -252,6 +485,10 @@ main(int argc, char **argv) g_test_add_func("/nm-platform/test_nmp_link_mode_all_advertised_modes_bits", test_nmp_link_mode_all_advertised_modes_bits); g_test_add_func("/nm-platform/test_nmpclass_consistency", test_nmpclass_consistency); + g_test_add_func("/nm-platform/test_nmp_utils_bridge_vlans_normalize", + test_nmp_utils_bridge_vlans_normalize); + g_test_add_func("/nm-platform/nmp-utils-bridge-vlans-equal", + test_nmp_utils_bridge_normalized_vlans_equal); return g_test_run(); } From 447e50d74e73e97a67d3563754e2169965f92d57 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 26 Jul 2024 11:31:48 +0200 Subject: [PATCH 5/5] bridge: reapply port VLANs only when necessary Don't touch the bridge VLANs if they are already set. --- src/core/devices/nm-device-bridge.c | 35 ++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/src/core/devices/nm-device-bridge.c b/src/core/devices/nm-device-bridge.c index 7628e4600f..7a496d965c 100644 --- a/src/core/devices/nm-device-bridge.c +++ b/src/core/devices/nm-device-bridge.c @@ -13,6 +13,7 @@ #include "NetworkManagerUtils.h" #include "nm-device-private.h" #include "libnm-platform/nm-platform.h" +#include "libnm-platform/nm-platform-utils.h" #include "nm-device-factory.h" #include "libnm-core-aux-intern/nm-libnm-core-utils.h" #include "libnm-core-intern/nm-core-internal.h" @@ -770,14 +771,18 @@ merge_bridge_vlan_default_pvid(NMPlatformBridgeVlan *vlans, guint *num_vlans, gu void nm_device_reapply_bridge_port_vlans(NMDevice *device) { + NMDevice *self = device; /* for logging */ NMSettingBridgePort *s_bridge_port; NMDevice *controller; NMSettingBridge *s_bridge; gs_unref_ptrarray GPtrArray *tmp_vlans = NULL; gs_free NMPlatformBridgeVlan *setting_vlans = NULL; + gs_free NMPlatformBridgeVlan *plat_vlans = NULL; guint num_setting_vlans = 0; + guint num_plat_vlans = 0; NMPlatform *plat; int ifindex; + gboolean do_reapply; s_bridge_port = nm_device_get_applied_setting(device, NM_TYPE_SETTING_BRIDGE_PORT); if (!s_bridge_port) @@ -810,9 +815,33 @@ nm_device_reapply_bridge_port_vlans(NMDevice *device) plat = nm_device_get_platform(device); ifindex = nm_device_get_ifindex(device); - nm_platform_link_set_bridge_vlans(plat, ifindex, TRUE, NULL, 0); - if (num_setting_vlans > 0) - nm_platform_link_set_bridge_vlans(plat, ifindex, TRUE, setting_vlans, num_setting_vlans); + if (!nm_platform_link_get_bridge_vlans(plat, ifindex, &plat_vlans, &num_plat_vlans)) { + _LOGD(LOGD_DEVICE, "reapply-bridge-port-vlans: can't get current VLANs from platform"); + do_reapply = TRUE; + } else { + nmp_utils_bridge_vlan_normalize(setting_vlans, &num_setting_vlans); + nmp_utils_bridge_vlan_normalize(plat_vlans, &num_plat_vlans); + if (!nmp_utils_bridge_normalized_vlans_equal(setting_vlans, + num_setting_vlans, + plat_vlans, + num_plat_vlans)) { + _LOGD(LOGD_DEVICE, "reapply-bridge-port-vlans: VLANs in platform need reapply"); + do_reapply = TRUE; + } else { + _LOGD(LOGD_DEVICE, "reapply-bridge-port-vlans: VLANs in platform didn't change"); + do_reapply = FALSE; + } + } + + if (do_reapply) { + nm_platform_link_set_bridge_vlans(plat, ifindex, TRUE, NULL, 0); + if (num_setting_vlans > 0) + nm_platform_link_set_bridge_vlans(plat, + ifindex, + TRUE, + setting_vlans, + num_setting_vlans); + } } static void