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::prctl: Adding set_vma_anon_name. #2378

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Apr 20, 2024

to set a name for an anonymous region for Linux/Android.

What does this PR do

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@devnexen devnexen force-pushed the prctl_set_addr_name branch 7 times, most recently from d79553f to c808911 Compare April 20, 2024 10:13
@devnexen devnexen marked this pull request as ready for review April 20, 2024 11:35
src/sys/prctl.rs Outdated

/// Set an identifier (or reset it) to the address memory range.
pub fn set_addr_name(addr: NonNull<c_void>, length: NonZeroUsize, name: &CStr) -> Result<()> {
let res = unsafe { libc::prctl(libc::PR_SET_VMA, libc::PR_SET_VMA_ANON_NAME, addr.as_ptr(), length, name.as_ptr()) };
Copy link
Member

Choose a reason for hiding this comment

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

From the manual, to reset the name, the arg5 has to be NULL:

If arg5 is NULL, the name of the appropriate anonymous virtual memory areas will be reset.

Maybe we should change the name argument to Option<&CStr>?

src/sys/prctl.rs Outdated
@@ -213,3 +215,10 @@ pub fn set_thp_disable(flag: bool) -> Result<()> {
pub fn get_thp_disable() -> Result<bool> {
prctl_get_bool(libc::PR_GET_THP_DISABLE)
}

/// Set an identifier (or reset it) to the address memory range.
pub fn set_addr_name(addr: NonNull<c_void>, length: NonZeroUsize, name: &CStr) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

I kinda think we should rename this function to set_vma_anon_name() so that it is consistent with the attribute name SET_VMA_ANON_NAME, though I do agree that set_addr_name is the clearer one.

sz,
CStr::from_bytes_with_nul(b"Nix\0").unwrap(),
)
.unwrap_or_default();
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using .unwrap_or_default() here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because of the comment above, the kernel might not support the feature.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry about missing that!

Google search shows that this option is a C macro defined in the kernel code, and if it is not defined, then this prctl call would return EINVAL:

> +#else /* CONFIG_ANON_VMA_NAME */
> +static int prctl_set_vma(unsigned long opt, unsigned long start,
> +                        unsigned long size, unsigned long arg)
> +{
> +       return -EINVAL;
> +}
> +#endif /* CONFIG_ANON_VMA_NAME */

Seems that there is no explicit way to check if this option is set or not?

@devnexen devnexen force-pushed the prctl_set_addr_name branch 3 times, most recently from 0ca2a2a to 565c830 Compare April 21, 2024 05:57
@devnexen devnexen changed the title sys::prctl: Adding set_addr_name. sys::prctl: Adding set_vma_anon_name. Apr 21, 2024
src/sys/prctl.rs Outdated
pub fn set_vma_anon_name(addr: NonNull<c_void>, length: NonZeroUsize, name: Option<&CStr>) -> Result<()> {
let nameref = match name {
Some(n) => n.as_ptr(),
_ => std::ptr::null_mut()
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we should use null() here

Suggested change
_ => std::ptr::null_mut()
_ => std::ptr::null()

sz,
CStr::from_bytes_with_nul(b"Nix\0").unwrap(),
)
.unwrap_or_default();
Copy link
Member

Choose a reason for hiding this comment

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

Sorry about missing that!

Google search shows that this option is a C macro defined in the kernel code, and if it is not defined, then this prctl call would return EINVAL:

> +#else /* CONFIG_ANON_VMA_NAME */
> +static int prctl_set_vma(unsigned long opt, unsigned long start,
> +                        unsigned long size, unsigned long arg)
> +{
> +       return -EINVAL;
> +}
> +#endif /* CONFIG_ANON_VMA_NAME */

Seems that there is no explicit way to check if this option is set or not?

to set a name for an `anonymous` region for Linux/Android.
@devnexen
Copy link
Contributor Author

Seems that there is no explicit way to check if this option is set or not?

No and in this case it does not really matter, this feature is more the "icing of top of cake" kind, so if it does not work it s just a no-op :)

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Thx!

@SteveLauC SteveLauC added this pull request to the merge queue Apr 21, 2024
Merged via the queue into nix-rust:master with commit 15dcd6b Apr 21, 2024
36 checks passed
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.

2 participants