Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor - Separates the text section from read-only sections #553

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

Lichtso
Copy link

@Lichtso Lichtso commented Feb 27, 2024

This PR removes the indirection in which the text section is retrieved from the possibly concatenated read-only sections. There is only ever one text-section and no need to concatenate it. So instead the text section can be borrowed as a sub-slice of the relocated ELF bytes directly. Also removes the mem_size of text_section_info as that was double counted and the debug name, as it was unused.

@Lichtso Lichtso force-pushed the refactor/separate_text_from_readonly_sections branch from 7830546 to d659b9f Compare February 27, 2024 14:20
@Lichtso Lichtso force-pushed the refactor/separate_text_from_readonly_sections branch 2 times, most recently from 63188cf to 7fcd282 Compare February 27, 2024 14:25
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.00%. Comparing base (a996494) to head (5749444).
Report is 1 commits behind head on main.

Files Patch % Lines
src/elf.rs 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
+ Coverage   89.92%   90.00%   +0.08%     
==========================================
  Files          22       22              
  Lines        9735     9685      -50     
==========================================
- Hits         8754     8717      -37     
+ Misses        981      968      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Lichtso Lichtso force-pushed the refactor/separate_text_from_readonly_sections branch from 7fcd282 to 5cceb4f Compare February 27, 2024 14:28
@alessandrod
Copy link

There is only ever one text-section and no need to concatenate it

I haven't looked at the change yet but this is not true?

@Lichtso
Copy link
Author

Lichtso commented Feb 28, 2024

There is only ever one text-section and no need to concatenate it

We only consider the byte range of one section:

rbpf/src/elf.rs

Line 415 in 179a0f9

offset_range: text_section.file_range().unwrap_or_default(),

which is exactly named ".text":

rbpf/src/elf.rs

Line 396 in 179a0f9

let text_section = elf.section(b".text")?;

@Lichtso Lichtso force-pushed the refactor/separate_text_from_readonly_sections branch from 5cceb4f to 7c70307 Compare July 3, 2024 11:16
@Lichtso Lichtso requested review from LucasSte and removed request for alessandrod July 8, 2024 15:01
@Lichtso Lichtso force-pushed the refactor/separate_text_from_readonly_sections branch from 7c70307 to 5749444 Compare July 9, 2024 11:25
(
self.text_section_info.vaddr,
&ro_section[offset..offset.saturating_add(self.text_section_info.offset_range.len())],
ebpf::MM_PROGRAM_START.saturating_add(offset as u64),
Copy link

@LucasSte LucasSte Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing that you are adding to MM_PROGRAM_START because you subtracted it from the offset in the initialization to maintain parallelism between this function and the get_ro_region, right?

Would you mind explaining this in a comment?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rbpf/src/elf.rs

Line 237 in a400aa7

/// The first field is the offset of the section from MM_PROGRAM_START. The

The structs field is defined as being relative to MM_PROGRAM_START, both the text section and all read-only sections live in that MemoryRegion.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line 469, you're subtracting the values

 text_section_vaddr.saturating_sub(ebpf::MM_PROGRAM_START) as usize,

My concern was that it was a little confusing that you subtract and then you add in get_text_bytes. A comment to explain why this is the case would be helpful.

Comment on lines 490 to 494
Section::Owned(_, data) => data.capacity(),
Section::Borrowed(_, _) => 0,
})
Copy link

@LucasSte LucasSte Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include an arm like this to account for a possible owned data (in the text section)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text section is not supposed to ever be owned / copied. Hence why all match expressions against it have an unreachable! arm.

@Lichtso Lichtso force-pushed the refactor/separate_text_from_readonly_sections branch from 5749444 to 552a629 Compare July 9, 2024 17:28
@Lichtso Lichtso merged commit 48b1527 into main Jul 9, 2024
12 checks passed
@Lichtso Lichtso deleted the refactor/separate_text_from_readonly_sections branch July 9, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants