Skip to content

Commit

Permalink
Fix off-by-32 bits in LPM tree prefix length
Browse files Browse the repository at this point in the history
The intergration tests started failing starting with kernel 6.8, and
after some tests, it pointed to this commit
torvalds/linux@9b75dbe.

The tests fail now because the prefix length we pass is 32 bits higher
(see initialiser) than the maximum prefix length. After this commit was
introduced, any prefix length that's higher than the key size will
return NULL earlier, rather than returning the value with the longest
match, even if it's beyond the size the key can encode.

Test Plan
=========

Ran the tests and some profiles on a box running 6.8.7-200 (Fedora) without issues.
  • Loading branch information
javierhonduco committed May 1, 2024
1 parent a932078 commit 64663e5
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 11 deletions.
9 changes: 7 additions & 2 deletions src/bpf/profiler_bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,14 @@ unsafe impl Plain for exec_mappings_key {}
unsafe impl Plain for mapping_t {}

impl exec_mappings_key {
pub fn new(pid: u32, address: u64, prefix: u32) -> Self {
pub fn new(pid: u32, address: u64, prefix_len: u32) -> Self {
let key_size_bits = std::mem::size_of::<Self>() * 8;
if prefix_len > key_size_bits.try_into().unwrap() {
panic!("prefix_len {} should be <= than the size of exec_mappings_key {}", prefix_len, key_size_bits);
}

Self {
prefix_len: 32 + prefix,
prefix_len,
pid: pid.to_be(),
data: address.to_be(),
}
Expand Down
2 changes: 1 addition & 1 deletion src/profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -924,7 +924,7 @@ impl Profiler<'_> {
let key = exec_mappings_key::new(
pid.try_into().unwrap(),
address_range.addr,
address_range.range,
32 + address_range.prefix_len,
);

self.add_bpf_mapping(&key, &mapping).unwrap();
Expand Down
16 changes: 8 additions & 8 deletions src/util.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#[derive(Debug, PartialEq)]
pub struct AddressBlockRange {
pub addr: u64,
pub range: u32,
pub prefix_len: u32,
}

/// For a given address range, calculate all the prefix ranges to ensure searching
Expand All @@ -22,7 +22,7 @@ pub fn summarize_address_range(low: u64, high: u64) -> Vec<AddressBlockRange> {
);
res.push(AddressBlockRange {
addr: curr,
range: 64 - number_of_bits,
prefix_len: 64 - number_of_bits,
});
curr += 1 << number_of_bits;
if curr - 1 == u64::MAX {
Expand Down Expand Up @@ -51,18 +51,18 @@ mod tests {
assert_eq!(
summarize_address_range(0, 100),
vec![
AddressBlockRange { addr: 0, range: 58 },
AddressBlockRange { addr: 0, prefix_len: 58 },
AddressBlockRange {
addr: 64,
range: 59
prefix_len: 59
},
AddressBlockRange {
addr: 96,
range: 62
prefix_len: 62
},
AddressBlockRange {
addr: 100,
range: 64
prefix_len: 64
}
]
);
Expand Down Expand Up @@ -100,7 +100,7 @@ mod tests {
assert!(mapping1.end < mapping2.begin);

for address_range in summarize_address_range(mapping1.begin, mapping1.end - 1) {
let key = exec_mappings_key::new(510530, address_range.addr, address_range.range);
let key = exec_mappings_key::new(510530, address_range.addr, 32 + address_range.prefix_len);
map.update(
unsafe { plain::as_bytes(&key) },
unsafe { plain::as_bytes(&mapping1) },
Expand All @@ -110,7 +110,7 @@ mod tests {
}

for address_range in summarize_address_range(mapping2.begin, mapping2.end - 1) {
let key = exec_mappings_key::new(510530, address_range.addr, address_range.range);
let key = exec_mappings_key::new(510530, address_range.addr, 32 + address_range.prefix_len);
map.update(
unsafe { plain::as_bytes(&key) },
unsafe { plain::as_bytes(&mapping2) },
Expand Down

0 comments on commit 64663e5

Please sign in to comment.