Hi Robert,
Thanks for the patch
I see two ways to fix this: either add a check at creation time so that
no more than 2^23 tbl8 groups are accepted (which should generally be
fine, although there may be exceptions, hence option 2), or widen the
variables holding tbl8 indexes, as this patch does.
This patch fixes the issue in the "roughly 630,000 to 650,000 routes"
range, but not completely. It only gains a single bit, so the problem
reappears at tbl8 group 2^24. With /128 routes as in your test, that is
at about 1.3M routes, and this time lookups silently return wrong next
hops instead of crashing. I'd encourage you to widen these indexes to
uint64_t, not only in build_common_root(), but also in get_idx(),
get_tbl_pos() and get_tbl_val_by_idx().
On 10/21/2026 7:18 PM, Robert Sanford wrote:
From: Robert Sanford <[email protected]>
In build_common_root(), get_idx() returns a uint32_t, but the result
was stored in an int. Once the tbl8 group index reaches 2^23
(prev_idx * 256 reaches 2^31), the value has its high bit set and
goes negative. It is then sign-extended when converted to the
uint64_t index passed to get_tbl_val_by_idx() and
get_tbl_p_by_idx(), which yields a bogus table pointer.
This was hit in practice: a modified fib6_perf_autotest with 750,000
random /128 routes crashed with SIGSEGV in install_to_dp(), where
build_common_root() is inlined, when tbl8 group 0x800001 was
allocated (after roughly 630,000 to 650,000 routes). The faulting
store went to the table base plus the sign-extended index 0x800000a4
(a negative int), which wrapped to an unmapped address. With this
change the same test runs to completion.
Make idx and prev_idx unsigned, and use a separate signed tbl8_idx
for the tbl8_alloc() result, which returns a negative value on error.
Also cast tbl8_idx to uint64_t before shifting it left, in
build_common_root() and in write_edge(). The shift was done in int,
which can overflow before the result is widened.
The problem is also visible at compile time: GCC's -Wsign-conversion
reports the original get_idx() assignment, without having to build a
table large enough to hit it.
Fixes: c3e12e0f0354 ("fib: add dataplane algorithm for IPv6")
Cc: [email protected]
Signed-off-by: Robert Sanford <[email protected]>
---
v2: use a 12-character sha in the Fixes tag.
lib/fib/trie.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/lib/fib/trie.c b/lib/fib/trie.c
index 187360d1c8..95f79e26a5 100644
--- a/lib/fib/trie.c
+++ b/lib/fib/trie.c
@@ -317,7 +317,8 @@ build_common_root(struct rte_trie_tbl *dp, const struct
rte_ipv6_addr *ip,
void *tbl_ptr = NULL;
uint64_t *cur_tbl;
uint64_t val;
- int i, j, idx, prev_idx = 0;
+ uint32_t idx, prev_idx = 0;
+ int i, j, tbl8_idx;
cur_tbl = dp->tbl24;
for (i = 3, j = 0; i <= common_bytes; i++) {
@@ -325,12 +326,12 @@ build_common_root(struct rte_trie_tbl *dp, const struct
rte_ipv6_addr *ip,
val = get_tbl_val_by_idx(cur_tbl, idx, dp->nh_sz);
tbl_ptr = get_tbl_p_by_idx(cur_tbl, idx, dp->nh_sz);
if ((val & TRIE_EXT_ENT) != TRIE_EXT_ENT) {
- idx = tbl8_alloc(dp, val);
- if (unlikely(idx < 0))
- return idx;
- write_to_dp(tbl_ptr, (idx << 1) |
+ tbl8_idx = tbl8_alloc(dp, val);
+ if (unlikely(tbl8_idx < 0))
+ return tbl8_idx;
+ write_to_dp(tbl_ptr, ((uint64_t)tbl8_idx << 1) |
TRIE_EXT_ENT, dp->nh_sz, 1);
- prev_idx = idx;
+ prev_idx = (uint32_t)tbl8_idx;
} else
prev_idx = val >> 1;
@@ -359,7 +360,7 @@ write_edge(struct rte_trie_tbl *dp, const uint8_t *ip_part, uint64_t next_hop,
tbl8_idx = tbl8_alloc(dp, val);
if (tbl8_idx < 0)
return tbl8_idx;
- val = (tbl8_idx << 1)|TRIE_EXT_ENT;
+ val = ((uint64_t)tbl8_idx << 1) | TRIE_EXT_ENT;
}
p = get_tbl_p_by_idx(dp->tbl8, (tbl8_idx *
TRIE_TBL8_GRP_NUM_ENT) + *ip_part, dp->nh_sz);
--
Regards,
Vladimir
