-
Notifications
You must be signed in to change notification settings - Fork 185
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
trace exec: add upper_layer field #2353
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
I took a quick look and I have some comments, I will test it later.
Should I use a bitfield?
For now, it is OK to use a uint8
but if we plan to add plenty of boolean at bitfield would definitely be welcomed.
How should the output look like for the user?
What about:
COMM RET ARGS UPPERLAYER
sh 0 /usr/bin/sh -c cat /bin/echo > /bin/echo2 ; /bin/echo… false
or
COMM RET ARGS UPPERLAYER
sh 0 /usr/bin/sh -c cat /bin/echo > /bin/echo2 ; /bin/echo… ❌
Best regards.
@@ -163,6 +163,7 @@ func (t *Tracer) run() { | |||
Gid: bpfEvent.Gid, | |||
LoginUid: bpfEvent.Loginuid, | |||
SessionId: bpfEvent.Sessionid, | |||
UpperLayer: uint32(bpfEvent.UpperLayer), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why uint32
as it is an uint8
in eBPF code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to a bool so that it shows true or false in the output.
By the way, would it make sense to port everything to the image-based flavor? |
My plan is to document this limitation and work on it in a future PR. |
977521e
to
c5624d3
Compare
c5624d3
to
78e570a
Compare
I updated the PR:
|
78e570a
to
e17aea2
Compare
The CI fails because:
Both are unrelated to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and works fine. LGTM, thanks!
e17aea2
to
99e8ee3
Compare
Tested with: $ go run -exec 'sudo -E' ./cmd/ig/... trace exec RUNTIME.CONTAINERNAME PID PPID COMM RET ARGS UPPE… lucid_carver 48098 48076 sh 0 /usr/bin/sh -c cp /bin/echo /bin/echo2… false lucid_carver 48121 48098 cp 0 /usr/bin/cp /bin/echo /bin/echo2 false lucid_carver 48122 48098 echo 0 /bin/echo lower false lucid_carver 48123 48098 echo2 0 /bin/echo2 upper true $ docker run -ti --rm ubuntu sh -c 'cp /bin/echo /bin/echo2 ; /bin/echo lower ; /bin/echo2 upper' lower upper Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>
Tested with: $ sudo -E ./ig image build -t execsnoop ./gadgets/trace_exec/ $ sudo -E ./ig run execsnoop INFO[0000] Experimental features enabled RUNTIME.CONTAINERNAME PID PPID UID GID R… UPP… COMM test 97952 97928 0 0 0 fal… sh test 97976 97952 0 0 0 fal… cp test 97977 97952 0 0 0 fal… date test 97952 97928 0 0 0 true date $ docker run -ti --rm --name=test busybox sh -c 'cp /bin/date /date ; date ; /date' Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>
99e8ee3
to
8c433c4
Compare
@alban it seems it broke the CI on AKS and ARO: AKSupperlayer for captured: {
"runtime": {
"containerImageName": "docker.io/library/busybox:latest"
},
"k8s": {
"namespace": "test-trace-exec-7345717025894922932",
"podName": "test-pod",
"containerName": "test-pod"
},
"type": "normal",
"comm": "sh",
"args": [
"/bin/sh",
"-c",
"cp /bin/date /date ; setuidgid 1000:1111 sh -c 'while true; do /date ; /bin/sleep 0.1; done'"
],
"uid": 0,
"gid": 0,
"upperlayer": true,
"loginuid": 0,
"sessionid": 0,
"cwd": "/"
} expected: {
"runtime": {
"containerImageName": "docker.io/library/busybox:latest"
},
"k8s": {
"namespace": "test-trace-exec-7345717025894922932",
"podName": "test-pod",
"containerName": "test-pod"
},
"type": "normal",
"comm": "sh",
"args": [
"/bin/sh",
"-c",
"cp /bin/date /date ; setuidgid 1000:1111 sh -c 'while true; do /date ; /bin/sleep 0.1; done'"
],
"uid": 0,
"gid": 0,
"upperlayer": false,
"loginuid": 0,
"sessionid": 0,
"cwd": "/"
} https://github.com/inspektor-gadget/inspektor-gadget/actions/runs/7572632543/attempts/1#summary-20624245156 AROEvent for captured: {
"runtime": {
"containerImageName": "docker.io/library/busybox:latest"
},
"k8s": {
"namespace": "test-trace-exec-5865986315796987308",
"podName": "test-pod",
"containerName": "test-pod"
},
"type": "normal",
"comm": "date",
"args": [
"/date"
],
"uid": 1000,
"gid": 1111,
"upperlayer": false,
"loginuid": 0,
"sessionid": 0,
"cwd": "/"
} expected: {
"runtime": {
"containerImageName": "docker.io/library/busybox:latest"
},
"k8s": {
"namespace": "test-trace-exec-5865986315796987308",
"podName": "test-pod",
"containerName": "test-pod"
},
"type": "normal",
"comm": "date",
"args": [
"/date"
],
"uid": 1000,
"gid": 1111,
"upperlayer": true,
"loginuid": 0,
"sessionid": 0,
"cwd": "/"
} |
I tested manually on AKS and it worked for me:
Also tested with a azure node pool, and it worked fine too:
The kernel version in the CI logs is:
This is exactly the same version, so the difference is not explained by a different kernel. More logs show that
|
Kernel configuration:
On my laptop:
On the Azure node:
In our current code, we rely on fields relative positions:
But we cannot rely on fields relative positions in a struct when fields positions are randomized. It might or might not work, depending on how the random order was selected. |
According to this lkml email, |
trace exec: add upper_layer field
The upper_layer field says whether the program file is on the upper layer of overlayfs, i.e. it was modified in the container.
How to use
As you can see in the output, only
/bin/echo2
is reported as being on the overlay upper layer.Testing done
See above.
TODOs
Should I use a bitfield?Use boolsHow should the output look like for the user?Show booleans (true/false)Limitations
The reporting is incorrect if the exec failed (e.g. due to lack of +x permission). Example:
Misc
Fixes #2345
See also #1913
cc @galofir-ms