From c4097b758e5d13fbd01306167079ab608165ec3a Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 15 Oct 2019 13:42:15 -0400 Subject: [PATCH 1/4] zebra: Only free if rule was found/release in table We were seeing a double free on shutdown if the hash release fails here due to the interface state changing. We probably shouldn't free the data if its still being handled in the table so adding a check there and a debug message. Signed-off-by: Stephen Worley --- zebra/zebra_pbr.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c index f95a4ff950..9a7d506731 100644 --- a/zebra/zebra_pbr.c +++ b/zebra/zebra_pbr.c @@ -471,8 +471,12 @@ static void zebra_pbr_cleanup_rules(struct hash_bucket *b, void *data) if (rule->sock == *sock) { (void)kernel_del_pbr_rule(rule); - hash_release(zrouter.rules_hash, rule); - XFREE(MTYPE_TMP, rule); + if (hash_release(zrouter.rules_hash, rule)) + XFREE(MTYPE_TMP, rule); + else + zlog_debug( + "%s: Rule seq: %u is being cleaned but we can't find it in our tables", + __func__, rule->rule.seq); } } From 3dabc387a6f846debc944eb42bcab34cdee67d9b Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 15 Oct 2019 14:48:33 -0400 Subject: [PATCH 2/4] lib: Use ifindex_t for struct pbr_rule We should be using the ifindex_t typedef here for the type, not uint32_t. Signed-off-by: Stephen Worley --- lib/pbr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pbr.h b/lib/pbr.h index ecd50447e5..cf6ac41d32 100644 --- a/lib/pbr.h +++ b/lib/pbr.h @@ -90,7 +90,7 @@ struct pbr_rule { uint32_t unique; struct pbr_filter filter; struct pbr_action action; - uint32_t ifindex; + ifindex_t ifindex; }; /* TCP flags value shared From b77a69bdc601133612d89cb5e389f014d3e68970 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 15 Oct 2019 14:50:10 -0400 Subject: [PATCH 3/4] zebra: Use the rule ifindex as a hash key, not ifp Use the ifindex value as a primary hash key/identifier, not the ifp pointer. It is possible for that data to be freed and then we would not be able to hash and find the rule entry anymore. Using the ifindex, we can still find the rule even if the interface is removed. Signed-off-by: Stephen Worley --- zebra/rule_netlink.c | 2 +- zebra/zapi_msg.c | 12 +++++------- zebra/zebra_pbr.c | 17 ++++++----------- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/zebra/rule_netlink.c b/zebra/rule_netlink.c index 711c4e0877..1b465b0335 100644 --- a/zebra/rule_netlink.c +++ b/zebra/rule_netlink.c @@ -123,7 +123,7 @@ static int netlink_rule_update(int cmd, struct zebra_pbr_rule *rule) "Tx %s family %s IF %s(%u) Pref %u Fwmark %u Src %s Dst %s Table %u", nl_msg_type_to_str(cmd), nl_family_to_str(family), rule->ifp ? rule->ifp->name : "Unknown", - rule->ifp ? rule->ifp->ifindex : 0, rule->rule.priority, + rule->rule.ifindex, rule->rule.priority, rule->rule.filter.fwmark, prefix2str(&rule->rule.filter.src_ip, buf1, sizeof(buf1)), diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index b0488b7559..96faf6f1f7 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -2294,7 +2294,6 @@ static inline void zread_rule(ZAPI_HANDLER_ARGS) struct zebra_pbr_rule zpr; struct stream *s; uint32_t total, i; - ifindex_t ifindex; s = msg; STREAM_GETL(s, total); @@ -2319,15 +2318,14 @@ static inline void zread_rule(ZAPI_HANDLER_ARGS) STREAM_GETW(s, zpr.rule.filter.dst_port); STREAM_GETL(s, zpr.rule.filter.fwmark); STREAM_GETL(s, zpr.rule.action.table); - STREAM_GETL(s, ifindex); + STREAM_GETL(s, zpr.rule.ifindex); - if (ifindex) { - zpr.ifp = if_lookup_by_index_per_ns( - zvrf->zns, - ifindex); + if (zpr.rule.ifindex) { + zpr.ifp = if_lookup_by_index_per_ns(zvrf->zns, + zpr.rule.ifindex); if (!zpr.ifp) { zlog_debug("Failed to lookup ifindex: %u", - ifindex); + zpr.rule.ifindex); return; } } diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c index 9a7d506731..e77e29f13a 100644 --- a/zebra/zebra_pbr.c +++ b/zebra/zebra_pbr.c @@ -144,17 +144,12 @@ uint32_t zebra_pbr_rules_hash_key(const void *arg) key = jhash_3words(rule->rule.seq, rule->rule.priority, rule->rule.action.table, prefix_hash_key(&rule->rule.filter.src_ip)); - if (rule->ifp) - key = jhash_1word(rule->ifp->ifindex, key); - else - key = jhash_1word(0, key); if (rule->rule.filter.fwmark) - key = jhash_1word(rule->rule.filter.fwmark, key); + key = jhash_3words(rule->rule.filter.fwmark, rule->vrf_id, + rule->rule.ifindex, key); else - key = jhash_1word(0, key); - - key = jhash_1word(rule->vrf_id, key); + key = jhash_2words(rule->vrf_id, rule->rule.ifindex, key); return jhash_3words(rule->rule.filter.src_port, rule->rule.filter.dst_port, @@ -208,7 +203,7 @@ bool zebra_pbr_rules_hash_equal(const void *arg1, const void *arg2) struct pbr_rule_unique_lookup { struct zebra_pbr_rule *rule; uint32_t unique; - struct interface *ifp; + ifindex_t ifindex; vrf_id_t vrf_id; }; @@ -218,7 +213,7 @@ static int pbr_rule_lookup_unique_walker(struct hash_bucket *b, void *data) struct zebra_pbr_rule *rule = b->data; if (pul->unique == rule->rule.unique - && pul->ifp == rule->ifp + && pul->ifindex == rule->rule.ifindex && pul->vrf_id == rule->vrf_id) { pul->rule = rule; return HASHWALK_ABORT; @@ -233,7 +228,7 @@ pbr_rule_lookup_unique(struct zebra_pbr_rule *zrule) struct pbr_rule_unique_lookup pul; pul.unique = zrule->rule.unique; - pul.ifp = zrule->ifp; + pul.ifindex = zrule->rule.ifindex; pul.rule = NULL; pul.vrf_id = zrule->vrf_id; hash_walk(zrouter.rules_hash, &pbr_rule_lookup_unique_walker, &pul); From b19d55d048f6a644629595da361df9e6d0674555 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 15 Oct 2019 15:39:49 -0400 Subject: [PATCH 4/4] zebra: Don't bother ref'ing ifp in zebra_pbr_rule If we only really use the ifp for the name, then don't bother referencing the ifp. If that ifp is freed, we don't expect zebra to handle the rules that use it (that's pbrd's job), so it is going to be pointing to unintialized memory when we decide to remove that rule later. Thus, just keep the name in the data and dont mess with pointer refs. Signed-off-by: Stephen Worley --- zebra/rule_netlink.c | 20 ++++++++++---------- zebra/zapi_msg.c | 15 ++++++++------- zebra/zebra_pbr.c | 2 +- zebra/zebra_pbr.h | 2 +- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/zebra/rule_netlink.c b/zebra/rule_netlink.c index 1b465b0335..2fdb215128 100644 --- a/zebra/rule_netlink.c +++ b/zebra/rule_netlink.c @@ -86,9 +86,8 @@ static int netlink_rule_update(int cmd, struct zebra_pbr_rule *rule) addattr32(&req.n, sizeof(req), FRA_PRIORITY, rule->rule.priority); /* interface on which applied */ - if (rule->ifp) - addattr_l(&req.n, sizeof(req), FRA_IFNAME, rule->ifp->name, - strlen(rule->ifp->name) + 1); + addattr_l(&req.n, sizeof(req), FRA_IFNAME, rule->ifname, + strlen(rule->ifname) + 1); /* source IP, if specified */ if (IS_RULE_FILTERING_ON_SRC_IP(rule)) { @@ -122,8 +121,7 @@ static int netlink_rule_update(int cmd, struct zebra_pbr_rule *rule) zlog_debug( "Tx %s family %s IF %s(%u) Pref %u Fwmark %u Src %s Dst %s Table %u", nl_msg_type_to_str(cmd), nl_family_to_str(family), - rule->ifp ? rule->ifp->name : "Unknown", - rule->rule.ifindex, rule->rule.priority, + rule->ifname, rule->rule.ifindex, rule->rule.priority, rule->rule.filter.fwmark, prefix2str(&rule->rule.filter.src_ip, buf1, sizeof(buf1)), @@ -227,13 +225,15 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup) if (tb[FRA_IFNAME] == NULL) return 0; - /* If we don't know the interface, we don't care. */ ifname = (char *)RTA_DATA(tb[FRA_IFNAME]); zns = zebra_ns_lookup(ns_id); - rule.ifp = if_lookup_by_name_per_ns(zns, ifname); - if (!rule.ifp) + + /* If we don't know the interface, we don't care. */ + if (!if_lookup_by_name_per_ns(zns, ifname)) return 0; + strlcpy(rule.ifname, ifname, sizeof(rule.ifname)); + if (tb[FRA_PRIORITY]) rule.rule.priority = *(uint32_t *)RTA_DATA(tb[FRA_PRIORITY]); @@ -268,8 +268,8 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup) zlog_debug( "Rx %s family %s IF %s(%u) Pref %u Src %s Dst %s Table %u", nl_msg_type_to_str(h->nlmsg_type), - nl_family_to_str(frh->family), rule.ifp->name, - rule.ifp->ifindex, rule.rule.priority, + nl_family_to_str(frh->family), rule.ifname, + rule.rule.ifindex, rule.rule.priority, prefix2str(&rule.rule.filter.src_ip, buf1, sizeof(buf1)), prefix2str(&rule.rule.filter.dst_ip, buf2, diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 96faf6f1f7..e61e68b7fe 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -783,10 +783,7 @@ void zsend_rule_notify_owner(struct zebra_pbr_rule *rule, stream_putl(s, rule->rule.seq); stream_putl(s, rule->rule.priority); stream_putl(s, rule->rule.unique); - if (rule->ifp) - stream_putl(s, rule->ifp->ifindex); - else - stream_putl(s, 0); + stream_putl(s, rule->rule.ifindex); stream_putw_at(s, 0, stream_get_endp(s)); @@ -2321,13 +2318,17 @@ static inline void zread_rule(ZAPI_HANDLER_ARGS) STREAM_GETL(s, zpr.rule.ifindex); if (zpr.rule.ifindex) { - zpr.ifp = if_lookup_by_index_per_ns(zvrf->zns, - zpr.rule.ifindex); - if (!zpr.ifp) { + struct interface *ifp; + + ifp = if_lookup_by_index_per_ns(zvrf->zns, + zpr.rule.ifindex); + if (!ifp) { zlog_debug("Failed to lookup ifindex: %u", zpr.rule.ifindex); return; } + + strlcpy(zpr.ifname, ifp->name, sizeof(zpr.ifname)); } if (!is_default_prefix(&zpr.rule.filter.src_ip)) diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c index e77e29f13a..e24d2e2b42 100644 --- a/zebra/zebra_pbr.c +++ b/zebra/zebra_pbr.c @@ -191,7 +191,7 @@ bool zebra_pbr_rules_hash_equal(const void *arg1, const void *arg2) if (!prefix_same(&r1->rule.filter.dst_ip, &r2->rule.filter.dst_ip)) return false; - if (r1->ifp != r2->ifp) + if (r1->rule.ifindex != r2->rule.ifindex) return false; if (r1->vrf_id != r2->vrf_id) diff --git a/zebra/zebra_pbr.h b/zebra/zebra_pbr.h index fcc9c5c39a..b7fbc9b7d5 100644 --- a/zebra/zebra_pbr.h +++ b/zebra/zebra_pbr.h @@ -41,7 +41,7 @@ struct zebra_pbr_rule { struct pbr_rule rule; - struct interface *ifp; + char ifname[INTERFACE_NAMSIZ]; vrf_id_t vrf_id; };