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

Set ptracer permissions for IPC handle creation #1018

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PatKamin
Copy link
Contributor

@PatKamin PatKamin commented Jan 7, 2025

Fixes: #979

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used

@PatKamin PatKamin requested review from vinser52 and ldorau January 7, 2025 07:54
@PatKamin PatKamin requested a review from a team as a code owner January 7, 2025 07:54
@@ -12,21 +12,6 @@ set -e
# port should be a number from the range <1024, 65535>
PORT=$(( 1024 + ( $$ % ( 65535 - 1024 ))))

Copy link
Contributor

Choose a reason for hiding this comment

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

it's this time of the year - pls, bump license(s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done

@lplewa
Copy link
Contributor

lplewa commented Jan 7, 2025

  1. Should the library call it? imho this should be something done by the user, not by the library. So we should only update examples, and documentation, so user knows how to use this API. Imho this is security variability, if we do it always, without user consent.

  2. Do it even work? We set permission to parent process. Our tests will work as producer and customer shares the same parent process. What if producer, and consumer are different parents? I.E are start from different bash processes.

@PatKamin
Copy link
Contributor Author

PatKamin commented Jan 8, 2025

  1. Should the library call it? imho this should be something done by the user, not by the library. So we should only update examples, and documentation, so user knows how to use this API. Imho this is security variability, if we do it always, without user consent.

It's for the ease-of-use of the UMF IPC API. Perhaps at least we should enable tracing with a new CMake option so the user enables tracing explicitly? And this would be disabled by default as UMF users not using IPC API would not be interested in setting ptrace scope wider.

  1. Do it even work? We set permission to parent process. Our tests will work as producer and customer shares the same parent process. What if producer, and consumer are different parents? I.E are start from different bash processes.

It works for MPI ranks communication. That's the case covered in our tests where producer and consumer share the same parent process. As for the different parents case, it's not tested but I think it could work if they are using the same shared umf library.

@lplewa
Copy link
Contributor

lplewa commented Jan 8, 2025

As for the different parents case, it's not tested but I think it could work if they are using the same shared umf library.

This is about kernel level process security protection/permissions - common shared library between processes do not have to influence here.

Copy link
Contributor

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

26 tests out of 56 fail on WSL with the following error:

umf_example_ipc_ipcapi_producer: unified-memory-framework/src/ipc_cache.c:136: umfIpcHandleMappedCacheCreate: Assertion `IPC_MAPPED_CACHE_GLOBAL != NULL' failed.
Aborted (core dumped)

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.

Use prctl when ptrace is restricted
5 participants