From 9160d7bcd987b634e31689af1eb931143a4ba538 Mon Sep 17 00:00:00 2001 From: "G. Paul Ziemba" Date: Tue, 12 Mar 2024 15:29:02 -0700 Subject: [PATCH] zebra: pbr hash cleanup WIP In the zebra PBR rule database, the hash was previously calculated over many fields, including most of the filter and action values. But a given rule should be identifiable by just: 1. the identity of its originating daemon (ZAPI socket FD); and 2. the 'unique' field in struct pbr_rule However, pbrd assigns a 'unique' field value per map (which may be assigned to multiple interfaces, yielding multiple corresponding rules). We could possibly modify pbrd to generate a 'unique' field value per rule instance, but the simpler solution for the moment is to also hash on the interface name. This commit changes the PBR hash function to use only the following parameters: 1. the identity of its originating daemon (ZAPI socket FD); and 2. the 'unique' field in struct pbr_rule 3. the interface name The goal of the above change is to get a better handle on what parameters uniquely identify a rule, to (eventually) allow the assignment or parallel generation of a globally-unique identifier for each rule that can be used with a dataplane. This commit also removes the inefficient pbr_rule_lookup_unique() code that iterated linearly over the entire rule database. Signed-off-by: G. Paul Ziemba --- lib/pbr.h | 2 +- zebra/zebra_pbr.c | 104 ++++++---------------------------------------- 2 files changed, 14 insertions(+), 92 deletions(-) diff --git a/lib/pbr.h b/lib/pbr.h index fe2d32a44a17..937a8b848c02 100644 --- a/lib/pbr.h +++ b/lib/pbr.h @@ -134,7 +134,7 @@ struct pbr_rule { uint32_t seq; uint32_t priority; - uint32_t unique; + uint32_t unique; /* scope: originating daemon + interface */ struct pbr_filter filter; struct pbr_action action; diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c index 7f3635702f82..fb9dc643117e 100644 --- a/zebra/zebra_pbr.c +++ b/zebra/zebra_pbr.c @@ -156,31 +156,24 @@ void zebra_pbr_rules_free(void *arg) uint32_t zebra_pbr_rules_hash_key(const void *arg) { - const struct zebra_pbr_rule *rule; + const struct zebra_pbr_rule *rule = arg; uint32_t key; - rule = arg; - key = jhash_3words(rule->rule.seq, rule->rule.priority, - rule->rule.action.table, - prefix_hash_key(&rule->rule.filter.src_ip)); - - key = jhash_3words(rule->rule.filter.fwmark, rule->vrf_id, - rule->rule.filter.ip_proto, key); + key = jhash_2words(rule->rule.unique, rule->sock, 0); key = jhash(rule->ifname, strlen(rule->ifname), key); - key = jhash_3words(rule->rule.filter.pcp, rule->rule.filter.vlan_id, - rule->rule.filter.vlan_flags, key); - - key = jhash_3words(rule->rule.filter.src_port, - rule->rule.filter.dst_port, - prefix_hash_key(&rule->rule.filter.dst_ip), key); - - key = jhash_2words(rule->rule.unique, rule->sock, key); - return key; } +/* + * In zebra's PBR rule database, a rule is uniquely identified by + * two fields: + * + * socket number (i.e., originating daemon) + * interface name + * unique field (originating daemon must ensure unique for itself+interface) + */ bool zebra_pbr_rules_hash_equal(const void *arg1, const void *arg2) { const struct zebra_pbr_rule *r1, *r2; @@ -188,86 +181,18 @@ bool zebra_pbr_rules_hash_equal(const void *arg1, const void *arg2) r1 = (const struct zebra_pbr_rule *)arg1; r2 = (const struct zebra_pbr_rule *)arg2; - if (r1->rule.seq != r2->rule.seq) - return false; - - if (r1->rule.priority != r2->rule.priority) - return false; - if (r1->sock != r2->sock) return false; - if (r1->rule.unique != r2->rule.unique) - return false; - - if (r1->rule.action.table != r2->rule.action.table) - return false; - - if (r1->rule.filter.src_port != r2->rule.filter.src_port) - return false; - - if (r1->rule.filter.dst_port != r2->rule.filter.dst_port) - return false; - - if (r1->rule.filter.fwmark != r2->rule.filter.fwmark) - return false; - - if (r1->rule.filter.ip_proto != r2->rule.filter.ip_proto) - return false; - - if (!prefix_same(&r1->rule.filter.src_ip, &r2->rule.filter.src_ip)) - return false; - - if (!prefix_same(&r1->rule.filter.dst_ip, &r2->rule.filter.dst_ip)) - return false; - if (strcmp(r1->rule.ifname, r2->rule.ifname) != 0) return false; - if (r1->vrf_id != r2->vrf_id) + if (r1->rule.unique != r2->rule.unique) return false; return true; } -struct pbr_rule_unique_lookup { - struct zebra_pbr_rule *rule; - int sock; - uint32_t unique; - char ifname[IFNAMSIZ + 1]; - vrf_id_t vrf_id; -}; - -static int pbr_rule_lookup_unique_walker(struct hash_bucket *b, void *data) -{ - struct pbr_rule_unique_lookup *pul = data; - struct zebra_pbr_rule *rule = b->data; - - if (pul->sock == rule->sock && pul->unique == rule->rule.unique && - strmatch(pul->ifname, rule->rule.ifname) && - pul->vrf_id == rule->vrf_id) { - pul->rule = rule; - return HASHWALK_ABORT; - } - - return HASHWALK_CONTINUE; -} - -static struct zebra_pbr_rule * -pbr_rule_lookup_unique(struct zebra_pbr_rule *zrule) -{ - struct pbr_rule_unique_lookup pul; - - pul.unique = zrule->rule.unique; - strlcpy(pul.ifname, zrule->rule.ifname, IFNAMSIZ); - pul.rule = NULL; - pul.vrf_id = zrule->vrf_id; - pul.sock = zrule->sock; - hash_walk(zrouter.rules_hash, &pbr_rule_lookup_unique_walker, &pul); - - return pul.rule; -} - void zebra_pbr_ipset_free(void *arg) { struct zebra_pbr_ipset *ipset; @@ -702,11 +627,8 @@ void zebra_pbr_add_rule(struct zebra_pbr_rule *rule) struct zebra_pbr_rule *old; struct zebra_pbr_rule *new; - /** - * Check if we already have it (this checks via a unique ID, walking - * over the hash table, not via a hash operation). - */ - found = pbr_rule_lookup_unique(rule); + /* Check if we already have it */ + found = hash_lookup(zrouter.rules_hash, rule); /* If found, this is an update */ if (found) {