From 2b6b16fc21d35693914231509c8710eff6a58c4a Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Tue, 20 Apr 2021 06:11:57 +0200 Subject: [PATCH] lib, pimd: add address match mode to prefix lists ... the PIM code is kinda misusing prefix lists to match addresses. Considering the weird semantics of access-lists, I can't fault it. However, prefix lists aren't great at matching addresses by default, since they try to match the prefix length too. So, here's an "address match mode" for prefix lists to get that to work more reasonably. Fixes: #8492 Signed-off-by: David Lamparter --- lib/plist.c | 20 ++++++++++++-------- lib/plist.h | 15 +++++++++++---- pimd/pim_rp.c | 33 +++++++++++++++++++++++++++------ 3 files changed, 50 insertions(+), 18 deletions(-) diff --git a/lib/plist.c b/lib/plist.c index 0ee02f8a0b..1ee855a594 100644 --- a/lib/plist.c +++ b/lib/plist.c @@ -750,7 +750,7 @@ static const char *prefix_list_type_str(struct prefix_list_entry *pentry) } static int prefix_list_entry_match(struct prefix_list_entry *pentry, - const struct prefix *p) + const struct prefix *p, bool address_mode) { int ret; @@ -761,6 +761,9 @@ static int prefix_list_entry_match(struct prefix_list_entry *pentry, if (!ret) return 0; + if (address_mode) + return 1; + /* In case of le nor ge is specified, exact match is performed. */ if (!pentry->le && !pentry->ge) { if (pentry->prefix.prefixlen != p->prefixlen) @@ -777,14 +780,15 @@ static int prefix_list_entry_match(struct prefix_list_entry *pentry, return 1; } -enum prefix_list_type prefix_list_apply_which_prefix( +enum prefix_list_type prefix_list_apply_ext( struct prefix_list *plist, - const struct prefix **which, - const void *object) + const struct prefix_list_entry **which, + union prefixconstptr object, + bool address_mode) { struct prefix_list_entry *pentry, *pbest = NULL; - const struct prefix *p = (const struct prefix *)object; + const struct prefix *p = object.p; const uint8_t *byte = p->u.val; size_t depth; size_t validbits = p->prefixlen; @@ -809,7 +813,7 @@ enum prefix_list_type prefix_list_apply_which_prefix( pentry = pentry->next_best) { if (pbest && pbest->seq < pentry->seq) continue; - if (prefix_list_entry_match(pentry, p)) + if (prefix_list_entry_match(pentry, p, address_mode)) pbest = pentry; } @@ -830,7 +834,7 @@ enum prefix_list_type prefix_list_apply_which_prefix( pentry = pentry->next_best) { if (pbest && pbest->seq < pentry->seq) continue; - if (prefix_list_entry_match(pentry, p)) + if (prefix_list_entry_match(pentry, p, address_mode)) pbest = pentry; } break; @@ -838,7 +842,7 @@ enum prefix_list_type prefix_list_apply_which_prefix( if (which) { if (pbest) - *which = &pbest->prefix; + *which = pbest; else *which = NULL; } diff --git a/lib/plist.h b/lib/plist.h index 57eb763a68..c9507df57c 100644 --- a/lib/plist.h +++ b/lib/plist.h @@ -37,6 +37,7 @@ enum prefix_list_type { }; struct prefix_list; +struct prefix_list_entry; struct orf_prefix { uint32_t seq; @@ -63,12 +64,18 @@ extern struct prefix_list *prefix_list_lookup(afi_t, const char *); * * If no pointer is sent in, do not return anything. * If it is a empty plist return a NULL pointer. + * + * address_mode = the "prefix" being passed in is really an address, match + * regardless of prefix length (i.e. ge/le are ignored.) prefix->prefixlen + * must be /32. */ extern enum prefix_list_type -prefix_list_apply_which_prefix(struct prefix_list *plist, - const struct prefix **which, - const void *object); -#define prefix_list_apply(A, B) prefix_list_apply_which_prefix((A), NULL, (B)) +prefix_list_apply_ext(struct prefix_list *plist, + const struct prefix_list_entry **matches, + union prefixconstptr prefix, + bool address_mode); +#define prefix_list_apply(A, B) \ + prefix_list_apply_ext((A), NULL, (B), false) extern struct prefix_list *prefix_bgp_orf_lookup(afi_t, const char *); extern struct stream *prefix_bgp_orf_entry(struct stream *, diff --git a/pimd/pim_rp.c b/pimd/pim_rp.c index b6521132f7..56e1927528 100644 --- a/pimd/pim_rp.c +++ b/pimd/pim_rp.c @@ -203,6 +203,26 @@ static struct rp_info *pim_rp_find_exact(struct pim_instance *pim, return NULL; } +/* + * XXX: long-term issue: we don't actually have a good "ip address-list" + * implementation. ("access-list XYZ" is the closest but honestly it's + * kinda garbage.) + * + * So it's using a prefix-list to match an address here, which causes very + * unexpected results for the user since prefix-lists by default only match + * when the prefix length is an exact match too. i.e. you'd have to add the + * "le 32" and do "ip prefix-list foo permit 10.0.0.0/24 le 32" + * + * To avoid this pitfall, this code uses "address_mode = true" for the prefix + * list match (this is the only user for that.) + * + * In the long run, we need to add a "ip address-list", but that's a wholly + * separate bag of worms, and existing configs using ip prefix-list would + * drop into the UX pitfall. + */ + +#include "lib/plist_int.h" + /* * Given a group, return the rp_info for that group */ @@ -213,7 +233,8 @@ struct rp_info *pim_rp_find_match_group(struct pim_instance *pim, struct rp_info *best = NULL; struct rp_info *rp_info; struct prefix_list *plist; - const struct prefix *p, *bp; + const struct prefix *bp; + const struct prefix_list_entry *entry; struct route_node *rn; bp = NULL; @@ -221,19 +242,19 @@ struct rp_info *pim_rp_find_match_group(struct pim_instance *pim, if (rp_info->plist) { plist = prefix_list_lookup(AFI_IP, rp_info->plist); - if (prefix_list_apply_which_prefix(plist, &p, group) - == PREFIX_DENY) + if (prefix_list_apply_ext(plist, &entry, group, true) + == PREFIX_DENY || !entry) continue; if (!best) { best = rp_info; - bp = p; + bp = &entry->prefix; continue; } - if (bp && bp->prefixlen < p->prefixlen) { + if (bp && bp->prefixlen < entry->prefix.prefixlen) { best = rp_info; - bp = p; + bp = &entry->prefix; } } } -- 2.39.5