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

fix(driver/bpf): fixed a couple of verifier issues. #1896

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Jun 5, 2024

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area driver-bpf

Does this PR require a change in the driver versions?

What this PR does / why we need it:

#1814 and #1767 introduced 2 old bpf verifier regressions.
This PR aims at fixing them.

Which issue(s) this PR fixes:

Fixes #1890

Special notes for your reviewer:

Already tested against our kernel matrix (https://github.com/falcosecurity/libs/actions/runs/9386047428):
AMD64:

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 🟢 🟢 🟢 🟢 🟢 🟢
archlinux-6.7 🟢 🟢 🟢 🟢 🟢 🟢
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-5.8 🟢 🟢 🟢 🟢 🟢 🟡
ubuntu-6.5 🟢 🟢 🟢 🟢 🟢 🟢

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.5 🟢 🟢 🟢 🟢 🟢 🟢

Does this PR introduce a user-facing change?:

fix(driver/bpf): fixed a couple of verifier issues.

/milestone next-driver
/cc @incertum @Andreagit97

Copy link

github-actions bot commented Jun 5, 2024

Please double check driver/SCHEMA_VERSION file. See versioning.

/hold

@@ -571,236 +571,158 @@ FILLER(sys_poll_x, true)
return bpf_poll_parse_fds(data, false);
}

#define MAX_IOVCNT 32
#define MAX_IOVCNT_COMPAT 8
#ifdef CONFIG_COMPAT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only real downside of the patch (except for its ugliness probably :D ) is that we will have max iovec count of 8 when config compat is enabled; this is needed because we must have a constant to be able to unroll the loops below. Not a big deal; this is only on the old ebpf probe.

if (curr_shift > SCRATCH_SIZE_HALF)
break;

long curr_len;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know why, it won't make me use memcpy (that is an alias for __builtin_memcpy from clang) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If i could use memcpy the patch would look much tidier, but that's not the case unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh we can't because for __builtin_memcpy size must be a compile time constant.

@@ -4312,10 +4234,12 @@ FILLER(sys_recvfrom_x, true)
unsigned long val;
uint16_t size = 0;
long retval;
int addrlen;
int addrlen = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

recvfrom instead looks tidier now imo.


static __always_inline int bpf_parse_readv_writev_bufs_64(struct filler_data *data,
static __always_inline int bpf_parse_readv_writev_bufs(struct filler_data *data,
Copy link
Member

Choose a reason for hiding this comment

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

to be honest I would prefer to revert this code to what we had before this PR https://github.com/falcosecurity/libs/pull/1814/files#diff-a8b6fc07077f8fcef2f22e51392da619a5e68b7c603c62b3ab93ce5b6be22d6b. The new implementation seems pretty easy to break and the only advantage is that we support readv and writev on 32 bits... that IMO is a corner case.

Btw if you feel confident we can also merge it and maybe revert it if we see it doesn't work well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, i would love to be able to just use memcpy there (the clang builtin of course); but it won't work :/ it would make the code much better...

@incertum
Copy link
Contributor

incertum commented Jun 5, 2024

🚀 posted current results here #1890 (comment)

@FedeDP FedeDP force-pushed the fix/verifier branch 2 times, most recently from 1ed0fb2 to 5e706a2 Compare June 11, 2024 13:34
@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 12, 2024

@incertum this is now ready IMO. Other regressions you highlighted (mostly referring to #1890 (comment)) are not verifier issues, thus we can go on with this.

I'd like to understand them too though, we can digging into that on #1890 ; feel free to add any new info you got :)

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

FedeDP commented Jun 12, 2024

/unhold

@incertum
Copy link
Contributor

@incertum this is now ready IMO. Other regressions you highlighted (mostly referring to #1890 (comment)) are not verifier issues, thus we can go on with this.

I'd like to understand them too though, we can digging into that on #1890 ; feel free to add any new info you got :)

Agree, we will dig into this separately.

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

Nice job @FedeDP and also thanks!

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 Jun 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, 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:

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 e3d0ab8 into master Jun 13, 2024
49 of 50 checks passed
@poiana poiana deleted the fix/verifier branch June 13, 2024 08:16
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.

Serious eBPF driver regression starting with 0.16.x libs
4 participants