-
Notifications
You must be signed in to change notification settings - Fork 416
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
feat(events): add chmod_common event #4339
feat(events): add chmod_common event #4339
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.
LGTM
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.
Could you rebase on top of main after?
@@ -262,7 +263,7 @@ var CoreEvents = map[ID]Definition{ | |||
params: []trace.ArgMeta{ | |||
{Type: "const char*", Name: "pathname"}, | |||
{Type: "int", Name: "flags"}, | |||
{Type: "mode_t", Name: "mode"}, | |||
{Type: "umode_t", Name: "mode"}, |
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.
@NDStrahilevitz we need standardise names or migrate entirely to types instead of strings. Seeing this change and double checking 4737625#diff-561afb519199f76436d77252de43fdbc7020a8603bb0dd8226d8eea314c4ba32R29-R154, I realized that we can't be confident of the type size based on the name only, check these:
https://elixir.bootlin.com/linux/v6.11.1/source/include/linux/types.h#L23
https://elixir.bootlin.com/linux/v6.11.1/source/include/linux/types.h#L24
https://elixir.bootlin.com/linux/v6.11.1/source/arch/x86/include/uapi/asm/posix_types_32.h#L11
https://elixir.bootlin.com/linux/v6.11.1/source/include/uapi/asm-generic/posix_types.h#L24
unsigned short
OR unsigned int
.
I'll review all ArgZeroValueFromType()
named types again and chose the larger option when necessary.
f03f7f8
to
ee8f534
Compare
@OriGlassman to fix integration tests: diff --git a/tests/integration/event_filters_test.go b/tests/integration/event_filters_test.go
index e3e9868ea..ad5810821 100644
--- a/tests/integration/event_filters_test.go
+++ b/tests/integration/event_filters_test.go
@@ -1603,7 +1603,7 @@ func Test_EventFilters(t *testing.T) {
expectEvent(anyHost, "fakeprog1", testutils.CPUForTests, anyPID, 0, events.Openat, orPolNames("comm-event-data-64"), orPolIDs(64),
expectArg("dirfd", int32(0)),
expectArg("flags", int32(0)),
- expectArg("mode", uint32(0)),
+ expectArg("mode", uint16(0)),
),
},
[]string{},
@@ -1615,7 +1615,7 @@ func Test_EventFilters(t *testing.T) {
[]trace.Event{
expectEvent(anyHost, "fakeprog2", testutils.CPUForTests, anyPID, 0, events.Open, orPolNames("comm-event-data-42"), orPolIDs(42),
expectArg("flags", int32(0)),
- expectArg("mode", uint32(0)),
+ expectArg("mode", uint16(0)),
),
},
[]string{},
@@ -1683,7 +1683,7 @@ func Test_EventFilters(t *testing.T) {
expectEvent(anyHost, "fakeprog1", testutils.CPUForTests, anyPID, 0, events.Openat, orPolNames("comm-event-retval-64"), orPolIDs(64),
expectArg("dirfd", int32(0)),
expectArg("flags", int32(0)),
- expectArg("mode", uint32(0)),
+ expectArg("mode", uint16(0)),
),
},
[]string{}, |
ee8f534
to
5791514
Compare
events like chmod and mknod used mode_t type which is 32 bit, but should use umode_t (note the 'u'), which is 16 bit.
5791514
to
a869f80
Compare
/fast-forward |
@@ -0,0 +1,34 @@ | |||
# chmod_common |
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 don't think the name chmod_common is so clear to the user.
A better name for this event may be file_chmod
1. Explain what the PR does
add chmod_common event
2. Explain how to test it
./tracee -e=chmod_common
3. Other comments