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

usdt: use ProcMountNSGuard for pid and path attach #2064

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

tjfontaine
Copy link
Contributor

This adds a couple of lexical scopes for usage with ProcMountNSGuard so the hint'ing works for the C interface when using bcc_usdt_new_frompid(). Also, add a guard for the Context::Context(pid, path) constructor for resolving the probes.

From the commit message:

Since the probe may be in a descendent namespace, ensure that when stat()ing for HINTs and resolving linked elf locations, be sure to always perform them relative to any observed mount namespace.

However, usage of the mount namespace must be surgical, since we're observing the pid with its outter identity, make sure we're not in the mount namespace when attempting to readlink the /proc/<pid>/exe

Since the probe may be in a descendent namespace, ensure that when `stat()`ing
for HINTs and resolving linked elf locations, be sure to always perform them
relative to any observed mount namespace.

However, usage of the mount namespace must be surgical, since we're observing
the pid with its outter identity, make sure we're not in the mount namespace
when attempting to `readlink` the `/proc/<pid>/exe`
@yonghong-song
Copy link
Collaborator

[buildbot, test this please]

@yonghong-song
Copy link
Collaborator

@palmtenor could you help check the patch as well?

currently, we have some mount related tests at bcc/tests/cc/test_c_api.cc, could you check whether it is possible to add a test there or a different test for this patch? This way, with accumulated tests, we will be able to avoid regressions over time. Thanks!

Copy link
Member

@palmtenor palmtenor left a comment

Choose a reason for hiding this comment

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

Overall looks good. Thanks!

ProcMountNSGuard g(new ProcMountNS(pid));
struct stat buffer;
if (strlen(path) >= 1 && path[0] != '/') {
fprintf(stderr, "HINT: Binary path should be absolute.\n\n");
Copy link
Member

Choose a reason for hiding this comment

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

Well, I personally have used say trace.py u:./somebinary:tracePoint --pid PID. If the target Process is in global namespace this would actually work. Maybe just add a hint if the actual context construction failed and say this might be the reason, instead of just directly fail?

@palmtenor
Copy link
Member

However, usage of the mount namespace must be surgical, since we're observing the pid with its outter identity, make sure we're not in the mount namespace when attempting to readlink the /proc//exe

Good observation.
Not directly for this PR: In fact if you directly trace on /proc/PID/exe binary from outside the container it might just fail for the same reason. I had some plan to add some special check for procfs paths.

@dalehamel
Copy link
Member

What is still outstanding to merge this? Ran into the same issues as @tjfontaine when looking at usdt support in bpftrace, and discovered that bcc itself doesn't properly handle mount namespaces for its own path check, which this seems to fix.

@yonghong-song
Copy link
Collaborator

Let us merge this then.

@yonghong-song yonghong-song merged commit 9dcde27 into iovisor:master Feb 12, 2019
palexster pushed a commit to palexster/bcc that referenced this pull request Jul 7, 2019
Since the probe may be in a descendent namespace, ensure that when `stat()`ing
for HINTs and resolving linked elf locations, be sure to always perform them
relative to any observed mount namespace.

However, usage of the mount namespace must be surgical, since we're observing
the pid with its outter identity, make sure we're not in the mount namespace
when attempting to `readlink` the `/proc/<pid>/exe`
CrackerCat pushed a commit to CrackerCat/bcc that referenced this pull request Jul 31, 2024
Since the probe may be in a descendent namespace, ensure that when `stat()`ing
for HINTs and resolving linked elf locations, be sure to always perform them
relative to any observed mount namespace.

However, usage of the mount namespace must be surgical, since we're observing
the pid with its outter identity, make sure we're not in the mount namespace
when attempting to `readlink` the `/proc/<pid>/exe`
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