Skip to content

wip: chore: minor improvements; mainly: dropped g_syscall_code_routing_table and added an m4 script to automatically create syscall tables (normal + ia32)#86

Closed
FedeDP wants to merge 3 commits intofalcosecurity:masterfrom
FedeDP:minor_improvements

Conversation

@FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Sep 21, 2021

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area libscap

What this PR does / why we need it:

  • 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_table.c in favour of syscall_table.m4 script that will automatically build syscall_table.c
    at compile time. It allows adding a new syscall by just inserting a one-liner in the new X_SYSCALLS X macro.

It uses an m4 script; i think that m4 is the most widely available solution.
Why does this latter change matter? Because we were already hit by issues like forgetting to populate ia32 syscalls, or forgetting to populate PPM_SC_ code (the latter case is already somewhat mitigated by above point).

  • forced gnu99 as C standard

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@poiana
Copy link
Contributor

poiana commented Sep 21, 2021

Hi @FedeDP. Thanks for your PR.

I'm waiting for a falcosecurity member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@leogr
Copy link
Member

leogr commented Sep 21, 2021

/ok-to-test

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 21, 2021

/retest

@FedeDP FedeDP force-pushed the minor_improvements branch from cbcb25c to 338fddd Compare November 15, 2021 13:09
@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 15, 2021

Rebased on master.

@FedeDP FedeDP force-pushed the minor_improvements branch from 338fddd to 23e6ee1 Compare November 16, 2021 14:16
@leogr
Copy link
Member

leogr commented Nov 25, 2021

Closing and reopening to trigger the CI
/close

@poiana poiana closed this Nov 25, 2021
@poiana
Copy link
Contributor

poiana commented Nov 25, 2021

@leogr: Closed this PR.

Details

In response to this:

Closing and reopening to trigger the CI
/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.

@leogr
Copy link
Member

leogr commented Nov 25, 2021

/reopen

@poiana poiana reopened this Nov 25, 2021
@poiana
Copy link
Contributor

poiana commented Nov 25, 2021

@leogr: Reopened this PR.

Details

In response to this:

/reopen

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 FedeDP force-pushed the minor_improvements branch 2 times, most recently from 601be0f to 45c1c2f Compare December 17, 2021 13:47
@FedeDP FedeDP changed the title Minor improvements wip: minor improvements Dec 20, 2021
@FedeDP FedeDP force-pushed the minor_improvements branch from 45c1c2f to d5ed911 Compare December 20, 2021 11:30
@FedeDP FedeDP force-pushed the minor_improvements branch from d5ed911 to 925bd15 Compare February 1, 2022 09:20
@poiana
Copy link
Contributor

poiana commented Feb 1, 2022

[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

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

@poiana poiana added the approved label Feb 1, 2022
@FedeDP FedeDP force-pushed the minor_improvements branch from 1c59f39 to 3ebf236 Compare March 22, 2022 10:26
@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 22, 2022

/test build-libs-bundled-deps

@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 22, 2022

/test build-libs-with-chisels

@FedeDP FedeDP force-pushed the minor_improvements branch from 3ebf236 to 75c1523 Compare March 29, 2022 15:52
FedeDP added 3 commits April 15, 2022 10:10
…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>
…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 syscall name member, and a `ppm_code' member.

Moreover, dropped syscall_table.c in favour of syscall_table.m4 script that will automatically build syscall_table.c
at compile time.
This allows adding a new syscall by just inserting a one-liner in the new `X_SYSCALLS` X macro.
Note also that now all __NR_(ia32_)_foo code is protected by ifdef, as it makes sense.

Finally, added old syscall_table.c to gitignore for the driver folder.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Moreover, updated driver CMakeLists to always refresh syscall_table.c during builds.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the minor_improvements branch from 75c1523 to d803370 Compare April 15, 2022 08:11
@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 21, 2022

I think i will split this PR in:

  • a PR that adds the flexible array member to struct ppm_evt_hdr to manage events payload + enforces c99 + merges g_syscall_code_routing_table into g_syscall_table
  • a PR that uses an m4 script to automatically create syscall tables

The latter is quite opinionated and might not be merged soon; the former instead should have the consensus to be merged.

@leogr
Copy link
Member

leogr commented Apr 21, 2022

I think i will split this PR in:

  • a PR that adds the flexible array member to struct ppm_evt_hdr to manage events payload + enforces c99 + merges g_syscall_code_routing_table into g_syscall_table
  • a PR that uses an m4 script to automatically create syscall tables

The latter is quite opinionated and might not be merged soon; the former instead should have the consensus to be merged.

Hey @FedeDP

I totally agree with splitting this into several PRs. I'm also ok with C99. Moreover, I'd suggest documenting somewhere the C standard we are going to use.

@FedeDP FedeDP mentioned this pull request Apr 22, 2022
@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 22, 2022

I split #298 from this one, with the first point above:

  • flexible array member to struct ppm_evt_hdr to manage events payload
  • enforces c99
  • merges g_syscall_code_routing_table into g_syscall_table

This PR (the #86) will instead become only the m4 script one and will be rebased on top of #298

@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 26, 2022

/hold

@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 28, 2022

/test build-libs-bundled-deps

@FedeDP FedeDP changed the title chore: minor improvements; mainly: dropped g_syscall_code_routing_table and added an m4 script to automatically create syscall tables (normal + ia32) wip: chore: minor improvements; mainly: dropped g_syscall_code_routing_table and added an m4 script to automatically create syscall tables (normal + ia32) 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

@poiana
Copy link
Contributor

poiana commented Oct 27, 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 Oct 27, 2022
@poiana
Copy link
Contributor

poiana commented Oct 27, 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.

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 31, 2022

Most of this PR was split in multiple ones.
We decided to avoid the m4 script, therefore there is not much else we can do.
The only open points are:

@FedeDP FedeDP deleted the minor_improvements branch October 31, 2022 14:14
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.

4 participants