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

sys: add map_ids to bpf_prog_get_info_by_fd #747

Merged
merged 2 commits into from
Aug 10, 2023
Merged

sys: add map_ids to bpf_prog_get_info_by_fd #747

merged 2 commits into from
Aug 10, 2023

Conversation

tamird
Copy link
Member

@tamird tamird commented Aug 10, 2023

Allows the caller to pass a slice which the kernel will populate with
map ids used by the program.

@tamird tamird requested a review from astoycos August 10, 2023 22:03
@netlify
Copy link

netlify bot commented Aug 10, 2023

Deploy Preview for aya-rs-docs ready!

Name Link
🔨 Latest commit 5ac1862
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64d56765b191e80008618306
😎 Deploy Preview https://deploy-preview-747--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.

@mergify mergify bot added the aya This is about aya (userspace) label Aug 10, 2023
@mergify
Copy link

mergify bot commented Aug 10, 2023

⚠️ The sha of the head commit of this PR conflicts with #637. Mergify cannot evaluate rules on this PR. ⚠️

Allows the caller to pass a slice which the kernel will populate with
map ids used by the program.
let mut attr = unsafe { mem::zeroed::<bpf_attr>() };
// info gets entirely populated by the kernel
let info = MaybeUninit::zeroed();
let mut info = unsafe { mem::zeroed() };
Copy link
Member

Choose a reason for hiding this comment

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

Can this stay as MaybeUninit::zeroed(); ?

Copy link
Member Author

Choose a reason for hiding this comment

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

How's that better? We'll need to immediately assume_init so we can write to it.

Copy link
Member

Choose a reason for hiding this comment

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

why not just call as_mut_ptr() on info in the init closure? Since we only need to assume_init in that one case

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd still need to assume_init later, before returning. What is the benefit of using MaybeUninit here?

Copy link
Member

Choose a reason for hiding this comment

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

Or just use as_mut_ptr() here and ONLY do unsafe code in that one case

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't make sense - look at the diff. Before the change, we had unsafe code on the return path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Writing through MaybeUninit::as_mut_ptr is nasty:

Gets a mutable pointer to the contained value. Reading from this pointer or turning it into a reference is undefined behavior unless the MaybeUninit is initialized.

https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#method.as_mut_ptr

That means we can't turn it into a mutable reference because that's UB. Again, how is this better?

Copy link
Member

@astoycos astoycos Aug 10, 2023

Choose a reason for hiding this comment

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

Fair enough I guess there is no way to protect against the unsafe code here 👍 In the docs it says

This has the same effect as [MaybeUninit::zeroed().assume_init()](https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#method.zeroed). It is useful for FFI sometimes, but should generally be avoided.

This is the "useful for FFI case" lol :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup.

Copy link
Member

@astoycos astoycos left a comment

Choose a reason for hiding this comment

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

/LGTM

@tamird tamird merged commit 5bc922a into main Aug 10, 2023
21 checks passed
@tamird tamird deleted the helpers branch August 10, 2023 22:57
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants