trace exec: add exe path #1959

alban · 2023-08-16T13:42:01Z

trace exec: add exe path

Thanks to this new field, we can distinguish the executable with its full path when it is not visible in the args, e.g. when executing via a symlink or via a procfs magic link such as /proc/self/exe.

How to use

Example of output:

$ sudo ig trace exec
RUNTIME.CONTAINERNAME PID    PPID   COMM  RET ARGS
test                  644871 639225 mkdir 0   /usr/bin/mkdir -p /tmp/bar/foo/
test                  644888 639225 cat   0   /usr/bin/cat /dev/null

$ sudo ig trace exec --paths
RUNTIME.CONTAINERN… PID    PPID   COMM  RET ARGS                     CWD EXEPATH
test                644377 639225 mkdir 0   /usr/bin/mkdir -p /tmp/… /   /usr/bin/mkdir
test                644497 639225 cat   0   /usr/bin/cat /dev/null   /   /usr/bin/cat

TBD

Similar to the cwd option (#1700), I didn't enable it by default. This increases the event in the ring buffer by 4k. It would be nice to optimise the event size.

Testing done

...

cc @omriShneor

Fixes #1950

mauriciovasquezbernal · 2024-01-11T16:35:20Z

Similar to the cwd option (#1700), I didn't enable it by default. This increases the event in the ring buffer by 4k. It would be nice to optimise the event size.

TBH I'm not sure we should be keeping these optional and big fields on the event. Perhaps we can update the logic to send multiple events on the perf ring buffer, one with the main information and one (or more) with additional fields. However image-based gadgets will be challenge. I think your approach is good for this PR, we need to think more about it for the image-based gadgets case.

alban · 2024-01-17T16:38:35Z

I updated this PR with a single flag --paths for enabling both the cwd and exe paths.

I'll think more about the image-based gadgets.

alban · 2024-01-18T16:59:29Z

Rebased again to fix new conflicts from recent changes on the main branch.

github-actions · 2024-03-19T01:45:51Z

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

mauriciovasquezbernal · 2024-03-19T13:22:42Z

I suppose we still need it. @alban would you mind rebasing one last time. I'll review it after.

alban · 2024-03-19T13:54:27Z

I rebased and fixed the conflicts. Let's see if the CI accepts it.

eiffel-fl · 2024-03-20T08:47:53Z

Similar to the cwd option (#1700), I didn't enable it by default. This increases the event in the ring buffer by 4k. It would be nice to optimise the event size.

TBH I'm not sure we should be keeping these optional and big fields on the event. Perhaps we can update the logic to send multiple events on the perf ring buffer, one with the main information and one (or more) with additional fields. However image-based gadgets will be challenge. I think your approach is good for this PR, we need to think more about it for the image-based gadgets case.

I also do not like increasing the size of event.
I will not go against it for the built-in gadgets as we will deprecate it soon.
For the image based one, I would rather see people needing this feature maintaining their own flavor of it.

eiffel-fl

Hi!

Please, see my comment with regard to the event size.
However, this is a good addition for people needing it.
I tested it and it works fine:

$ sudo ./ig trace exec --paths                              alban_exec_long_paths % u=
RUNTIME.CONTAINER… PID        PPID       COMM       PCOMM     RET ARGS                    CWD                     EXE                    
eloquent_shirley   47511      47487      bash       containe… 0   /usr/bin/bash           /                       /usr/bin/bash          
eloquent_shirley   47540      47511      ls         bash      0   /usr/bin/ls             /                       /usr/bin/ls

I have some comments nonetheless.

Best regards.

integration/inspektor-gadget/trace_exec_test.go

eiffel-fl · 2024-03-20T08:43:33Z

pkg/gadgets/trace/exec/tracer/tracer.go

@@ -213,10 +214,11 @@ func (t *Tracer) run() {
 		buf := []byte{}
 		args := record.RawSample[unsafe.Offsetof(execsnoopEvent{}.Args):]


I am wondering if we should else if and set this properly.

I am not sure what you suggest. I didn't change the logic of the if below.

eiffel-fl · 2024-03-20T08:45:51Z

pkg/gadgets/trace/exec/types/types.go

@@ -36,7 +36,8 @@ type Event struct {
 	UpperLayer bool     `json:"upperlayer" column:"upperlayer,width:10,fixed,hide"`
 	LoginUid   uint32   `json:"loginuid" column:"loginuid,template:uid,hide"`
 	SessionId  uint32   `json:"sessionid" column:"sessionid,minWidth:10,hide"`
-	Cwd        string   `json:"cwd,omitempty" column:"cwd,width:40" columnTags:"param:cwd"`
+	Cwd        string   `json:"cwd,omitempty" column:"cwd,width:40" columnTags:"param:paths"`
+	Exe        string   `json:"exe,omitempty" column:"exe,width:40" columnTags:"param:paths"`


I am not convinced about this name, what about BinPath?
At least, this should make clear this is a path to something.
With Exe, I am awaiting ELF or stuff like this.

Good idea. I suggest ExePath because it is similar to sysdig's proc.exepath.

Sysdig has both exe and exepath:

proc.exe The first command line argument argv[0] (truncated after 4096 bytes) which is usually the executable name but it could be also a custom string, it depends on what the user specifies. [...] proc.exepath The full executable path of the process [...]

mauriciovasquezbernal

Just one minor comment. LGTM once CI is green.

pkg/gadgets/trace/exec/tracer/tracer_test.go

Thanks to this new field, we can distinguish the executable with its full path when it is not visible in the args, e.g. when executing via a symlink or via a procfs magic link such as /proc/self/exe. Example of output: ``` $ sudo ig trace exec RUNTIME.CONTAINERNAME PID PPID COMM RET ARGS test 644871 639225 mkdir 0 /usr/bin/mkdir -p /tmp/bar/foo/ test 644888 639225 cat 0 /usr/bin/cat /dev/null $ sudo ig trace exec --paths RUNTIME.CONTAINERN… PID PPID COMM RET ARGS CWD EXEPATH test 644377 639225 mkdir 0 /usr/bin/mkdir -p /tmp/… / /usr/bin/mkdir test 644497 639225 cat 0 /usr/bin/cat /dev/null / /usr/bin/cat ``` Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>

alban force-pushed the alban_exec_long_paths branch 2 times, most recently from c37e192 to fca718b Compare August 21, 2023 12:33

alban mentioned this pull request Dec 6, 2023

[RFE] args[0] in the trace exec command displays relative path and not an absolute path. #1950

Closed

alban force-pushed the alban_exec_long_paths branch from fca718b to 60b7cce Compare January 17, 2024 16:32

alban marked this pull request as ready for review January 17, 2024 16:32

alban requested a review from mauriciovasquezbernal as a code owner January 17, 2024 16:32

alban force-pushed the alban_exec_long_paths branch 2 times, most recently from 0b87e3f to 43e2272 Compare January 17, 2024 16:37

alban self-assigned this Jan 18, 2024

alban force-pushed the alban_exec_long_paths branch from 43e2272 to 3dad5c1 Compare January 18, 2024 16:58

github-actions bot added the lifecycle/stale Marked to be closed in next 14 days because of inactivity. label Mar 19, 2024

alban force-pushed the alban_exec_long_paths branch from 3dad5c1 to f261406 Compare March 19, 2024 13:53

alban removed the lifecycle/stale Marked to be closed in next 14 days because of inactivity. label Mar 19, 2024

alban force-pushed the alban_exec_long_paths branch from f261406 to 982eaa5 Compare March 19, 2024 15:59

alban requested review from eiffel-fl, blanquicet and mqasimsarfraz as code owners March 19, 2024 15:59

eiffel-fl reviewed Mar 20, 2024

View reviewed changes

mauriciovasquezbernal approved these changes Mar 20, 2024

View reviewed changes

pkg/gadgets/trace/exec/tracer/tracer_test.go Outdated Show resolved Hide resolved

alban force-pushed the alban_exec_long_paths branch from 982eaa5 to 4f47190 Compare March 20, 2024 19:26

alban merged commit eb06221 into main Mar 20, 2024
59 checks passed

alban deleted the alban_exec_long_paths branch March 20, 2024 20:11

This was referenced May 8, 2024

trace exec --paths uses too much memory #2821

Closed

trace exec builtin gadget: fix memory usage #2840

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trace exec: add exe path #1959

trace exec: add exe path #1959

alban commented Aug 16, 2023 •

edited

mauriciovasquezbernal commented Jan 11, 2024

alban commented Jan 17, 2024

alban commented Jan 18, 2024

github-actions bot commented Mar 19, 2024

mauriciovasquezbernal commented Mar 19, 2024

alban commented Mar 19, 2024

eiffel-fl commented Mar 20, 2024

eiffel-fl left a comment

eiffel-fl Mar 20, 2024

alban Mar 20, 2024

eiffel-fl Mar 20, 2024

alban Mar 20, 2024

mauriciovasquezbernal left a comment

		@@ -213,10 +214,11 @@ func (t *Tracer) run() {
		buf := []byte{}
		args := record.RawSample[unsafe.Offsetof(execsnoopEvent{}.Args):]

trace exec: add exe path #1959

trace exec: add exe path #1959

Conversation

alban commented Aug 16, 2023 • edited

trace exec: add exe path

How to use

TBD

Testing done

mauriciovasquezbernal commented Jan 11, 2024

alban commented Jan 17, 2024

alban commented Jan 18, 2024

github-actions bot commented Mar 19, 2024

mauriciovasquezbernal commented Mar 19, 2024

alban commented Mar 19, 2024

eiffel-fl commented Mar 20, 2024

eiffel-fl left a comment

Choose a reason for hiding this comment

eiffel-fl Mar 20, 2024

Choose a reason for hiding this comment

alban Mar 20, 2024

Choose a reason for hiding this comment

eiffel-fl Mar 20, 2024

Choose a reason for hiding this comment

alban Mar 20, 2024

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

alban commented Aug 16, 2023 •

edited