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

aya: Ensure that truncated map names are NULL terminated #1216

Merged
merged 1 commit into from
Mar 12, 2025

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented Mar 12, 2025

Limit of map names in eBPF is 16 bytes and they have to be NULL terminated.

Before this change, long names were truncated to 16 bytes. MAP_WITH_LOOOONG_NAAAAAAAAME would become MAP_WITH_LOOOONG, which doesn't contain the NULL byte.

This change fixes that by truncating the name to 15 bytes, ensuring that the 16th byte is NULL. MAP_WITH_LOOOONG_NAAAAAAAAME is truncated to MAP_WITH_LOOOON\0.


This change is Reviewable

@mergify mergify bot added aya This is about aya (userspace) test A PR that improves test cases or CI labels Mar 12, 2025
Copy link

netlify bot commented Mar 12, 2025

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b035d79
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/67d15f4a6a0bc400085fc8d0
😎 Deploy Preview https://deploy-preview-1216--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@vadorovsky
Copy link
Member Author

@dave-tucker @tamird this looks wild https://github.com/aya-rs/aya/actions/runs/13806711273/job/38618907343?pr=1216 given no API changes in my PR. I'll try to reproduce locally, but looks that something fishy is going on with public-api.

@vadorovsky
Copy link
Member Author

nvm, I missed #1215

// u.map_name is 16 bytes max and must be NULL terminated
let name_bytes = name.to_bytes_with_nul();
let len = cmp::min(name_bytes.len(), u.map_name.len());
let len = cmp::min(name_bytes.len(), u.map_name.len() - 1);
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm I annoyed at myself:
#1165 (comment)

I should have caught that using cmp::min would strip the nul byte 😭

Do you recall offhand if the rules are the same for program names as the logic there was changed in the same commit

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly the same rule applies to program names, but that was already fixed in 9eefb48:

let len = cmp::min(name_bytes.len(), u.prog_name.len() - 1); // Ensure NULL termination.

Copy link
Member

Choose a reason for hiding this comment

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

🤦

@tamird tamird requested a review from dave-tucker March 12, 2025 10:11
Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dave-tucker)


aya/src/sys/bpf.rs line 97 at r1 (raw file):

    if KernelVersion::at_least(4, 15, 0) {
        let name_bytes = name.to_bytes();
        // u.map_name is 16 bytes max and must be NULL terminated

can we drop this comment and follow the style used below?

let len = ...; // Ensure NULL termination.


aya/src/sys/bpf.rs line 153 at r1 (raw file):

    if let Some(name) = &aya_attr.name {
        let name_bytes = name.to_bytes();
        let len = cmp::min(name_bytes.len(), u.prog_name.len() - 1); // Ensure NULL termination.

@dave-tucker I think I fixed this after the original commit - the truncation was happening on the ingress path

9eefb48

// u.map_name is 16 bytes max and must be NULL terminated
let name_bytes = name.to_bytes_with_nul();
let len = cmp::min(name_bytes.len(), u.map_name.len());
let len = cmp::min(name_bytes.len(), u.map_name.len() - 1);
Copy link
Member

Choose a reason for hiding this comment

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

🤦

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dave-tucker and @vadorovsky)

Copy link
Member Author

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @dave-tucker and @tamird)


aya/src/sys/bpf.rs line 97 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can we drop this comment and follow the style used below?

let len = ...; // Ensure NULL termination.

Done.

Limit of map names in eBPF is 16 bytes and they have to be NULL
terminated.

Before this change, long names were truncated to 16 bytes.
`MAP_WITH_LOOOONG_NAAAAAAAAME` would become `MAP_WITH_LOOOONG`, which
doesn't contain the NULL byte.

This change fixes that by truncating the name to 15 bytes, ensuring
that the 16th byte is NULL. `MAP_WITH_LOOOONG_NAAAAAAAAME` is truncated
to `MAP_WITH_LOOOON\0`.
Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dave-tucker)

@vadorovsky vadorovsky changed the title aya: Ensure that truncated map names are NUL terminated aya: Ensure that truncated map names are NULL terminated Mar 12, 2025
@vadorovsky vadorovsky merged commit f48b5a4 into aya-rs:main Mar 12, 2025
30 of 31 checks passed
@vadorovsky vadorovsky deleted the map-names-trim branch March 12, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya This is about aya (userspace) test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants