Skip to content

new(driver,userspace/libsinsp): added support for state modifying epoll_create and epoll_create1.#596

Merged
poiana merged 13 commits intomasterfrom
new/epoll_create
Oct 31, 2022
Merged

new(driver,userspace/libsinsp): added support for state modifying epoll_create and epoll_create1.#596
poiana merged 13 commits intomasterfrom
new/epoll_create

Conversation

@FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Sep 12, 2022

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area driver-kmod
/area driver-bpf
/area libsinsp

Does this PR require a change in the driver versions?

Yep, because it adds a new event.

/version driver-SCHEMA-version-minor

What this PR does / why we need it:

Adds support for epoll_create and epoll_create1 syscalls, because they create a FD thus are needed for sinsp state.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new: support `epoll_create` and `epoll_create1` syscalls.

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 12, 2022

Github 503 :/

Copy link
Contributor

@jasondellaluce jasondellaluce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Andreagit97
Copy link
Member

Probably we need to add them also in the modern probe
/hold

@LucaGuerra
Copy link
Contributor

Would this case fit along the others in the creates_fd_generic test you recently added?

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 13, 2022

Would this case fit along the others in the creates_fd_generic test you recently added?

Done!

Probably we need to add them also in the modern probe

Done!

@LucaGuerra @Andreagit97

@hbrueckner
Copy link
Contributor

Probably we need to add them also in the modern probe

Done!

Awesome @FedeDP . I will look later thru the patch set. Thanks.

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 13, 2022

But epoll_create1 tests are failing and i don't get why :/

@hbrueckner
Copy link
Contributor

But epoll_create1 tests are failing and i don't get why :/

Looks like the syscall passes but does not create an event:

[ RUN      ] SyscallEnter.epoll_create1E
HALLO: fd=245 errno=Success
/root/git/falcosecurity/libs/test/modern_bpf/event_class/event_class.cpp:378: Failure
Failed
There is no event in the buffer.

with using printf("HALLO: fd=%i errno=%s\n", fd, strerror(errno));

@hbrueckner
Copy link
Contributor

@FedeDP Tests pass on s390x:

# ./test/modern_bpf/bpf_test  |grep epoll
[ RUN      ] SyscallExit.epoll_create1X
[       OK ] SyscallExit.epoll_create1X (0 ms)
[ RUN      ] SyscallExit.epoll_createX
[       OK ] SyscallExit.epoll_createX (0 ms)
[ RUN      ] SyscallEnter.epoll_create1E
[       OK ] SyscallEnter.epoll_create1E (0 ms)
[ RUN      ] SyscallEnter.epoll_createE
[       OK ] SyscallEnter.epoll_createE (0 ms)

will review then later.

Copy link
Contributor

@hbrueckner hbrueckner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FedeDP Reviewed the modern bpf part. Few minor typos and we should map the flags to scap.

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 13, 2022

@hbrueckner fixed everything!

Copy link
Contributor

@hbrueckner hbrueckner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FedeDP only minor comments for kernel / bpf driver.

@hbrueckner
Copy link
Contributor

@FedeDP other the minors above, this looks good to me.
/lgtm

@poiana poiana added the lgtm label Sep 13, 2022
Moreover, actually make mlock2 use correct flags, instead of mlockall ones.
Finally, fixed sinsp tests on syscall-driver events number.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 28, 2022

Rebased on top of master.

@poiana poiana removed the lgtm label Oct 28, 2022
@poiana poiana requested a review from leogr October 28, 2022 07:57
leogr
leogr previously approved these changes Oct 28, 2022
@poiana poiana added the lgtm label Oct 28, 2022
@poiana
Copy link
Contributor

poiana commented Oct 28, 2022

LGTM label has been added.

DetailsGit tree hash: 8daa64dbb061e4f2e87d90f356b29bbf5f80b615

Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work as always!

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 31, 2022

@Andreagit97 thanks for your review! Addressed in latest commit!

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

Co-authored-by: Andrea Terzolo <andreaterzolo@polito.it>
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@poiana
Copy link
Contributor

poiana commented Oct 31, 2022

LGTM label has been added.

DetailsGit tree hash: b57738ff9664e41821f9765b3a8adcd2cbccef33

@poiana
Copy link
Contributor

poiana commented Oct 31, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, hbrueckner, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [Andreagit97,FedeDP,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 31, 2022

/unhold

@poiana poiana merged commit 212870e into master Oct 31, 2022
@poiana poiana deleted the new/epoll_create branch October 31, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants