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.

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

cleanup(driver): fix flags param #1469

Merged
merged 2 commits into from Nov 25, 2023

Conversation

ecbadeaux
Copy link
Contributor

@ecbadeaux ecbadeaux commented Nov 2, 2023

What type of PR is this?

/kind bug

/kind cleanup

Any specific area of the project related to this PR?

/area driver-bpf

Does this PR require a change in the driver versions?

I am not sure about this.

What this PR does / why we need it:

I am trying to address the issue where dup3 is handling the flag params as an unsigned integer when it should be handling it as in int.

Which issue(s) this PR fixes:

part of #515
(partial fix other issues still remain in 515)

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Copy link

github-actions bot commented Nov 2, 2023

Please double check driver/API_VERSION file. See versioning.

/hold

driver/bpf/fillers.h Outdated Show resolved Hide resolved
@Andreagit97 Andreagit97 added this to the next-driver milestone Nov 6, 2023
@ecbadeaux ecbadeaux force-pushed the incorrect-flags-dup3 branch 2 times, most recently from aa3a955 to da8036f Compare November 13, 2023 00:39
@poiana poiana added size/M and removed size/S labels Nov 13, 2023
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.

Ty! We may need another rebase.

driver/modern_bpf/definitions/events_dimensions.h Outdated Show resolved Hide resolved
userspace/libsinsp/parsers.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/parsers.cpp Outdated Show resolved Hide resolved
userspace/libscap/engine/gvisor/fillers.h Outdated Show resolved Hide resolved
@poiana poiana added size/S and removed size/M labels Nov 20, 2023
@ecbadeaux ecbadeaux force-pushed the incorrect-flags-dup3 branch 2 times, most recently from a724a4c to 5c1cec3 Compare November 20, 2023 07:14
Signed-off-by: Everett Badeaux <everettc1810@gmail.com>
@ecbadeaux
Copy link
Contributor Author

@incertum Should be good now.

driver/ppm_fillers.c Outdated Show resolved Hide resolved
driver/bpf/fillers.h Outdated Show resolved Hide resolved
driver/bpf/fillers.h Outdated Show resolved Hide resolved
@ecbadeaux
Copy link
Contributor Author

@incertum @Andreagit97 changes implemented

@FedeDP
Copy link
Contributor

FedeDP commented Nov 21, 2023

I just started kernel matrixes tests against this branch, to check whether we break anything!
https://github.com/falcosecurity/libs/actions/runs/6949833686

@ecbadeaux
Copy link
Contributor Author

case __NR_dup3:
			ret.status = scap_gvisor::fillers::fill_event_dup3_x(
			                 scap_buf, &ret.size, scap_err,
			                 gvisor_evt.exit().result(),
			                 gvisor_evt.old_fd(),
			                 gvisor_evt.new_fd(),
			                 dup3_flags_to_scap(gvisor_evt.flags()));
			break;

@incertum @Andreagit97
Can someone point to the codebase where I can find out the integer type for gvisor_evt.flags()? This may need to be C casted to (int) since it makes use of dup3_flags_to_scap()

@FedeDP
Copy link
Contributor

FedeDP commented Nov 22, 2023

Matrix for arm64:

KERNEL CMAKE-CONFIGURE KMOD BUILD KMOD SCAP-OPEN BPF-PROBE BUILD BPF-PROBE SCAP-OPEN MODERN-BPF SCAP-OPEN
amazonlinux2-5.4 🟢 🟢 🟢 🟢 🟢 🟡
amazonlinux2022-5.15 🟢 🟢 🟢 🟢 🟢 🟢
fedora-6.2 🟢 🟢 🟢 🟢 🟢 🟢
oraclelinux-4.14 🟢 🟢 🟢 🟡 🟡 🟡
oraclelinux-5.15 🟢 🟢 🟢 🟢 🟢 🟢
ubuntu-6.3 🟢 🟢 🟢 🟢 🟢 🟢

Matrix for x86_64:

KERNEL CMAKE-CONFIGURE KMOD BUILD KMOD SCAP-OPEN BPF-PROBE BUILD BPF-PROBE SCAP-OPEN MODERN-BPF SCAP-OPEN
amazonlinux2-4.19 🟢 🟢 🟢 🟢 🟢 🟡
amazonlinux2-5.10 🟢 🟢 🟢 🟢 🟢 🟢
amazonlinux2-5.15 🟢 🟢 🟢 🟢 🟢 🟢
amazonlinux2-5.4 🟢 🟢 🟢 🟢 🟢 🟡
amazonlinux2022-5.15 🟢 🟢 🟢 🟢 🟢 🟢
amazonlinux2023-6.1 🟢 🟢 🟢 🟢 🟢 🟢
archlinux-6.0 🟢 🟢 🟢 🟢 🟢 🟢
centos-3.10 🟢 🟢 🟢 🟡 🟡 🟡
centos-4.18 🟢 🟢 🟢 🟢
centos-5.14 🟢 🟢 🟢 🟢 🟢 🟢
fedora-5.17 🟢 🟢 🟢 🟢
fedora-5.8 🟢 🟢 🟢 🟢 🟢 🟢
fedora-6.2 🟢 🟢 🟢 🟢 🟢 🟢
oraclelinux-3.10 🟢 🟢 🟢 🟡 🟡 🟡
oraclelinux-4.14 🟢 🟢 🟢 🟢 🟢 🟡
oraclelinux-5.15 🟢 🟢 🟢 🟢 🟢 🟢
oraclelinux-5.4 🟢 🟢 🟢 🟢 🟢 🟡
ubuntu-4.15 🟢 🟢 🟢 🟢 🟢 🟡
ubuntu-6.3 🟢 🟢 🟢 🟢 🟢 🟢

No regressions from master!

Andreagit97
Andreagit97 previously approved these changes Nov 22, 2023
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 Nov 22, 2023

LGTM label has been added.

Git tree hash: 250e9c1d7f2b4796dccbd6ea818149a4ab8cc628

@Andreagit97
Copy link
Member

/hold

@Andreagit97
Copy link
Member

@incertum @Andreagit97
Can someone point to the codebase where I can find out the integer type for gvisor_evt.flags()? This may need to be C casted to (int) since it makes use of dup3_flags_to_scap()

I think that the right way to do that would be to change the proto message

but i'm not sure this is what we want, in the end, it is already on 32 bits... WDYT? @LucaGuerra

FedeDP
FedeDP previously approved these changes Nov 24, 2023
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@LucaGuerra
Copy link
Contributor

I think that the right way to do that would be to change the proto message

That proto is vendored from gVisor and we should not change it directly in the Falco codebase. I think a cast (implicit like it is now or explicit) is fine in this case.

Signed-off-by: Everett Badeaux <everettc1810@gmail.com>
@ecbadeaux
Copy link
Contributor Author

ecbadeaux commented Nov 25, 2023

Can you guys reapprove @FedeDP @Andreagit97 and merge? I added the explicit cast to parsers.cpp now everything is consistent.

Copy link
Contributor

@FedeDP FedeDP 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 Nov 25, 2023
@poiana
Copy link
Contributor

poiana commented Nov 25, 2023

LGTM label has been added.

Git tree hash: 884e361d2e608978aa3dd7dfdad4b3190ba1dcbe

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 Nov 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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,FedeDP,incertum]

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

@incertum
Copy link
Contributor

/unhold

@poiana poiana merged commit c2fd308 into falcosecurity:master Nov 25, 2023
36 checks passed
@Andreagit97 Andreagit97 changed the title cleanup(dup3): fix flags param cleanup(driver): fix flags param Jan 8, 2024
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.

None yet

6 participants