From e805fa93db1aae4186b9479ceab238e1dd8e562e Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 22 Feb 2024 13:47:59 +0100 Subject: [PATCH 1/3] netlink: fix vmac deletion issue due to race condition Due to a race condition, the VMAC interface sometimes remains undeleted after Netlink reports the removal of its link interface. The function cleanup_lost_interface() fails to delete the interface. This failure occurs because the condition 'IF_BASE_IFP(vrrp->ifp) != ifp' in the following code segment never matches: > if (vrrp->ifp != ifp > #ifdef _HAVE_VRRP_VMAC_ > && IF_BASE_IFP(vrrp->ifp) != ifp && VRRP_CONFIGURED_IFP(vrrp) != ifp > #endif This non-matching situation arises because 'vrrp->ifp->base_ifp' is the same as 'vrrp->ifp'. During the initialization in if_get_by_ifname(), when a new interface_t pointer is created, 'ifp->base_ifp' is set as follows: > ifp->base_ifp = ifp; netlink_if_link_populate() gets bypassed whenever 'ifp->base_ifp->ifindex' is different from the index of the actual link interface. This bypass is inevitable when 'ifp' is newly allocated. netlink_if_link_populate() normally populates the 'ifp' data including the 'ifp->base_ifp'. Fix the issue of VMAC deletion by adjusting the logic so that netlink_if_link_populate() does not get skipped when the base_ifp of a VMAC is the VMAC itself. Signed-off-by: Louis Scalbert --- keepalived/core/keepalived_netlink.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/keepalived/core/keepalived_netlink.c b/keepalived/core/keepalived_netlink.c index 198d0e10f..dabd91309 100644 --- a/keepalived/core/keepalived_netlink.c +++ b/keepalived/core/keepalived_netlink.c @@ -1875,7 +1875,9 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg /* If a macvlan, check the underlying interface hasn't changed */ if (IS_MAC_IP_VLAN(ifp) && - (!tb[IFLA_LINK] || ifp->base_ifp->ifindex != *PTR_CAST(uint32_t, RTA_DATA(tb[IFLA_LINK])))) + (!tb[IFLA_LINK] || + (ifp->ifindex != ifp->base_ifp->ifindex && + ifp->base_ifp->ifindex != *PTR_CAST(uint32_t, RTA_DATA(tb[IFLA_LINK]))))) return false; } #endif From 566f44bb59bdeec36d6e3f7c973a9d6f997c709b Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 22 Feb 2024 15:08:51 +0100 Subject: [PATCH 2/3] netlink: add endif descriptions in netlink_if_link_populate() Add #endif descriptions in netlink_if_link_populate() to clarify in which #ifdef blocks the next commit is modifying the code. This is a cosmetic change. Signed-off-by: Louis Scalbert --- keepalived/core/keepalived_netlink.c | 34 ++++++++++++++-------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/keepalived/core/keepalived_netlink.c b/keepalived/core/keepalived_netlink.c index dabd91309..0f95cbfb3 100644 --- a/keepalived/core/keepalived_netlink.c +++ b/keepalived/core/keepalived_netlink.c @@ -1813,12 +1813,12 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg bool is_vrf = false; uint32_t new_vrf_master_index; bool is_vrf_master = false; -#endif -#endif +#endif /* _HAVE_VRF_ */ +#endif /* _HAVE_VRRP_VMAC_ */ #ifdef _HAVE_VRRP_VMAC_ was_vlan = IS_MAC_IP_VLAN(ifp); -#endif +#endif /* _HAVE_VRRP_VMAC_ */ name = (char *)RTA_DATA(tb[IFLA_IFNAME]); @@ -1827,10 +1827,10 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg ifp->ifindex = (ifindex_t)ifi->ifi_index; #ifdef _HAVE_VRRP_VMAC_ ifp->if_type = IF_TYPE_STANDARD; -#endif +#endif /* _HAVE_VRRP_VMAC_ */ #ifdef HAVE_IFLA_LINK_NETNSID /* from Linux v4.0 */ ifp->base_netns_id = -1; -#endif +#endif /* HAVE_IFLA_LINK_NETNSID */ #ifdef _HAVE_VRRP_VMAC_ if (tb[IFLA_LINKINFO]) { @@ -1848,7 +1848,7 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg ifp->if_type = IF_TYPE_IPVLAN; parse_rtattr_nested(linkattr, IFLA_IPVLAN_MAX, linkinfo[IFLA_INFO_DATA]); } -#endif +#endif /* _HAVE_VRRP_IPVLAN_ */ #ifdef _HAVE_VRF_ else if (!strcmp((char *)RTA_DATA(linkinfo[IFLA_INFO_KIND]), "vrf") ) { is_vrf = true; @@ -1857,14 +1857,14 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg if (vrf_attr[IFLA_VRF_TABLE]) ifp->vrf_tb_id = *PTR_CAST(uint32_t, RTA_DATA(vrf_attr[IFLA_VRF_TABLE])); } -#endif +#endif /* _HAVE_VRF_ */ } } #ifdef _HAVE_IPV4_DEVCONF_ if (tb[IFLA_AF_SPEC]) parse_af_spec(tb[IFLA_AF_SPEC], ifp); -#endif +#endif /* _HAVE_IPV4_DEVCONF_ */ /* Check there hasn't been an unsupported interface type change */ if (!global_data->allow_if_changes && ifp->seen_interface) { @@ -1880,7 +1880,7 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg ifp->base_ifp->ifindex != *PTR_CAST(uint32_t, RTA_DATA(tb[IFLA_LINK]))))) return false; } -#endif +#endif /* _HAVE_VRRP_VMAC_ */ ifp->mtu = *PTR_CAST(uint32_t, RTA_DATA(tb[IFLA_MTU])); ifp->hw_type = ifi->ifi_type; @@ -1903,7 +1903,7 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg if (((ifp->if_type == IF_TYPE_MACVLAN && linkattr[IFLA_MACVLAN_MODE]) #ifdef _HAVE_VRRP_IPVLAN_ || (ifp->if_type == IF_TYPE_IPVLAN && linkattr[IFLA_IPVLAN_MODE]) -#endif +#endif /* _HAVE_VRRP_IPVLAN_ */ ) && tb[IFLA_LINK]) { if (ifp->if_type == IF_TYPE_MACVLAN) @@ -1913,15 +1913,15 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg ifp->vmac_type = *PTR_CAST(uint32_t, RTA_DATA(linkattr[IFLA_IPVLAN_MODE])); #if HAVE_DECL_IFLA_IPVLAN_FLAGS ifp->ipvlan_flags = *PTR_CAST(uint32_t, RTA_DATA(linkattr[IFLA_IPVLAN_FLAGS])); -#endif +#endif /* HAVE_DECL_IFLA_IPVLAN_FLAGS */ } -#endif +#endif /* _HAVE_VRRP_IPVLAN_ */ ifp->base_ifindex = *PTR_CAST(uint32_t, RTA_DATA(tb[IFLA_LINK])); #ifdef HAVE_IFLA_LINK_NETNSID /* from Linux v4.0 */ if (tb[IFLA_LINK_NETNSID]) /* Only use link details if in same network namespace */ ifp->base_netns_id = *PTR_CAST(int32_t, RTA_DATA(tb[IFLA_LINK_NETNSID])); else -#endif +#endif /* HAVE_IFLA_LINK_NETNSID */ { ifp->base_ifp = if_get_by_ifindex(ifp->base_ifindex); if (ifp->base_ifp) @@ -1939,11 +1939,11 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg is_vrf_master = true; } } -#endif +#endif /* _HAVE_VRF_ */ #ifdef _FIXED_IF_TYPE_ if (strcmp(_FIXED_IF_TYPE_, (char *)RTA_DATA(linkinfo[IFLA_INFO_KIND]))) -#endif +#endif /* _FIXED_IF_TYPE_ */ ifp->changeable_type = true; } } @@ -1975,10 +1975,10 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg } } } -#endif +#endif /* _HAVE_VRF_ */ ifp->rp_filter = UINT_MAX; /* We have not read it yet */ -#endif +#endif /* _HAVE_VRRP_VMAC_ */ ifp->ifi_flags = ifi->ifi_flags; if (FLAGS_UP(ifi->ifi_flags)) From 0624ca433bf8ad232b6bfcc70d475f4322ebb236 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 22 Feb 2024 14:19:49 +0100 Subject: [PATCH 3/3] vrrp: fix vmac creation issue due to race condition The VMAC interface sometimes does not reappear when its associated link interface is quickly re-added after being deleted, a situation caused by a race condition. This problem manifests during the operations of cleanup_lost_interface() This function checks for VMAC interfaces on top of the removed link interface. It deletes these VMAC interfaces if they are present. Subsequently, Netlink is invoked to refresh the information of all interfaces, and this is followed by the cleaning of the link interface data in memory. The problem occurs when Netlink, queried indirectly by sub-functions of cleanup_lost_interface(), detects that the link interface has reappeared. Although the interface data in memory is updated accordingly, cleanup_lost_interface() unconditionally clears this refreshed information. As a result, the data regarding the link interface is lost, preventing the re-creation of its associated VMAC interfaces. Fix the VMAC creation issue by adding a 'cleaning flag' that is set at the start of the cleanup process. This flag says whether to proceed with the interface data cleanup. If the interface is refreshed during the Netlink polling, the flag is unset, thereby preventing the subsequent clearing of the updated interface information. Signed-off-by: Louis Scalbert --- keepalived/core/keepalived_netlink.c | 2 ++ keepalived/include/vrrp_if.h | 1 + keepalived/vrrp/vrrp_if.c | 12 ++++++++++++ 3 files changed, 15 insertions(+) diff --git a/keepalived/core/keepalived_netlink.c b/keepalived/core/keepalived_netlink.c index 0f95cbfb3..3582dee04 100644 --- a/keepalived/core/keepalived_netlink.c +++ b/keepalived/core/keepalived_netlink.c @@ -1978,6 +1978,8 @@ netlink_if_link_populate(interface_t *ifp, struct rtattr *tb[], struct ifinfomsg #endif /* _HAVE_VRF_ */ ifp->rp_filter = UINT_MAX; /* We have not read it yet */ + + ifp->cleaning = false; #endif /* _HAVE_VRRP_VMAC_ */ ifp->ifi_flags = ifi->ifi_flags; diff --git a/keepalived/include/vrrp_if.h b/keepalived/include/vrrp_if.h index 0ee5bb757..248b09cfc 100644 --- a/keepalived/include/vrrp_if.h +++ b/keepalived/include/vrrp_if.h @@ -161,6 +161,7 @@ typedef struct _interface { otherwise the physical interface */ bool is_ours; /* keepalived created the interface */ bool deleting; /* Set when we are deleting the interface */ + bool cleaning; /* Set when we are cleaning the interface */ bool seen_interface; /* The interface has existed at some point since we started */ bool changeable_type; /* The interface type or underlying interface can be changed */ #ifdef _HAVE_VRF_ diff --git a/keepalived/vrrp/vrrp_if.c b/keepalived/vrrp/vrrp_if.c index f9bde8dba..60befa04d 100644 --- a/keepalived/vrrp/vrrp_if.c +++ b/keepalived/vrrp/vrrp_if.c @@ -1394,6 +1394,11 @@ cleanup_lost_interface(interface_t *ifp) tracking_obj_t *top; vrrp_t *vrrp; +#ifdef _HAVE_VRRP_VMAC_ + ifp->cleaning = true; +#endif /* _HAVE_VRRP_VMAC_ */ + + list_for_each_entry(top, &ifp->tracking_vrrp, e_list) { vrrp = top->obj.vrrp; @@ -1476,12 +1481,19 @@ cleanup_lost_interface(interface_t *ifp) interface_down(ifp); +#ifdef _HAVE_VRRP_VMAC_ + if (!ifp->cleaning) + /* interface has been refreshed. Do not clean */ + return; +#endif /* _HAVE_VRRP_VMAC_ */ + ifp->ifindex = 0; ifp->ifi_flags = 0; ifp->seen_up = false; #ifdef _HAVE_VRRP_VMAC_ if (!ifp->is_ours) ifp->base_ifp = ifp; + ifp->cleaning = false; #endif #ifdef _HAVE_VRF_ ifp->vrf_master_ifp = NULL;