From cbc9aa025f52c90560a82d6d005d4c76aded9121 Mon Sep 17 00:00:00 2001 From: Neale Ranns Date: Sat, 13 May 2017 05:52:58 -0700 Subject: Fix FIB recursion loops via cover (VPP-842) Change-Id: Ia91c3e8cb27b9e4c1cccefc0a4857dd9995450ab Signed-off-by: Neale Ranns --- src/vnet/fib/fib_entry_src_rr.c | 78 +++++++++++++++++++++++------------------ src/vnet/fib/fib_path.c | 16 +++++++-- src/vnet/fib/fib_test.c | 30 ++++++++++++++++ 3 files changed, 87 insertions(+), 37 deletions(-) diff --git a/src/vnet/fib/fib_entry_src_rr.c b/src/vnet/fib/fib_entry_src_rr.c index ff15c54e281..cb70d3993b5 100644 --- a/src/vnet/fib/fib_entry_src_rr.c +++ b/src/vnet/fib/fib_entry_src_rr.c @@ -69,6 +69,47 @@ fib_entry_src_rr_init (fib_entry_src_t *src) src->rr.fesr_sibling = FIB_NODE_INDEX_INVALID; } + +/* + * use the path-list of the cover, unless it would form a loop. + * that is unless the cover is via this entry. + * If a loop were to form it would be a 1 level loop (i.e. X via X), + * and there would be 2 locks on the path-list; one since its used + * by the cover, and 1 from here. The first lock will go when the + * cover is removed, the second, and last, when the covered walk + * occurs during the cover's removel - this is not a place where + * we can handle last lock gone. + * In short, don't let the loop form. The usual rules of 'we must + * let it form so we know when it breaks' don't apply here, since + * the loop will break when the cover changes, and this function + * will be called again when that happens. + */ +static void +fib_entry_src_rr_use_covers_pl (fib_entry_src_t *src, + const fib_entry_t *fib_entry, + const fib_entry_t *cover) +{ + fib_node_index_t *entries = NULL; + fib_protocol_t proto; + + proto = fib_entry->fe_prefix.fp_proto; + vec_add1(entries, fib_entry_get_index(fib_entry)); + + if (fib_path_list_recursive_loop_detect(cover->fe_parent, + &entries)) + { + src->fes_pl = fib_path_list_create_special( + proto, + FIB_PATH_LIST_FLAG_DROP, + drop_dpo_get(fib_proto_to_dpo(proto))); + } + else + { + src->fes_pl = cover->fe_parent; + } + vec_free(entries); +} + /* * Source activation. Called when the source is the new best source on the entry */ @@ -112,40 +153,7 @@ fib_entry_src_rr_activate (fib_entry_src_t *src, } else { - /* - * use the path-list of the cover, unless it would form a loop. - * that is unless the cover is via this entry. - * If a loop were to form it would be a 1 level loop (i.e. X via X), - * and there would be 2 locks on the path-list; one since its used - * by the cover, and 1 from here. The first lock will go when the - * cover is removed, the second, and last, when the covered walk - * occurs during the cover's removel - this is not a place where - * we can handle last lock gone. - * In short, don't let the loop form. The usual rules of 'we must - * let it form so we know when it breaks' don't apply here, since - * the loop will break when the cover changes, and this function - * will be called again when that happens. - */ - fib_node_index_t *entries = NULL; - fib_protocol_t proto; - - proto = fib_entry->fe_prefix.fp_proto; - vec_add1(entries, fib_entry_get_index(fib_entry)); - - if (fib_path_list_recursive_loop_detect(cover->fe_parent, - &entries)) - { - src->fes_pl = fib_path_list_create_special( - proto, - FIB_PATH_LIST_FLAG_DROP, - drop_dpo_get(fib_proto_to_dpo(proto))); - } - else - { - src->fes_pl = cover->fe_parent; - } - vec_free(entries); - + fib_entry_src_rr_use_covers_pl(src, fib_entry, cover); } fib_path_list_lock(src->fes_pl); @@ -256,7 +264,7 @@ fib_entry_src_rr_cover_update (fib_entry_src_t *src, } else { - src->fes_pl = cover->fe_parent; + fib_entry_src_rr_use_covers_pl(src, fib_entry, cover); } fib_path_list_lock(src->fes_pl); fib_path_list_unlock(old_path_list); diff --git a/src/vnet/fib/fib_path.c b/src/vnet/fib/fib_path.c index e7338a8c56c..090c5026ce2 100644 --- a/src/vnet/fib/fib_path.c +++ b/src/vnet/fib/fib_path.c @@ -1634,7 +1634,11 @@ fib_path_get_resolving_interface (fib_node_index_t path_index) case FIB_PATH_TYPE_RECEIVE: return (path->receive.fp_interface); case FIB_PATH_TYPE_RECURSIVE: - return (fib_entry_get_resolving_interface(path->fp_via_fib)); + if (fib_path_is_resolved(path_index)) + { + return (fib_entry_get_resolving_interface(path->fp_via_fib)); + } + break; case FIB_PATH_TYPE_SPECIAL: case FIB_PATH_TYPE_DEAG: case FIB_PATH_TYPE_EXCLUSIVE: @@ -1698,7 +1702,15 @@ fib_path_contribute_urpf (fib_node_index_t path_index, break; case FIB_PATH_TYPE_RECURSIVE: - fib_entry_contribute_urpf(path->fp_via_fib, urpf); + if (FIB_NODE_INDEX_INVALID != path->fp_via_fib && + !fib_path_is_looped(path_index)) + { + /* + * there's unresolved due to constraints, and there's unresolved + * due to ain't got no via. can't do nowt w'out via. + */ + fib_entry_contribute_urpf(path->fp_via_fib, urpf); + } break; case FIB_PATH_TYPE_EXCLUSIVE: diff --git a/src/vnet/fib/fib_test.c b/src/vnet/fib/fib_test.c index baf66738ef6..d3cda929e3a 100644 --- a/src/vnet/fib/fib_test.c +++ b/src/vnet/fib/fib_test.c @@ -2841,6 +2841,36 @@ fib_test_v4 (void) FIB_TEST((ENBR+7 == fib_entry_pool_size()), "entry pool size is %d", fib_entry_pool_size()); + /* + * Make the default route recursive via a unknown next-hop. Thus the + * next hop's cover would be the default route + */ + fei = fib_table_entry_path_add(fib_index, + &pfx_0_0_0_0_s_0, + FIB_SOURCE_API, + FIB_ENTRY_FLAG_NONE, + FIB_PROTOCOL_IP4, + &pfx_23_23_23_23_s_32.fp_addr, + ~0, // recursive + fib_index, + 1, + NULL, + FIB_ROUTE_PATH_FLAG_NONE); + dpo = fib_entry_contribute_ip_forwarding(fei); + FIB_TEST(load_balance_is_drop(dpo), + "0.0.0.0.0/0 via is DROP"); + FIB_TEST((fib_entry_get_resolving_interface(fei) == ~0), + "no resolving interface for looped 0.0.0.0/0"); + + fei = fib_table_lookup_exact_match(fib_index, &pfx_23_23_23_23_s_32); + dpo = fib_entry_contribute_ip_forwarding(fei); + FIB_TEST(load_balance_is_drop(dpo), + "23.23.23.23/32 via is DROP"); + FIB_TEST((fib_entry_get_resolving_interface(fei) == ~0), + "no resolving interface for looped 23.23.23.23/32"); + + fib_table_entry_delete(fib_index, &pfx_0_0_0_0_s_0, FIB_SOURCE_API); + /* * A recursive route with recursion constraints. * 200.200.200.200/32 via 1.1.1.1 is recurse via host constrained -- cgit 1.2.3-korg