wip: fix: PT_FD, PT_ERRNO, PT_PID, PT_FDLIST with int32 #526
Conversation
…LIST32 field types. This commit addresses userspace side. Signed-off-by: Federico Di Pierro <[email protected]>
…NO32 types. Signed-off-by: Federico Di Pierro <[email protected]>
…RNO32 types. Signed-off-by: Federico Di Pierro <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Missing:
|
|
This should fix first 2 points of #515 ! |
… PT_ERRNO32 types. Signed-off-by: Federico Di Pierro <[email protected]>
…T32, PT_ERRNO32 types. I kept backward compatibility with existing dynamic params too. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
FedeDP
force-pushed
the
fix/fd_errno_pid_fdlist_int32
branch
from
b2c7c07 to
a5ab505
Compare
|
/retest |
| /* PPME_SYSCALL_READ_X */{"read", EC_IO_READ, EF_USES_FD | EF_READS_FROM_FD | EF_DROP_SIMPLE_CONS, 2, {{"res", PT_ERRNO, PF_DEC}, {"data", PT_BYTEBUF, PF_NA} } }, | ||
| /* PPME_SYSCALL_WRITE_E */{"write", EC_IO_WRITE, EF_USES_FD | EF_WRITES_TO_FD | EF_DROP_SIMPLE_CONS, 2, {{"fd", PT_FD, PF_DEC}, {"size", PT_UINT32, PF_DEC} } }, | ||
| /* PPME_SYSCALL_WRITE_X */{"write", EC_IO_WRITE, EF_USES_FD | EF_WRITES_TO_FD | EF_DROP_SIMPLE_CONS, 2, {{"res", PT_ERRNO, PF_DEC}, {"data", PT_BYTEBUF, PF_NA} } }, | ||
| /* PPME_SYSCALL_OPEN_X */{"open", EC_FILE, EF_CREATES_FD | EF_MODIFIES_STATE, 6, {{"fd", PT_FD32, PF_DEC}, {"name", PT_FSPATH, PF_NA}, {"flags", PT_FLAGS32, PF_HEX, file_flags}, {"mode", PT_UINT32, PF_OCT}, {"dev", PT_UINT32, PF_HEX}, {"ino", PT_UINT64, PF_DEC} } }, |
There was a problem hiding this comment.
I think we should indeed create new events alltogether, instead of updating parameters types, to retain backward compatibility.
There was a problem hiding this comment.
Yeah, it's going to be a lot of fun when reading captures. Parameter length checks every other line ;)
|
Another possible solution, discussed with @Andreagit97 , is to add a new event flag, like |
|
I took another look at this, I have some remarks that could be useful when you refactor/rewrite/rebase this PR: I'm fine with FDs but keep in mind that if we want to create versions of events with a 32-bit PID/TID (for clone, fork, execve for example) we'll have to still maintain 64 bit versions since we can now rely on them (we do in gVisor). So, you can't deprecate or mark as "old" or "backwards compatibility" the 64 bit event but they'll be two different current events which could make the whole thing a bit more complicated. Since the events that use PIDs are not a lot and not so frequent, except maybe for |
|
/milestone 0.11.0 |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale I keep this open for reference, but most probably the patch will need to be entirely rewritten ;) |
|
/milestone 0.12.0 |
|
@FedeDP it turns out for Therefore wanted to ask you if we could revive this PR and create the new events throughout and then for the new execve* events I can push onto this branch with changes to support loginuid changes, see staging commit incertum@b171a24 we can go with u32 and re-convert -1 in userspace or we just throughout go with s64, either way fine by me as long as it fixes wrongly casted uids. We definitely need to keep -1 ... |
|
FWIW, in the kernel uids are unsigned 32 bit (on x86-64 at least): https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/posix_types.h#L49 so using s64 is IMHO wrong precisely because it introduces a difference between |
oh thanks @gnosek 🙏 meaning we can only go the route u32, how would you still ensure we expose -1 in userspace as we currently do? |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
maybe we could also close this, not sure we will tackle this in the short term |
|
I will leave this open just to remind me that sooner or later this should be tackled :D |
@FedeDP more concrete plans? 🙃 ty! |
|
In my opinion we can close this one 🤔 |
|
Yep this needs to be re-done from scratch. |
|
@FedeDP: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area driver-kmod
/area driver-bpf
/area driver-modern-bpf
/area libscap
/area libsinsp
Does this PR require a change in the driver versions?
This will probably need a maj schema bump.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: