Skip to content

wip: chore/improvements#298

Closed
FedeDP wants to merge 6 commits intomasterfrom
chore/improvements
Closed

wip: chore/improvements#298
FedeDP wants to merge 6 commits intomasterfrom
chore/improvements

Conversation

@FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Apr 22, 2022

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area driver-kmod

/area driver-ebpf

/area libscap

/area libsinsp

/area tests

/area proposals

What this PR does / why we need it:

This PR has been splitted from #86.

  • added a flexible array member to struct ppm_evt_hdr to manage events payload
    Std states:

As a special case, the last element of a structure with more than one named member may have an incomplete array type; this is called a flexible array member. In most situations, the flexible array member is ignored. In particular, the size of the structure is as if the flexible array member were omitted except that it may have more trailing padding than the omission would imply.

Note about the padding: we use an uint8_t for flexible array member, thus it shall not change padding in our case: you store sizeof(ppm_evt_hdr) + sizeof hdrs + sizeof(payload) and you load exactly the same;
but the flexible array member helps in avoiding pointer arithmetic when dealing with hdrs and payload.

I tested the dump/read of scap files:

  1. new version from new version
  2. new version from old version
  3. old version from new version
  • merged g_syscall_code_routing_table into g_syscall_table
    The g_syscall_code_routing_table was not really useful by itself, and the 2 tables had same indexing.

  • dropped syscall_info_table; it is now automatically generated during first call to scap_get_syscall_info_table using data from syscall_table and event_table. This way, categories or flags cannot be desynced anymore.

  • forced gnu99 as C standard

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Andreagit97
Andreagit97 previously approved these changes Apr 24, 2022
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.

Thanks for the amazing work @FedeDP 😍 Seeing the two event tables together was one of my forbidden dreams 😆
/approve

@poiana
Copy link
Contributor

poiana commented Apr 24, 2022

LGTM label has been added.

DetailsGit tree hash: 67d6ead259b50e92e94a915b3c737d8e84bbb5de

Andreagit97
Andreagit97 previously approved these changes Apr 26, 2022
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 poiana added the lgtm label Apr 26, 2022
@poiana
Copy link
Contributor

poiana commented Apr 26, 2022

LGTM label has been added.

DetailsGit tree hash: 5fa4cf3bd8ca592ca229cbc30ec8fe5084dd302a

@poiana
Copy link
Contributor

poiana commented Apr 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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 Apr 26, 2022

Hold this one until Falco 0.32.0 is out; too big and risky change (even if it is just a big refactor).

/hold

@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 28, 2022

/test build-libs

@FedeDP
Copy link
Contributor Author

FedeDP commented May 24, 2022

Remainder for myself: understand why syscall_info_table has EF_NONE/EF_DROP_SIMPLE_CONS (ie: a subset of event_table event flags). Can't we drop the flags from there and deduplicate a bit these 2 tables?

@FedeDP
Copy link
Contributor Author

FedeDP commented May 24, 2022

/cc @Andreagit97 @alacuku

@poiana poiana requested a review from Andreagit97 May 24, 2022 13:55
@poiana
Copy link
Contributor

poiana commented May 24, 2022

@FedeDP: GitHub didn't allow me to request PR reviews from the following users: alacuku.

Note that only falcosecurity members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @Andreagit97 @alacuku

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.

FedeDP added 5 commits May 25, 2022 11:26
…array member to struct ppm_evt_hdr to manage events payload (both len headers and actual event data) without abusing c pointer arithmetic.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…le` into `g_syscall_table`.

They were the exactly same table (ie: same indexes) but with different values.
Extended `struct syscall_evt_pair` adding a `ppm_code' member.

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

It now gets generated at the first call to scap_get_syscall_info_table(), and uses flags, categories and names from event_table.
This way, we avoid issues where the 2 tables get unsynced.

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

poiana commented May 25, 2022

New changes are detected. LGTM label has been removed.

@FedeDP
Copy link
Contributor Author

FedeDP commented May 25, 2022

TODO for completely dropping syscall_info_table, in favor of an autogenerated table during first call to scap_get_syscall_info_table:

  • fix category and name for syscalls that are not mapped to a specific event (like PPM_SC_EXIT and PPM_SC_RESTART_SYSCALL)

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the chore/improvements branch from 2fef46c to d367474 Compare May 25, 2022 09:35
@FedeDP FedeDP changed the title Chore/improvements wip: chore/improvements May 30, 2022
@poiana
Copy link
Contributor

poiana commented Aug 28, 2022

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@poiana
Copy link
Contributor

poiana commented Sep 27, 2022

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 5, 2022

Biggest parts of this one, ie:

merged g_syscall_code_routing_table into g_syscall_table
The g_syscall_code_routing_table was not really useful by itself, and the 2 tables had same indexing.

Merged.

dropped syscall_info_table; it is now automatically generated during first call to scap_get_syscall_info_table using data from syscall_table and event_table. This way, categories or flags cannot be desynced anymore.

There is an open PR for that: #649

The only remaining part depends upon C99. I am not sure whether it is an issue or now. Still, i think the flexible array member cleanup is good from a dev PoV.

@poiana
Copy link
Contributor

poiana commented Nov 4, 2022

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

@poiana poiana closed this Nov 4, 2022
@poiana
Copy link
Contributor

poiana commented Nov 4, 2022

@poiana: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

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.

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.

3 participants