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

Allow uprobes to work across filesystem namespace boundaries #2662

Closed
wants to merge 9 commits into from

Conversation

ivankelly
Copy link
Contributor

@ivankelly ivankelly commented Jul 4, 2023

When attaching to a uprobe, bpftrace will try to cache symbols for all pids using that program. It does this by canonicalizing the executable path and scanning /proc for processes whose /exe matches.

For containers, this will not work as the canonicalize command will resolve /proc/123/root/foo/bar to /foo/bar in the host filesystem and then error trying to canonicalize that because it doesn't exist on the host filesystem.

The proposed solution is, when getting the pids for a program, if the path is of the form /proc/<pid>/<root>/<path>, don't bother with the search and just extract the pid from the path itself.

Checklist
  • [n/a] Language changes are updated in man/adoc/bpftrace.adoc and if needed in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

When attaching to a uprobe, bpftrace will try to cache symbols for
all pids using that program. It does this by canonicalizing the
executable path and scanning /proc for processes whose /exe matches.

For containers, this will not work as the canonicalize command will
resolve /proc/123/root/foo/bar to /foo/bar in the host filesystem and
then error trying to canonicalize that because it doesn't exist on the
host filesystem.

The proposed solution is, when getting the pids for a program, if the path is
of the form /proc/<pid>/<root>/<path>, don't bother with the search
and just extract the pid from the path itself.

Testing uses a simulation of docker root mount setup, using unshare and
pivot_root.
Copy link
Contributor

@viktormalik viktormalik left a comment

Choose a reason for hiding this comment

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

Thanks! Looks ok to me, just a couple of nits and suggestions, mainly for the tests.

@lenticularis39 mind having a quick look?

tests/CMakeLists.txt Outdated Show resolved Hide resolved
tests/runtime/uprobe Outdated Show resolved Hide resolved
tests/runtime/uprobe Outdated Show resolved Hide resolved
tests/runtime/uprobe Show resolved Hide resolved
tests/testprogs/mountns_pivot_wrapper.c Outdated Show resolved Hide resolved
tests/runtime/engine/runner.py Outdated Show resolved Hide resolved
tests/runtime/engine/runner.py Outdated Show resolved Hide resolved
src/utils.cpp Outdated
process.path() / "exe", ec);
if (!ec && program_abs == pid_program)
pids.emplace_back(std::stoi(filename));
pids.emplace_back(std::stoi(match.str(1)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is the right idea, I can trace e.g. /proc/123/root/bin/bash, and 123 does not have to be the ID of the bash process, it can be e.g. the root of the namespace or another process in the namespace. Since the cache is loaded by PID, it won't cause incorrect symbol resolution, but perhaps a smarter solution could be implemented (see comment below).

if (!std::all_of(filename.begin(), filename.end(), ::isdigit))
continue;
std::error_code ec;
std_filesystem::path pid_program = std_filesystem::read_symlink(
Copy link
Contributor

Choose a reason for hiding this comment

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

Idea: I am not sure what this does for programs which are in a different mount namespace. Maybe also trying std_filesystem::absolute next to std_filesystem::canonical could work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think absolute is correct either, as the absolute path would simply be the path passed in. Perhaps weakly_canonical would work though. I'll give it a go.

Copy link
Contributor Author

@ivankelly ivankelly Jul 11, 2023

Choose a reason for hiding this comment

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

nope

[  FAILED  ] uprobe.uprobes - attach to probe for executable in a pivot_root'd mount namespace
        Command: ../src//bpftrace -e 'uprobe:/proc/20424/root/uprobe_test_static:function1 { printf("func %s\n", func); exit(); }'
        Unclean exit code: 1
        Output: ERROR: failed to write elf: filesystem error: cannot make canonical path: No such file or directory [/proc/20424/root/uprobe_test_static]\n

Which is strange, because that path does exist, but maybe the problem is that its reading across a filesystem boundary or something.

A solution could be to just catch the exception and return no pids if it gets a file not found. This method is only used to cache symbols for pids at probe attach time. if it doesn't cache anything, that's no big whoop, they can be resolved at output time.

@ivankelly
Copy link
Contributor Author

@viktormalik @lenticularis39 Addressed comments. Now get_pids_for_program just bails if it cannot get the canonical path.

tests/testprogs/mountns_pivot_wrapper.c Dismissed Show dismissed Hide dismissed
Copy link
Contributor

@viktormalik viktormalik left a comment

Choose a reason for hiding this comment

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

Sorry for a late review, looks good to me now, thanks!

Could you please fix the formatting issues reported by clang-format and resolve the merge conflict in CHANGELOG? Thank you!

@ajor
Copy link
Member

ajor commented Jan 16, 2024

Closing in favour of #2802

@ajor ajor closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants