From 20c87e98d8399c322f3f8da9d34ca19fd4ca1865 Mon Sep 17 00:00:00 2001 From: Thibaut Collet Date: Thu, 30 Aug 2018 11:42:55 +0200 Subject: [PATCH 1/4] vrf: return vrf implementation for default vrf To correct potential crash with netns implementation of vrf (see next commit) it is necessary to allow any daemons to know the vrf implementation whatever the vrf. With current implementation the daemons do not know the vrf implementation for the default vrf. For this vrf the returned vrf implementation is always vrf-lite. To solve this issue a netns name is set to the default vrf to just test is presence to know the used implementation. For zebra a netns name (if needed) is set in the vrf_init function just before enabling the vrf. So this information is propagated to the other daemons thanks the zapi message called when the vrf is enable at zebra layer and override the default configuration (vrf-lite) of the daemon. Signed-off-by: Thibaut Collet --- lib/vrf.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/vrf.c b/lib/vrf.c index 1fb1b786c7..696ae3f48c 100644 --- a/lib/vrf.c +++ b/lib/vrf.c @@ -497,6 +497,16 @@ void vrf_init(int (*create)(struct vrf *), int (*enable)(struct vrf *), strlcpy(default_vrf->data.l.netns_name, VRF_DEFAULT_NAME, NS_NAMSIZ); + if (vrf_is_backend_netns()) { + struct ns *ns; + + strlcpy(default_vrf->data.l.netns_name, + VRF_DEFAULT_NAME, NS_NAMSIZ); + ns = ns_lookup(ns_get_default_id()); + ns->vrf_ctxt = (void *)default_vrf; + default_vrf->ns_ctxt = (void *)ns; + } + /* Enable the default VRF. */ if (!vrf_enable(default_vrf)) { flog_err(LIB_ERR_VRF_START, @@ -711,8 +721,6 @@ int vrf_is_mapped_on_netns(struct vrf *vrf) { if (!vrf || vrf->data.l.netns_name[0] == '\0') return 0; - if (vrf->vrf_id == VRF_DEFAULT) - return 0; return 1; } From ee2f2c23ca2778feafa398e773d24da07d6f174e Mon Sep 17 00:00:00 2001 From: Thibaut Collet Date: Tue, 29 May 2018 10:34:38 +0200 Subject: [PATCH 2/4] zebra: fix crash when interface vrf changes This crash occurs only with netns implementation. vrf meaning is different regarging its implementation (netns or vrf-lite) - With vrf-lite implementation vrf is a property of the interface that can be changed as the speed or the state (iproute2 command: "ip link set dev IF_NAME master VRF_NAME"). All interfaces of the system are in the same netns and so interface name is unique. - With netns implementation vrf is a characteristic of the interface that CANNOT be changed: it is the id of the netns where the interface is located. To change the vrf of an interface (iproute2 command to move an interface "ip netns exec VRF_NAME1 ip link set dev IF_NAME netns VRF_NAME2") the interface is deleted from the old vrf and created in the new vrf. Interface name is not unique, the same name can be present in the different netns (typically the lo interface) and search of interface must be done by the tuple (interface name, netns id). Current tests on the vrf implementation (vrf-lite or netns) are not sufficient. In some cases (for example when an interface is moved from a vrf X to the default vrf and then move back to VRF X) we can have a corruption message and then a crash of zebra. To avoid this corruption test on the vrf implementation, needed when an interface changes, has been rewritten: - For all interface changes except deletion the if_get_by_name function, that checks if an interface exists and creates or updates it if needed, is changed: * The vrf-lite implementation is unchanged: search of the interface is based only on the name and update the vrf-id if needed. * The netns implementation search of the interface is based on the (name, vrf-id) tuple and interface is created if not found, the vrf-id is never updated. - deletion of an interface (reception of a RTM_DELLINK netlink message): * The vrf-lite implementation is unchanged: the interface information are cleared and the interface is moved to the default vrf if it does not belong to (to allow vrf deletion) * The netns implementation is changed: only the interface information are cleared and the interface stays in its vrf to avoid conflict with interface with the same name in the default vrf. This implementation reverts (partially or totally): commit 393ec5424e35 ("zebra: fix missing node attribute set in ifp") commit e9e9b1150f0c ("lib: create interface even if name is the same") commit 9373219c67e1 ("zebra: improve logs when replacing interface to an other netns") Fixes: b53686c52a59 ("zebra: delete interface that disappeared") Signed-off-by: Thibaut Collet Signed-off-by: Philippe Guibert --- lib/if.c | 66 ++++++++++++++++++++++++++------------------- zebra/if_netlink.c | 67 ---------------------------------------------- zebra/interface.c | 33 +++++------------------ zebra/interface.h | 2 -- 4 files changed, 44 insertions(+), 124 deletions(-) diff --git a/lib/if.c b/lib/if.c index 2bf0c6e6b5..c630dd140a 100644 --- a/lib/if.c +++ b/lib/if.c @@ -371,37 +371,47 @@ struct interface *if_lookup_prefix(struct prefix *prefix, vrf_id_t vrf_id) one. */ struct interface *if_get_by_name(const char *name, vrf_id_t vrf_id, int vty) { - struct interface *ifp; + struct interface *ifp = NULL; - ifp = if_lookup_by_name(name, vrf_id); - if (ifp) - return ifp; - /* Not Found on same VRF. If the interface command - * was entered in vty without a VRF (passed as VRF_DEFAULT), - * accept the ifp we found. If a vrf was entered and there is - * a mismatch, reject it if from vty. - */ - ifp = if_lookup_by_name_all_vrf(name); - if (!ifp) - return if_create(name, vrf_id); - if (vty) { - if (vrf_id == VRF_DEFAULT) + if (vrf_is_mapped_on_netns(vrf_lookup_by_id(vrf_id))) { + ifp = if_lookup_by_name(name, vrf_id); + if (ifp) return ifp; - return NULL; - } - /* if vrf backend uses NETNS, then - * this should not be considered as an update - * then create the new interface - */ - if (ifp->vrf_id != vrf_id && vrf_is_mapped_on_netns( - vrf_lookup_by_id(vrf_id))) + if (vty) { + /* If the interface command was entered in vty without a + * VRF (passed as VRF_DEFAULT), search an interface with + * this name in all VRs + */ + if (vrf_id == VRF_DEFAULT) + return if_lookup_by_name_all_vrf(name); + return NULL; + } return if_create(name, vrf_id); - /* If it came from the kernel - * or by way of zclient, believe it and update - * the ifp accordingly. - */ - if_update_to_new_vrf(ifp, vrf_id); - return ifp; + } else { + ifp = if_lookup_by_name_all_vrf(name); + if (ifp) { + if (ifp->vrf_id == vrf_id) + return ifp; + /* Found a match on a different VRF. If the interface + * command was entered in vty without a VRF (passed as + * VRF_DEFAULT), accept the ifp we found. If a vrf was + * entered and there is a mismatch, reject it if from + * vty. If it came from the kernel or by way of zclient, + * believe it and update the ifp accordingly. + */ + if (vty) { + if (vrf_id == VRF_DEFAULT) + return ifp; + return NULL; + } + /* If it came from the kernel or by way of zclient, + * believe it and update the ifp accordingly. + */ + if_update_to_new_vrf(ifp, vrf_id); + return ifp; + } + return if_create(name, vrf_id); + } } void if_set_index(struct interface *ifp, ifindex_t ifindex) diff --git a/zebra/if_netlink.c b/zebra/if_netlink.c index a15d914243..1fc3e61a3b 100644 --- a/zebra/if_netlink.c +++ b/zebra/if_netlink.c @@ -1042,67 +1042,6 @@ int netlink_interface_addr(struct nlmsghdr *h, ns_id_t ns_id, int startup) return 0; } -/* helper function called by if_netlink_change - * to delete interfaces in case the interface moved - * to an other netns - */ -static void if_netlink_check_ifp_instance_consistency(uint16_t cmd, - struct interface *ifp, - ns_id_t ns_id) -{ - struct interface *other_ifp; - - /* - * look if interface name is also found on other netns - * - only if vrf backend is netns - * - do not concern lo interface - * - then remove previous one - * - for new link case, check found interface is not active - */ - if (!vrf_is_backend_netns() || - !strcmp(ifp->name, "lo")) - return; - other_ifp = if_lookup_by_name_not_ns(ns_id, ifp->name); - if (!other_ifp) - return; - /* because previous interface may be inactive, - * interface is moved back to default vrf - * then one may find the same pointer; ignore - */ - if (other_ifp == ifp) - return; - if ((cmd == RTM_NEWLINK) - && (CHECK_FLAG(other_ifp->status, ZEBRA_INTERFACE_ACTIVE))) - return; - if (IS_ZEBRA_DEBUG_KERNEL && cmd == RTM_NEWLINK) { - zlog_debug("RTM_NEWLINK %s(%u, VRF %u) replaces %s(%u, VRF %u)\n", - ifp->name, - ifp->ifindex, - ifp->vrf_id, - other_ifp->name, - other_ifp->ifindex, - other_ifp->vrf_id); - } else if (IS_ZEBRA_DEBUG_KERNEL && cmd == RTM_DELLINK) { - zlog_debug("RTM_DELLINK %s(%u, VRF %u) is replaced by %s(%u, VRF %u)\n", - ifp->name, - ifp->ifindex, - ifp->vrf_id, - other_ifp->name, - other_ifp->ifindex, - other_ifp->vrf_id); - } - /* the found interface replaces the current one - * remove it - */ - if (cmd == RTM_DELLINK) - if_delete(ifp); - else - if_delete(other_ifp); - /* the found interface is replaced by the current one - * suppress it - */ -} - int netlink_link_change(struct nlmsghdr *h, ns_id_t ns_id, int startup) { int len; @@ -1276,8 +1215,6 @@ int netlink_link_change(struct nlmsghdr *h, ns_id_t ns_id, int startup) if (IS_ZEBRA_IF_BRIDGE_SLAVE(ifp)) zebra_l2if_update_bridge_slave(ifp, bridge_ifindex); - if_netlink_check_ifp_instance_consistency(RTM_NEWLINK, - ifp, ns_id); } else if (ifp->vrf_id != vrf_id) { /* VRF change for an interface. */ if (IS_ZEBRA_DEBUG_KERNEL) @@ -1351,8 +1288,6 @@ int netlink_link_change(struct nlmsghdr *h, ns_id_t ns_id, int startup) if (IS_ZEBRA_IF_BRIDGE_SLAVE(ifp) || was_bridge_slave) zebra_l2if_update_bridge_slave(ifp, bridge_ifindex); - if_netlink_check_ifp_instance_consistency(RTM_NEWLINK, - ifp, ns_id); } } else { /* Delete interface notification from kernel */ @@ -1376,8 +1311,6 @@ int netlink_link_change(struct nlmsghdr *h, ns_id_t ns_id, int startup) if (!IS_ZEBRA_IF_VRF(ifp)) if_delete_update(ifp); - if_netlink_check_ifp_instance_consistency(RTM_DELLINK, - ifp, ns_id); } return 0; diff --git a/zebra/interface.c b/zebra/interface.c index 763931d350..bb87728b5d 100644 --- a/zebra/interface.c +++ b/zebra/interface.c @@ -202,7 +202,6 @@ struct interface *if_link_per_ns(struct zebra_ns *ns, struct interface *ifp) if (rn->info) { ifp = (struct interface *)rn->info; route_unlock_node(rn); /* get */ - ifp->node = rn; return ifp; } @@ -253,30 +252,6 @@ struct interface *if_lookup_by_name_per_ns(struct zebra_ns *ns, return NULL; } -/* this function must be used only if the vrf backend - * is a netns backend - */ -struct interface *if_lookup_by_name_not_ns(ns_id_t ns_id, - const char *ifname) -{ - struct interface *ifp; - struct ns *ns; - - RB_FOREACH (ns, ns_head, &ns_tree) { - if (ns->ns_id == ns_id) - continue; - /* if_delete_update has removed interface - * from zns->if_table - * so to look for interface, use the vrf list - */ - ifp = if_lookup_by_name(ifname, (vrf_id_t)ns->ns_id); - if (!ifp) - continue; - return ifp; - } - return NULL; -} - const char *ifindex2ifname_per_ns(struct zebra_ns *zns, unsigned int ifindex) { struct interface *ifp; @@ -753,8 +728,12 @@ void if_delete_update(struct interface *ifp) ifp->node = NULL; /* if the ifp is in a vrf, move it to default so vrf can be deleted if - * desired */ - if (ifp->vrf_id) + * desired. This operation is not done for netns implementation to avoid + * collision with interface with the same name in the default vrf (can + * occur with this implementation whereas it is not possible with + * vrf-lite). + */ + if ((ifp->vrf_id) && !vrf_is_backend_netns()) if_handle_vrf_change(ifp, VRF_DEFAULT); /* Reset some zebra interface params to default values. */ diff --git a/zebra/interface.h b/zebra/interface.h index 9634bfdb3f..2add0a2de6 100644 --- a/zebra/interface.h +++ b/zebra/interface.h @@ -324,8 +324,6 @@ extern void zebra_if_init(void); extern struct interface *if_lookup_by_index_per_ns(struct zebra_ns *, uint32_t); extern struct interface *if_lookup_by_name_per_ns(struct zebra_ns *, const char *); -extern struct interface *if_lookup_by_name_not_ns(ns_id_t ns_id, - const char *ifname); extern struct interface *if_link_per_ns(struct zebra_ns *, struct interface *); extern const char *ifindex2ifname_per_ns(struct zebra_ns *, unsigned int); From 379eb245f649f04e2ceed0c577bed919dfebd5f6 Mon Sep 17 00:00:00 2001 From: Thibaut Collet Date: Thu, 30 Aug 2018 16:06:03 +0200 Subject: [PATCH 3/4] lib/if.c: fix CLANG warning Fix CLANG warning: Report for if.c | 2 issues =============================================== < WARNING: else is not generally useful after a break or return < #390: FILE: /tmp/f1-28557/if.c:390: Signed-off-by: Thibaut Collet --- lib/if.c | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/lib/if.c b/lib/if.c index c630dd140a..25da7a8188 100644 --- a/lib/if.c +++ b/lib/if.c @@ -387,31 +387,31 @@ struct interface *if_get_by_name(const char *name, vrf_id_t vrf_id, int vty) return NULL; } return if_create(name, vrf_id); - } else { - ifp = if_lookup_by_name_all_vrf(name); - if (ifp) { - if (ifp->vrf_id == vrf_id) - return ifp; - /* Found a match on a different VRF. If the interface - * command was entered in vty without a VRF (passed as - * VRF_DEFAULT), accept the ifp we found. If a vrf was - * entered and there is a mismatch, reject it if from - * vty. If it came from the kernel or by way of zclient, - * believe it and update the ifp accordingly. - */ - if (vty) { - if (vrf_id == VRF_DEFAULT) - return ifp; - return NULL; - } - /* If it came from the kernel or by way of zclient, - * believe it and update the ifp accordingly. - */ - if_update_to_new_vrf(ifp, vrf_id); - return ifp; - } - return if_create(name, vrf_id); } + /* vrf is based on vrf-lite */ + ifp = if_lookup_by_name_all_vrf(name); + if (ifp) { + if (ifp->vrf_id == vrf_id) + return ifp; + /* Found a match on a different VRF. If the interface command + * was entered in vty without a VRF (passed as VRF_DEFAULT), + * accept the ifp we found. If a vrf was entered and there is a + * mismatch, reject it if from vty. If it came from the kernel + * or by way of zclient, believe it and update the ifp + * accordingly. + */ + if (vty) { + if (vrf_id == VRF_DEFAULT) + return ifp; + return NULL; + } + /* If it came from the kernel or by way of zclient, believe it + * and update the ifp accordingly. + */ + if_update_to_new_vrf(ifp, vrf_id); + return ifp; + } + return if_create(name, vrf_id); } void if_set_index(struct interface *ifp, ifindex_t ifindex) From c3568c4d1a080bc0510894374e16c157be28e8e4 Mon Sep 17 00:00:00 2001 From: Thibaut Collet Date: Thu, 6 Sep 2018 07:48:12 +0200 Subject: [PATCH 4/4] zebra/lib: code cleaning Remove useless parenthesis and explicit cast. Remove redundant code. Signed-off-by: Thibaut Collet --- lib/vrf.c | 8 ++------ zebra/interface.c | 2 +- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/vrf.c b/lib/vrf.c index 696ae3f48c..3ab830c556 100644 --- a/lib/vrf.c +++ b/lib/vrf.c @@ -493,18 +493,14 @@ void vrf_init(int (*create)(struct vrf *), int (*enable)(struct vrf *), "vrf_init: failed to create the default VRF!"); exit(1); } - if (vrf_is_backend_netns()) - strlcpy(default_vrf->data.l.netns_name, - VRF_DEFAULT_NAME, NS_NAMSIZ); - if (vrf_is_backend_netns()) { struct ns *ns; strlcpy(default_vrf->data.l.netns_name, VRF_DEFAULT_NAME, NS_NAMSIZ); ns = ns_lookup(ns_get_default_id()); - ns->vrf_ctxt = (void *)default_vrf; - default_vrf->ns_ctxt = (void *)ns; + ns->vrf_ctxt = default_vrf; + default_vrf->ns_ctxt = ns; } /* Enable the default VRF. */ diff --git a/zebra/interface.c b/zebra/interface.c index bb87728b5d..48adfd5a3d 100644 --- a/zebra/interface.c +++ b/zebra/interface.c @@ -733,7 +733,7 @@ void if_delete_update(struct interface *ifp) * occur with this implementation whereas it is not possible with * vrf-lite). */ - if ((ifp->vrf_id) && !vrf_is_backend_netns()) + if (ifp->vrf_id && !vrf_is_backend_netns()) if_handle_vrf_change(ifp, VRF_DEFAULT); /* Reset some zebra interface params to default values. */