Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix listen syscall backlog field size #1256

Merged
merged 1 commit into from
Aug 18, 2023
Merged

fix listen syscall backlog field size #1256

merged 1 commit into from
Aug 18, 2023

Conversation

oheifetz
Copy link
Contributor

Reported by: github issue #515

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 API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap-engine-udig

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it: fix listen syscall backlog field size

Which issue(s) this PR fixes: issue #515

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@poiana poiana added the size/M label Jul 29, 2023
@github-actions
Copy link

github-actions bot commented Jul 29, 2023 •
edited

Please double check driver/API_VERSION file. See versioning.

/hold

@Andreagit97 Andreagit97 modified the milestones: 0.13.0, next-driver Jul 31, 2023
Andreagit97
Andreagit97 reviewed Jul 31, 2023
View reviewed changes
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.

Also here we need to remove the new event pair, the patch should become much easier

driver/event_table.c Outdated Show resolved Hide resolved
driver/modern_bpf/definitions/events_dimensions.h Outdated Show resolved Hide resolved
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/listen.bpf.c Show resolved Hide resolved
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/listen.bpf.c Outdated Show resolved Hide resolved
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/listen.bpf.c Outdated Show resolved Hide resolved
test/drivers/test_suites/syscall_enter_suite/socketcall_e.cpp Outdated Show resolved Hide resolved
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/listen.bpf.c Show resolved Hide resolved
@incertum
Copy link
Contributor

incertum commented Aug 1, 2023

Same comment as @Andreagit97 see my old post in the closed PR #1200 (comment) again our apologies.

@incertum
Copy link
Contributor

incertum commented Aug 1, 2023

One more note we enforce usage of git rebase as we have git merge banned, see https://github.com/falcosecurity/.github/blob/main/CONTRIBUTING.md#pull-requests

@poiana poiana added size/S and removed size/M labels Aug 4, 2023
@incertum
Copy link
Contributor

incertum commented Aug 4, 2023

@oheifetz thanks for removing the new event type.

  • Now we still need to update the respective type for PPME_SOCKET_LISTEN_E in the event table
  • And update in the userspace/libsinsp/test/* respectively

Uhm @Andreagit97 I don't see a filler for listen in https://github.com/falcosecurity/libs/blob/6ddef943526368cb9a6a52ea51a5c691cd41caae/driver/ppm_fillers.h it's pointing to sys_autofill

libs/driver/fillers_table.c

Line 42 in 6ddef94

[PPME_SOCKET_LISTEN_E] = {FILLER_REF(sys_autofill), 2, APT_SOCK, {{0}, {1} } },

and we only push values of unsigned long in there. The right fix is likely adding dedicated fillers for old bpf and kernel module as well.

@oheifetz oheifetz force-pushed the issue_515_listen branch 3 times, most recently from 8e74607 to b9a3329 Compare August 5, 2023 18:51
@poiana poiana added size/M and removed size/S labels Aug 5, 2023
@oheifetz
Copy link
Contributor Author

oheifetz commented Aug 6, 2023

OK, pushed all changes and all driver tests passed also fixed the issues noted in this thread by @incertum and @Andreagit97

incertum
incertum reviewed Aug 7, 2023
View reviewed changes
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

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

@Andreagit97 re schema version do we bump in this PR?

driver/ppm_fillers.c Outdated Show resolved Hide resolved
Andreagit97
Andreagit97 reviewed Aug 7, 2023
View reviewed changes
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 thanks! For what concern the SCHEMA_VERSION, yeah we need a minor bump

driver/ppm_fillers.c Outdated Show resolved Hide resolved
@Andreagit97 Andreagit97 mentioned this pull request Aug 7, 2023
Open
Andreagit97
Andreagit97 previously approved these changes Aug 14, 2023
View reviewed changes
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
Thank you!

@poiana poiana added the lgtm label Aug 14, 2023
@poiana
Copy link
Contributor

poiana commented Aug 14, 2023

LGTM label has been added.

Git tree hash: 6cb15fd12ed6c47cb13a95ce2fb6183df88f55eb

incertum
incertum reviewed Aug 14, 2023
View reviewed changes
driver/ppm_fillers.c Outdated Show resolved Hide resolved
@oheifetz
fix listen syscall backlog field size
5eb1c0b 
bump schema version minor
Reported by: github issue falcosecurity#515

Signed-off-by: Ofer Heifetz <oheifetz@gmail.com>
@poiana poiana removed the lgtm label Aug 18, 2023
Andreagit97
Andreagit97 approved these changes Aug 18, 2023
View reviewed changes
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 Aug 18, 2023
@poiana
Copy link
Contributor

poiana commented Aug 18, 2023

LGTM label has been added.

Git tree hash: d9707002c3d189357940d4ab45b6c087e9dd5f5c

incertum
incertum approved these changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@incertum incertum 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 Aug 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, incertum, oheifetz

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:
  • OWNERS [Andreagit97,incertum]

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

@poiana poiana merged commit 2293e3e into falcosecurity:master Aug 18, 2023
35 checks passed
@oheifetz oheifetz deleted the issue_515_listen branch August 19, 2023 09:13
@incertum incertum mentioned this pull request Aug 19, 2023
Open
59 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@incertum incertum incertum approved these changes

@Andreagit97 Andreagit97 Andreagit97 approved these changes

@hbrueckner hbrueckner Awaiting requested review from hbrueckner

@jasondellaluce jasondellaluce Awaiting requested review from jasondellaluce

Assignees

@incertum incertum

@Andreagit97 Andreagit97

Labels
approved dco-signoff: yes lgtm release-note-none size/M
Projects
None yet
Milestone
6.0.0+driver
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@oheifetz @incertum @poiana @Andreagit97