From 02c34d538c6a2b22bd09318496ba104eb43246b4 Mon Sep 17 00:00:00 2001 From: Gris Ge Date: Thu, 8 Feb 2024 23:36:34 +0800 Subject: [PATCH] bridge: skip VLAN filtering resetting in reapply if no vlan change changed When doing reapply on linux bridge interface, NetworkManager will reset the VLAN filtering and default PVID which cause PVID been readded to all bridge ports regardless they are managed by NetworkManager. This is because Linux kernel will re-add PVID to bridge port upon the changes of bridge default-pvid value. To fix the issue, this patch introduce netlink parsing code for `vlan_filtering` and `default_pvid` of NMPlatformLnkBridge, and use that to compare desired VLAN filtering settings, skip the reset of VLAN filter if `default_pvid` and `vlan_filtering` are unchanged. Signed-off-by: Gris Ge --- src/core/devices/nm-device-bridge.c | 79 +++++++++++++++++--------- src/core/platform/tests/test-link.c | 2 + src/libnm-platform/nm-linux-platform.c | 6 ++ src/libnm-platform/nm-platform.c | 13 ++++- src/libnm-platform/nm-platform.h | 2 + 5 files changed, 72 insertions(+), 30 deletions(-) diff --git a/src/core/devices/nm-device-bridge.c b/src/core/devices/nm-device-bridge.c index 193b655ce..ab3a6be90 100644 --- a/src/core/devices/nm-device-bridge.c +++ b/src/core/devices/nm-device-bridge.c @@ -619,7 +619,27 @@ master_update_slave_connection(NMDevice *device, } static gboolean -bridge_set_vlan_options(NMDevice *device, NMSettingBridge *s_bridge) +is_bridge_pvid_changed(NMDevice *device, NMSettingBridge *s_bridge) +{ + int ifindex = nm_device_get_ifindex(device); + const NMPlatformLnkBridge *nmp_link_br; + NMPlatform *platform = nm_device_get_platform(device); + bool desired_vlan_filtering = nm_setting_bridge_get_vlan_filtering(s_bridge); + guint16 desired_pvid = nm_setting_bridge_get_vlan_default_pvid(s_bridge); + + nm_platform_link_refresh(platform, ifindex); + nmp_link_br = nm_platform_link_get_lnk_bridge(platform, ifindex, NULL); + + if (nmp_link_br) { + return desired_vlan_filtering != nmp_link_br->vlan_filtering + || desired_pvid != nmp_link_br->default_pvid; + } else { + return TRUE; + } +} + +static gboolean +bridge_set_vlan_options(NMDevice *device, NMSettingBridge *s_bridge, gboolean is_reapply) { NMDeviceBridge *self = NM_DEVICE_BRIDGE(device); gconstpointer hwaddr; @@ -660,31 +680,37 @@ bridge_set_vlan_options(NMDevice *device, NMSettingBridge *s_bridge) self->vlan_configured = TRUE; - /* Filtering must be disabled to change the default PVID. - * Clear the default PVID so that we later can force the re-creation of - * default PVID VLANs by writing the option again. */ + if (!is_reapply || is_bridge_pvid_changed(device, s_bridge)) { + /* Filtering must be disabled to change the default PVID. + * Clear the default PVID so that we later can force the re-creation of + * default PVID VLANs by writing the option again. */ - nm_platform_link_set_bridge_info( - plat, - ifindex, - &((NMPlatformLinkSetBridgeInfoData){.vlan_filtering_has = TRUE, - .vlan_filtering_val = FALSE, - .vlan_default_pvid_has = TRUE, - .vlan_default_pvid_val = 0})); + if (is_reapply) { + _LOGD(LOGD_BRIDGE, "default_pvid is changed, resetting bridge VLAN filtering"); + } - /* Clear all existing VLANs */ - if (!nm_platform_link_set_bridge_vlans(plat, ifindex, FALSE, NULL)) - return FALSE; - - /* Now set the default PVID. After this point the kernel creates - * a PVID VLAN on each port, including the bridge itself. */ - pvid = nm_setting_bridge_get_vlan_default_pvid(s_bridge); - if (pvid) { nm_platform_link_set_bridge_info( plat, ifindex, - &((NMPlatformLinkSetBridgeInfoData){.vlan_default_pvid_has = TRUE, - .vlan_default_pvid_val = pvid})); + &((NMPlatformLinkSetBridgeInfoData){.vlan_filtering_has = TRUE, + .vlan_filtering_val = FALSE, + .vlan_default_pvid_has = TRUE, + .vlan_default_pvid_val = 0})); + + /* Clear all existing VLANs */ + if (!nm_platform_link_set_bridge_vlans(plat, ifindex, FALSE, NULL)) + return FALSE; + + /* Now set the default PVID. After this point the kernel creates + * a PVID VLAN on each port, including the bridge itself. */ + pvid = nm_setting_bridge_get_vlan_default_pvid(s_bridge); + if (pvid) { + nm_platform_link_set_bridge_info( + plat, + ifindex, + &((NMPlatformLinkSetBridgeInfoData){.vlan_default_pvid_has = TRUE, + .vlan_default_pvid_val = pvid})); + } } /* Create VLANs only after setting the default PVID, so that @@ -743,7 +769,7 @@ _platform_lnk_bridge_init_from_setting(NMSettingBridge *s_bridge, NMPlatformLnkB } static gboolean -link_config(NMDevice *device, NMConnection *connection) +link_config(NMDevice *device, NMConnection *connection, gboolean is_reapply) { int ifindex = nm_device_get_ifindex(device); NMSettingBridge *s_bridge; @@ -757,7 +783,7 @@ link_config(NMDevice *device, NMConnection *connection) if (nm_platform_link_bridge_change(nm_device_get_platform(device), ifindex, &props) < 0) return FALSE; - return bridge_set_vlan_options(device, s_bridge); + return bridge_set_vlan_options(device, s_bridge, is_reapply); } static NMActStageReturn @@ -768,7 +794,7 @@ act_stage1_prepare(NMDevice *device, NMDeviceStateReason *out_failure_reason) connection = nm_device_get_applied_connection(device); g_return_val_if_fail(connection, NM_ACT_STAGE_RETURN_FAILURE); - if (!link_config(device, connection)) { + if (!link_config(device, connection, FALSE)) { NM_SET_OUT(out_failure_reason, NM_DEVICE_STATE_REASON_CONFIG_FAILED); return NM_ACT_STAGE_RETURN_FAILURE; } @@ -910,7 +936,7 @@ attach_port(NMDevice *device, s_port = nm_connection_get_setting_bridge_port(connection); if (!nm_device_sys_iface_state_is_external(device)) - bridge_set_vlan_options(device, s_bridge); + bridge_set_vlan_options(device, s_bridge, FALSE); if (nm_setting_bridge_get_vlan_filtering(s_bridge)) { gs_free const NMPlatformBridgeVlan **plat_vlans = NULL; @@ -1125,8 +1151,7 @@ reapply_connection(NMDevice *device, NMConnection *con_old, NMConnection *con_ne /* Make sure bridge_set_vlan_options() called by link_config() * sets vlan_filtering and default_pvid anew. */ self->vlan_configured = FALSE; - - link_config(device, con_new); + link_config(device, con_new, TRUE); } /*****************************************************************************/ diff --git a/src/core/platform/tests/test-link.c b/src/core/platform/tests/test-link.c index c931605ba..205559ceb 100644 --- a/src/core/platform/tests/test-link.c +++ b/src/core/platform/tests/test-link.c @@ -1424,6 +1424,8 @@ test_software_detect(gconstpointer user_data) lnk_bridge.mcast_query_interval = 12000; lnk_bridge.mcast_query_response_interval = 5200; lnk_bridge.mcast_startup_query_interval = 3000; + lnk_bridge.vlan_filtering = FALSE; + lnk_bridge.default_pvid = 1; if (!nmtstp_link_bridge_add(NULL, ext, DEVICE_NAME, &lnk_bridge)) g_error("Failed adding Bridge interface"); diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 22cd57848..a70782808 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -1530,6 +1530,8 @@ _parse_lnk_bridge(const char *kind, struct nlattr *info_data) [IFLA_BR_MCAST_QUERY_INTVL] = {.type = NLA_U64}, [IFLA_BR_MCAST_QUERY_RESPONSE_INTVL] = {.type = NLA_U64}, [IFLA_BR_MCAST_STARTUP_QUERY_INTVL] = {.type = NLA_U64}, + [IFLA_BR_VLAN_FILTERING] = {.type = NLA_U8}, + [IFLA_BR_VLAN_DEFAULT_PVID] = {.type = NLA_U16}, }; NMPlatformLnkBridge *props; struct nlattr *tb[G_N_ELEMENTS(policy)]; @@ -1600,6 +1602,10 @@ _parse_lnk_bridge(const char *kind, struct nlattr *info_data) props->mcast_query_response_interval = nla_get_u64(tb[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL]); if (tb[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]) props->mcast_startup_query_interval = nla_get_u64(tb[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]); + if (tb[IFLA_BR_VLAN_FILTERING]) + props->vlan_filtering = !!nla_get_u8(tb[IFLA_BR_VLAN_FILTERING]); + if (tb[IFLA_BR_VLAN_DEFAULT_PVID]) + props->default_pvid = nla_get_u16(tb[IFLA_BR_VLAN_DEFAULT_PVID]); return obj; } diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index 531c98e8d..1411fe9e1 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -6221,7 +6221,8 @@ nm_platform_lnk_bridge_to_string(const NMPlatformLnkBridge *lnk, char *buf, gsiz " mcast_querier_interval %" G_GUINT64_FORMAT " mcast_query_interval %" G_GUINT64_FORMAT " mcast_query_response_interval %" G_GUINT64_FORMAT - " mcast_startup_query_interval %" G_GUINT64_FORMAT "", + " mcast_startup_query_interval %" G_GUINT64_FORMAT " vlan_filtering %d" + " default_pvid %" G_GUINT16_FORMAT "", lnk->forward_delay, lnk->hello_time, lnk->max_age, @@ -6244,7 +6245,9 @@ nm_platform_lnk_bridge_to_string(const NMPlatformLnkBridge *lnk, char *buf, gsiz lnk->mcast_querier_interval, lnk->mcast_query_interval, lnk->mcast_query_response_interval, - lnk->mcast_startup_query_interval); + lnk->mcast_startup_query_interval, + lnk->vlan_filtering, + lnk->default_pvid); return buf; } @@ -8146,12 +8149,14 @@ nm_platform_lnk_bridge_hash_update(const NMPlatformLnkBridge *obj, NMHashState * obj->mcast_router, obj->mcast_query_response_interval, obj->mcast_startup_query_interval, + obj->default_pvid, NM_HASH_COMBINE_BOOLS(guint8, obj->stp_state, obj->mcast_querier, obj->mcast_query_use_ifaddr, obj->mcast_snooping, - obj->vlan_stats_enabled)); + obj->vlan_stats_enabled, + obj->vlan_filtering)); } void @@ -8304,6 +8309,8 @@ nm_platform_lnk_bridge_cmp(const NMPlatformLnkBridge *a, const NMPlatformLnkBrid NM_CMP_FIELD(a, b, mcast_query_interval); NM_CMP_FIELD(a, b, mcast_query_response_interval); NM_CMP_FIELD(a, b, mcast_startup_query_interval); + NM_CMP_FIELD_BOOL(a, b, vlan_filtering); + NM_CMP_FIELD(a, b, default_pvid); return 0; } diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index 996f84b8b..a6e60bd40 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -779,6 +779,8 @@ typedef struct { bool mcast_snooping : 1; bool stp_state : 1; bool vlan_stats_enabled : 1; + bool vlan_filtering; + guint16 default_pvid; } _nm_alignas(NMPlatformObject) NMPlatformLnkBridge; extern const NMPlatformLnkBridge nm_platform_lnk_bridge_default;