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

fix: nspid assign is not correct #3267

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Conversation

arthur-zhang
Copy link
Contributor

@arthur-zhang arthur-zhang commented Dec 31, 2024

Description

curr->nspid should be obtained from the child task arguments, not from the current task_struct (the parent process).

This will lead to a mismatch between pid and nspid.

__attribute__((section("kprobe/wake_up_new_task"), used)) int
BPF_KPROBE(event_wake_up_new_task, struct task_struct *task) {
struct execve_map_value *curr;

  tgid = BPF_CORE_READ(task, tgid); 
  curr->key.pid = tgid; // curr->key.pid is obtained from the task argument
  // is not correct.
  curr->nspid = get_task_pid_vnr(); // curr->nspid is obtained from the current task_struct (parent)!
}

for example. if we run in docker, we run a app which fork process every second, the pid in container will be like this

Hello from child process (PID: 485), parent: 1
Hello from child process (PID: 486), parent: 1
Hello from child process (PID: 487), parent: 1
Hello from child process (PID: 488), parent: 1
Hello from child process (PID: 489), parent: 1
Hello from child process (PID: 490), parent: 1

but when we get from bpf side, pid is the real host pid ,but nspid is always 1(the root parent process in container)

       fork_test-2040715 [002] d...1 3632982.277835: bpf_trace_printk: >>>>, pid: 2052883, nspid:1
       fork_test-2040333 [000] d...1 3632985.145937: bpf_trace_printk: >>>>, pid: 2052884, nspid:1
       fork_test-2040715 [002] d...1 3632985.278018: bpf_trace_printk: >>>>, pid: 2052885, nspid:1

Changelog

fix nspid value in bpf/process/bpf_fork.c

@arthur-zhang arthur-zhang requested a review from a team as a code owner December 31, 2024 08:50
@arthur-zhang arthur-zhang force-pushed the dev12 branch 2 times, most recently from 5c31503 to 0423e03 Compare December 31, 2024 08:56
Copy link
Contributor

@olsajiri olsajiri left a comment

Choose a reason for hiding this comment

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

nice catch, please put the PR description into commit changelog

FUNC_INLINE __u32 get_task_pid_vnr_curr(void)
{
struct task_struct *task = (struct task_struct *)get_current_task();
return get_task_pid_vnr_by_task(task);
Copy link
Contributor

Choose a reason for hiding this comment

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

please put extra line after declaration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

there needs to be empty line after declrations, so something like:

--- a/bpf/lib/bpf_task.h
+++ b/bpf/lib/bpf_task.h
@@ -98,6 +98,7 @@ FUNC_INLINE __u32 get_task_pid_vnr_by_task(struct task_struct *task)
 FUNC_INLINE __u32 get_task_pid_vnr_curr(void)
 {
        struct task_struct *task = (struct task_struct *)get_current_task();
+
        return get_task_pid_vnr_by_task(task);
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done modify

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, also please add the missing changelog, thanks

Copy link

netlify bot commented Jan 3, 2025

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit c25fc78
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/6777f594dd9f3b0008353f41
😎 Deploy Preview https://deploy-preview-3267--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@arthur-zhang arthur-zhang force-pushed the dev12 branch 2 times, most recently from 9be5633 to b65bc3c Compare January 6, 2025 02:38
@arthur-zhang arthur-zhang requested a review from olsajiri January 6, 2025 02:42
@olsajiri olsajiri added the release-note/minor This PR introduces a minor user-visible change label Jan 8, 2025
Copy link
Contributor

@olsajiri olsajiri left a comment

Choose a reason for hiding this comment

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

please add missing changelog and it's good to go, thanks

@arthur-zhang
Copy link
Contributor Author

please add missing changelog and it's good to go, thanks

Does current pr changelog meet your expectations?

@olsajiri
Copy link
Contributor

olsajiri commented Jan 9, 2025

please add missing changelog and it's good to go, thanks

Does current pr changelog meet your expectations?

pr changelog is fine, but there's nothing in commit changelog

The current code incorrectly obtains the nspid from the parent process's task_struct instead of the forked child process, leading to incorrect nspid values when running in containerized environments.

Signed-off-by: arthur-zhang <[email protected]>
@olsajiri olsajiri merged commit 8af0b97 into cilium:main Jan 9, 2025
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants