kallsyms: use ebpf alternative when kallsyms not available #2280

alban · 2023-12-10T17:50:45Z

kallsyms: use ebpf alternative when kallsyms not available

On the minikube kernel 5.10.57, /proc/kallsyms does not provide the following symbols:

bpf_prog_fops (used by the top-ebpf gadget)
socket_file_ops (used by the socket enricher)

This patch provides an alternative to get the address of those symbols using ebpf.

Most of the required ebpf code was already implemented for the container-hook package. The file is moved to make it available in a generic package:
pkg/container-hook/bpf/privatedata.bpf.c -> pkg/kfilefields/bpf/filefields.bpf.c

How to use

No changes

Testing done

minikube start -p minikube-kvm2 --driver=kvm2
kubectl run -ti --rm --image=busybox --restart=Never shell01 -- sh
nc -l -p 8080 &

minikube cp ig minikube-kvm2:/usr/bin/ig
minikube ssh
sudo chmod +x /usr/bin/ig
sudo ig top ebpf
sudo ig trace dns
sudo ig trace network

Tested by adding a fmt.Printf:

$ sudo ig trace dns
...
symbol socket_file_ops found at ffffffff94ea0660 using ebpf
...
$ sudo ig top ebpf
...
symbol bpf_prog_fops found at ffffffff94d99ea0 using ebpf
...

Tested by reverting 681c370 (#2270). It works too.

GKE

integration tests run on GKE are passing now since symbol are being resolved:

Links

Previous PR implementing most of the ebpf code for this: container-hook: replace iterator by alternative mechanism #1789
Issue with top-ebpf top/ebpf does not work on minikube with kvm driver #1213
Issue with the socket enricher in trace-dns Regression: #2268

TODO

More automatic tests? Which ones?
Should socketenricher: Print warning rather than error when problem with kal… #2270 be reverted in this PR?

eiffel-fl · 2023-12-11T12:09:39Z

pkg/kallsyms/kallsyms.go

@@ -179,6 +179,12 @@ func specUpdateAddresses(
 		}
 	}

+	// List of symbols that we're able to find without using /proc/kallsyms


I am wondering if we cannot use this approach for all symbols and get rid of /proc/kallsyms?

kallsyms is also used in:

profile-cpu gadget (kAllSyms.LookupByInstructionPointer(ip))

top-block-io gadget (__blk_account_io_start, blk_account_io_start, __blk_account_io_done, blk_account_io_done

In those cases, the symbol is not used in any field of a struct file *, so the same trick to pass a file descriptor in a unix socket (SCM_RIGHTS) cannot be used.

eiffel-fl · 2023-12-11T12:22:24Z

Should #2270 be reverted in this PR?

Why not, but if everything failed (reading /proc/kallsyms and using eBPF), we should still print a warning.

alban · 2023-12-12T18:01:43Z

I've refactorised the code to make it possible to add some unit tests. It is now possible to test that the two methods to get the addresses of bpf_prog_fops and socket_file_ops return the same values.

Should #2270 be reverted in this PR?

Why not, but if everything failed (reading /proc/kallsyms and using eBPF), we should still print a warning.

After more thinking, I think both could be done in a future PR.

eiffel-fl

Some more comments but it is looking great:

pkg/kallsyms/kallsyms.go

eiffel-fl · 2023-12-14T14:19:48Z

I've refactorised the code to make it possible to add some unit tests. It is now possible to test that the two methods to get the addresses of bpf_prog_fops and socket_file_ops return the same values.

Should #2270 be reverted in this PR?

Why not, but if everything failed (reading /proc/kallsyms and using eBPF), we should still print a warning.

After more thinking, I think both could be done in a future PR.

What do you mean by both?

eiffel-fl

I tested it and it works fine but I am wondering about #2270.

alban · 2024-01-17T11:25:41Z

I've refactorised the code to make it possible to add some unit tests. It is now possible to test that the two methods to get the addresses of bpf_prog_fops and socket_file_ops return the same values.

Should #2270 be reverted in this PR?

Why not, but if everything failed (reading /proc/kallsyms and using eBPF), we should still print a warning.

After more thinking, I think both could be done in a future PR.

What do you mean by both?

Sorry I was confused. At the moment, we print a warning when both methods (kallsyms and eBPF) fail, so I think it is fine as it is.

mauriciovasquezbernal

Some initial comments. Will look at tests later on.

pkg/kallsyms/bpf/privatedata.bpf.c

pkg/kallsyms/kallsyms.go

mauriciovasquezbernal · 2024-01-18T12:46:43Z

pkg/kallsyms/kallsyms.go

+	// List of symbols that we're able to find in 'struct file *' using eBPF
+	symbolsBypass := map[string]fdType{
+		"socket_file_ops": fdTypeSocket,
+		"bpf_prog_fops":   fdTypeEbpfProgram,
+	}


general golang question: is it better to use a map than a switch case?

pkg/kallsyms/kallsyms.go

mauriciovasquezbernal · 2024-01-18T12:54:10Z

pkg/kallsyms/kallsyms.go

Some comments about previous implementation:

Would merging symbolsMap and triedGetAddr maps in a single one containing a type symbolResolve struct { addr uint64; err error } as value make the code better?

Does SymbolExists have to acquire the lock? Why it doesn't trigger the lookup logic and only returns from cache?

Perhaps specUpdateAddresses can be refactored in a function that just returns the symbols and it'll be easier to test without having to always use the bpf spec?

I didn't touch these to avoid changing to much but happy to revisit if you think they should be part of this PR.

pkg/kallsyms/privatedata.go

mauriciovasquezbernal · 2024-01-18T14:10:02Z

pkg/kallsyms/kallsyms.go

Perhaps specUpdateAddresses can be refactored in a function that just returns the symbols and it'll be easier to test without having to always use the bpf spec?

mqasimsarfraz · 2024-02-12T22:05:54Z

It seems this is affecting integrations runs on GKE (OS: Container-Optimized OS from Google, Kernel: 5.15.133+) since I see following warnings in the log:

time="2024-02-09T13:38:05Z" level=warning msg="updating socket_file_ops address with ksyms: file does not exist\nEither you cannot access /proc/kallsyms or this file does not contain socket_file_ops"
time="2024-02-09T13:38:17Z" level=warning msg="updating socket_file_ops address with ksyms: file does not exist\nEither you cannot access /proc/kallsyms or this file does not contain socket_file_ops"

I tested it with the image from this PR and results are looking better.

eiffel-fl

Hi!

What is the status of this PR?
I did take a quick look but I can see there are still some previous unresolved comments.

Best regards.

pkg/kallsyms/kallsyms.go

mqasimsarfraz · 2024-02-26T09:48:03Z

What is the status of this PR? I did take a quick look but I can see there are still some previous unresolved comments.

Thanks for the quick look, I am still working on unresolved comments. I will mark this PR draft and open it for reviews once I am done!

On the minikube kernel 5.10.57, /proc/kallsyms does not provide the following symbols: - bpf_prog_fops (used by the top-ebpf gadget) - socket_file_ops (used by the socket enricher) This patch provides an alternative to get the address of those symbols using ebpf. Most of the required ebpf code was already implemented for the container-hook package. The file is moved to make it available in a generic package: pkg/container-hook/bpf/privatedata.bpf.c -> pkg/kfilefields/bpf/filefields.bpf.c Co-authored-by: Qasim Sarfraz <qasimsarfraz@microsoft.com> Co-authored-by: Alban Crequy <albancrequy@linux.microsoft.com> Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

mqasimsarfraz · 2024-03-28T14:45:17Z

Thanks for the reviews!

eiffel-fl reviewed Dec 11, 2023

View reviewed changes

alban force-pushed the alban_privatedata branch from 78e4382 to 86ca019 Compare December 12, 2023 17:58

alban force-pushed the alban_privatedata branch from 86ca019 to 76d4125 Compare December 12, 2023 18:02

eiffel-fl reviewed Dec 13, 2023

View reviewed changes

pkg/kallsyms/kallsyms.go Outdated Show resolved Hide resolved

pkg/kallsyms/kallsyms.go Outdated Show resolved Hide resolved

alban force-pushed the alban_privatedata branch from 76d4125 to 20f7d21 Compare December 13, 2023 15:44

alban requested review from eiffel-fl and mauriciovasquezbernal December 14, 2023 13:44

eiffel-fl reviewed Dec 14, 2023

View reviewed changes

mauriciovasquezbernal reviewed Jan 18, 2024

View reviewed changes

alban self-assigned this Jan 18, 2024

mauriciovasquezbernal reviewed Jan 18, 2024

View reviewed changes

mqasimsarfraz mentioned this pull request Feb 14, 2024

ci: Run integration test on GKE #2502

Merged

mqasimsarfraz assigned mqasimsarfraz and unassigned alban Feb 23, 2024

mqasimsarfraz force-pushed the alban_privatedata branch 4 times, most recently from ad5b24d to eaa4397 Compare February 23, 2024 18:00

eiffel-fl reviewed Feb 26, 2024

View reviewed changes

pkg/kallsyms/kallsyms.go Outdated Show resolved Hide resolved

mqasimsarfraz marked this pull request as draft February 26, 2024 09:48

mqasimsarfraz force-pushed the alban_privatedata branch 4 times, most recently from d08a73d to 661af6b Compare February 28, 2024 17:21

alban and others added 2 commits March 27, 2024 19:30

integration: Enable test on GKE

9036995

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

mqasimsarfraz force-pushed the alban_privatedata branch from 72a892c to 9036995 Compare March 27, 2024 18:30

mqasimsarfraz temporarily deployed to aks March 27, 2024 18:37 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks March 27, 2024 20:38 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks March 27, 2024 20:52 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks March 28, 2024 09:00 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks March 28, 2024 09:01 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks March 28, 2024 10:41 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks March 28, 2024 11:03 — with GitHub Actions Inactive

mqasimsarfraz merged commit 150ec74 into main Mar 28, 2024
123 checks passed

mqasimsarfraz deleted the alban_privatedata branch March 28, 2024 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kallsyms: use ebpf alternative when kallsyms not available #2280

kallsyms: use ebpf alternative when kallsyms not available #2280

alban commented Dec 10, 2023 •

edited by mqasimsarfraz

eiffel-fl Dec 11, 2023

alban Dec 12, 2023

eiffel-fl commented Dec 11, 2023

alban commented Dec 12, 2023

eiffel-fl left a comment

eiffel-fl commented Dec 14, 2023

eiffel-fl left a comment

alban commented Jan 17, 2024

mauriciovasquezbernal left a comment

mauriciovasquezbernal Jan 18, 2024

mauriciovasquezbernal Jan 18, 2024

mauriciovasquezbernal Jan 18, 2024

mqasimsarfraz Feb 29, 2024

mauriciovasquezbernal Jan 18, 2024

mqasimsarfraz commented Feb 12, 2024

eiffel-fl left a comment

mqasimsarfraz commented Feb 26, 2024

mqasimsarfraz commented Mar 28, 2024

kallsyms: use ebpf alternative when kallsyms not available #2280

kallsyms: use ebpf alternative when kallsyms not available #2280

Conversation

alban commented Dec 10, 2023 • edited by mqasimsarfraz

kallsyms: use ebpf alternative when kallsyms not available

How to use

Testing done

GKE

Links

TODO

eiffel-fl Dec 11, 2023

Choose a reason for hiding this comment

alban Dec 12, 2023

Choose a reason for hiding this comment

eiffel-fl commented Dec 11, 2023

alban commented Dec 12, 2023

eiffel-fl left a comment

Choose a reason for hiding this comment

eiffel-fl commented Dec 14, 2023

eiffel-fl left a comment

Choose a reason for hiding this comment

alban commented Jan 17, 2024

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 18, 2024

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 18, 2024

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 18, 2024

Choose a reason for hiding this comment

mqasimsarfraz Feb 29, 2024

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 18, 2024

Choose a reason for hiding this comment

mqasimsarfraz commented Feb 12, 2024

eiffel-fl left a comment

Choose a reason for hiding this comment

mqasimsarfraz commented Feb 26, 2024

mqasimsarfraz commented Mar 28, 2024

alban commented Dec 10, 2023 •

edited by mqasimsarfraz