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.

Sign up for GitHub

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

Jump to bottom

Don't unpack kernel headers or look in tmp #3156

Merged
merged 1 commit into from
May 15, 2024
Merged

Don't unpack kernel headers or look in tmp #3156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/clang_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,9 +660,6 @@ bool ClangParser::parse(ast::Program *program,
StderrSilencer silencer;
silencer.silence();
#endif
if (program->c_definitions.empty() && bpftrace.btf_set_.empty())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this into main

return true;

input = "#include </bpftrace/include/__btf_generated_header.h>\n" +
program->c_definitions;

Expand Down
2 changes: 1 addition & 1 deletion src/fuzz_main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ int fuzz_main(const char* data, size_t sz)
struct utsname utsname;
uname(&utsname);
std::string ksrc, kobj;
auto kdirs = get_kernel_dirs(utsname, !bpftrace.feature_->has_btf());
auto kdirs = get_kernel_dirs(utsname);
ksrc = std::get<0>(kdirs);
kobj = std::get<1>(kdirs);

Expand Down
62 changes: 34 additions & 28 deletions src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,41 +387,47 @@ static void parse_env(BPFtrace& bpftrace)
if (TracepointFormatParser::parse(driver.root.get(), bpftrace) == false)
return nullptr;

ClangParser clang;
std::vector<std::string> extra_flags;
{
struct utsname utsname;
uname(&utsname);
std::string ksrc, kobj;
auto kdirs = get_kernel_dirs(utsname);
ksrc = std::get<0>(kdirs);
kobj = std::get<1>(kdirs);

if (ksrc != "")
extra_flags = get_kernel_cflags(
utsname.machine, ksrc, kobj, bpftrace.kconfig);
}
extra_flags.push_back("-include");
extra_flags.push_back("/bpftrace/include/" CLANG_WORKAROUNDS_H);

for (auto dir : include_dirs) {
extra_flags.push_back("-I");
extra_flags.push_back(dir);
}
for (auto file : include_files) {
extra_flags.push_back("-include");
extra_flags.push_back(file);
}

// NOTE(mmarchini): if there are no C definitions, clang parser won't run to
// avoid issues in some versions. Since we're including files in the command
// line, we want to force parsing, so we make sure C definitions are not
// empty before going to clang parser stage.
if (!include_files.empty() && driver.root->c_definitions.empty())
driver.root->c_definitions = "#define __BPFTRACE_DUMMY__";

if (!clang.parse(driver.root.get(), bpftrace, extra_flags))
return nullptr;
bool should_clang_parse = !(driver.root.get()->c_definitions.empty() &&
bpftrace.btf_set_.empty());

if (should_clang_parse) {
ClangParser clang;
std::vector<std::string> extra_flags;
{
struct utsname utsname;
uname(&utsname);
std::string ksrc, kobj;
auto kdirs = get_kernel_dirs(utsname);
ksrc = std::get<0>(kdirs);
kobj = std::get<1>(kdirs);

if (ksrc != "") {
extra_flags = get_kernel_cflags(
utsname.machine, ksrc, kobj, bpftrace.kconfig);
}
}
extra_flags.push_back("-include");
extra_flags.push_back(CLANG_WORKAROUNDS_H);

for (auto dir : include_dirs) {
extra_flags.push_back("-I");
extra_flags.push_back(dir);
}
for (auto file : include_files) {
extra_flags.push_back("-include");
extra_flags.push_back(file);
}

if (!clang.parse(driver.root.get(), bpftrace, extra_flags))
return nullptr;
}

err = driver.parse();
if (err)
Expand Down
115 changes: 9 additions & 106 deletions src/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ const struct vmlinux_location vmlinux_locs[] = {
{ nullptr, false },
};

constexpr std::string_view PROC_KHEADERS_PATH = "/sys/kernel/kheaders.tar.xz";

static bool pid_in_different_mountns(int pid);
static std::vector<std::string> resolve_binary_path(const std::string &cmd,
const char *env_paths,
Expand Down Expand Up @@ -683,103 +681,6 @@ bool is_dir(const std::string &path)
return std_filesystem::is_directory(buf, ec);
}

bool file_exists_and_ownedby_root(const char *f)
{
struct stat st;
if (stat(f, &st) == 0) {
if (st.st_uid != 0) {
LOG(ERROR) << "header file ownership expected to be root: "
<< std::string(f);
return false;
}
return true;
}
return false;
}

namespace {
struct KernelHeaderTmpDir {
KernelHeaderTmpDir(const std::string &prefix) : path{ prefix + "XXXXXX" }
{
if (::mkdtemp(&path[0]) == nullptr) {
LOG(FATAL) << "creating temporary path for kheaders.tar.xz failed";
}
}

~KernelHeaderTmpDir()
{
if (path.size() > 0) {
// move_to either did not succeed or did not run, so clean up after
// ourselves
exec_system(("rm -rf " + path).c_str());
}
}

void move_to(const std::string &new_path)
{
int err = ::rename(path.c_str(), new_path.c_str());
if (err == 0) {
path = "";
}
}

std::string path;
};

std::string unpack_kheaders_tar_xz(const struct utsname &utsname)
{
std::error_code ec;
#if defined(__ANDROID__)
std_filesystem::path path_prefix{ "/data/local/tmp" };
#else
std_filesystem::path path_prefix{ "/tmp" };
#endif
std_filesystem::path path_kheaders{ PROC_KHEADERS_PATH };
if (const char *tmpdir = ::getenv("TMPDIR")) {
path_prefix = tmpdir;
}
path_prefix /= "kheaders-";
std_filesystem::path shared_path{ path_prefix.string() + utsname.release };

if (file_exists_and_ownedby_root(shared_path.c_str())) {
// already unpacked
return shared_path.string();
}

if (!std_filesystem::exists(path_kheaders, ec)) {
StderrSilencer silencer;
silencer.silence();

FILE *modprobe = ::popen("modprobe kheaders", "w");
if (modprobe == nullptr || pclose(modprobe) != 0) {
return "";
}

if (!std_filesystem::exists(path_kheaders, ec)) {
return "";
}
}

KernelHeaderTmpDir tmpdir{ path_prefix };

FILE *tar = ::popen(("tar xf " + std::string(PROC_KHEADERS_PATH) + " -C " +
tmpdir.path)
.c_str(),
"w");
if (!tar) {
return "";
}

int rc = ::pclose(tar);
if (rc == 0) {
tmpdir.move_to(shared_path);
return shared_path;
}

return "";
}
} // namespace

// get_kernel_dirs returns {ksrc, kobj} - directories for pristine and
// generated kernel sources.
//
Expand All @@ -798,8 +699,7 @@ std::string unpack_kheaders_tar_xz(const struct utsname &utsname)
// Both ksrc and kobj are guaranteed to be != "", if at least some trace of
// kernel sources was found.
std::tuple<std::string, std::string> get_kernel_dirs(
const struct utsname &utsname,
bool unpack_kheaders)
const struct utsname &utsname)
{
#ifdef KERNEL_HEADERS_DIR
return { KERNEL_HEADERS_DIR, KERNEL_HEADERS_DIR };
Expand Down Expand Up @@ -827,11 +727,14 @@ std::tuple<std::string, std::string> get_kernel_dirs(
kobj = "";
}
if (ksrc.empty() && kobj.empty()) {
if (unpack_kheaders) {
const auto kheaders_tar_xz_path = unpack_kheaders_tar_xz(utsname);
if (kheaders_tar_xz_path.size() > 0)
return std::make_tuple(kheaders_tar_xz_path, kheaders_tar_xz_path);
}
LOG(WARNING) << "Could not find kernel headers in " << ksrc << " or "
<< kobj
<< ". To specify a particular path to kernel headers, set the "
"env variables BPFTRACE_KERNEL_SOURCE and, optionally, "
"BPFTRACE_KERNEL_BUILD if the kernel was built in a "
"different directory than its source. To create kernel "
"headers run 'modprobe kheaders', which will create a tar "
"file at /sys/kernel/kheaders.tar.xz";
Comment on lines +730 to +737
Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine that in most cases bpftrace doesn't need kheaders (can use BTF data instead); would it make sense to only print this warning if kernel headers end up being required?

A couple of cases come to mind:

  • Bpftrace script unconditionally includes one or more kernel headers (e.g. #include <linux/sched.h> without gating on BPFTRACE_HAVE_BTF)
  • Conflicts between user-defined and BTF types

Related: commit 91db77c5

cc @danobi

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good point. I agree that we should not print the warning by default as there's a number of situations where kheaders are not necessary.

Copy link
Contributor Author

@jordalgo jordalgo May 13, 2024 •
edited

Choose a reason for hiding this comment

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

How about as a stop-gap (in the interest of getting this patched), I just reinstate the !bpftrace.feature_->has_btf() check as a conditional to show the warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

I may remember it wrong (it's been a while since I wrote that code) but IMHO kheaders are needed iff ClangParser runs so we should use the same condition as in clang_parser.cpp:663:

  if (program->c_definitions.empty() && bpftrace.btf_set_.empty())

It'd be nice if we could reuse it.

An alternative would be to issue the warning from ClangParser when it fails but that may be even more complicated.

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 moved a few things around but I'm using this same check before even calling get_kernel_dirs so the warning should only be shown in that case.

return std::make_tuple("", "");
}
if (ksrc.empty()) {
Expand Down
3 changes: 1 addition & 2 deletions src/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,7 @@ std::vector<int> get_possible_cpus();
bool is_dir(const std::string &path);
bool file_exists_and_ownedby_root(const char *f);
std::tuple<std::string, std::string> get_kernel_dirs(
const struct utsname &utsname,
bool unpack_kheaders = true);
const struct utsname &utsname);
std::vector<std::string> get_kernel_cflags(const char *uname_machine,
const std::string &ksrc,
const std::string &kobj,
Expand Down
21 changes: 0 additions & 21 deletions tests/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,27 +358,6 @@ TEST(utils, get_pids_for_program)
ASSERT_EQ(pids.size(), 0);
}

TEST(utils, file_exists_and_ownedby_root)
{
std::string tmpdir = "/tmp/bpftrace-test-utils-XXXXXX";
std::string file1 = "/ownedby-user";
std::string file2 = "/no-exists";
if (::mkdtemp(tmpdir.data()) == nullptr) {
throw std::runtime_error("creating temporary path for tests failed");
}

int fd;
fd = open((tmpdir + file1).c_str(), O_CREAT, S_IRUSR);
close(fd);
ASSERT_GE(fd, 0);

EXPECT_FALSE(file_exists_and_ownedby_root((tmpdir + file1).c_str()));
EXPECT_FALSE(file_exists_and_ownedby_root((tmpdir + file2).c_str()));
EXPECT_TRUE(file_exists_and_ownedby_root("/proc/1/maps"));

EXPECT_GT(std_filesystem::remove_all(tmpdir), 0);
}

} // namespace utils
} // namespace test
} // namespace bpftrace