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(bpf): fixed a couple of clang15 verifier issues. #858

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Feb 1, 2023

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:

This fixes a couple of issues spotted while running a probe built with clang 15:

clang -v
clang version 15.0.7
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-pc-linux-gnu/12.2.1
Found candidate GCC installation: /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.1
Selected GCC installation: /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.1
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Selected multilib: .;@m64

Below the verifier details:

math between map_value pointer and register with unbounded min value is not allowed
processed 1719 insns (limit 1000000) max_states_per_insn 0 total_states 32 peak_states 32 mark_read 19

-- END PROG LOAD LOG --
libscap: bpf_load_program() event=raw_tracepoint/filler/proc_startupdate: Operation not permitted (1)
R8 invalid mem access 'map_value_or_null'
processed 119 insns (limit 1000000) max_states_per_insn 0 total_states 6 peak_states 6 mark_read 5

-- END PROG LOAD LOG --
libscap: bpf_load_program() event=raw_tracepoint/filler/sys_empty: Operation not permitted (1)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(bpf): fixed a couple of verifier issues with clang15

@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 1, 2023

/milestone next-driver

@@ -303,6 +303,7 @@ FILLER_RAW(terminate_filler)

FILLER(sys_empty, true)
{

Copy link
Contributor

Choose a reason for hiding this comment

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

unneeded extra newline


if (exe_len == -EFAULT)
if (exe_len < 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

For reviewers:

 * long bpf_probe_read_str(void *dst, u32 size, const void *unsafe_ptr)
 * 	Description
 * 		Copy a NUL terminated string from an unsafe kernel address
 * 		*unsafe_ptr* to *dst*. See **bpf_probe_read_kernel_str**\ () for
 * 		more details.
 *
 * 		Generally, use **bpf_probe_read_user_str**\ () or
 * 		**bpf_probe_read_kernel_str**\ () instead.
 * 	Return
 * 		On success, the strictly positive length of the string,
 * 		including the trailing NUL character. On error, a negative
 * 		value.

Source: linux include/uapi/linux/bpf.h

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@FedeDP FedeDP Feb 1, 2023

Choose a reason for hiding this comment

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

I think so! You just earned a coauthored commit :P

@poiana poiana added size/M and removed size/S labels Feb 1, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 2, 2023

Uh oh, ci is failing with:

terminate called after throwing an instance of 'scap_open_exception'
  what():  libscap: bpf_load_program() event=raw_tracepoint/filler/proc_startupdate_2: Operation not permitted
BPF program is too large.

Well, i can't see where i did touch proc_startupdate_2 though.

@FedeDP FedeDP force-pushed the fix/clang15_bpf branch 2 times, most recently from e940165 to 80bda9b Compare February 2, 2023 10:40
@poiana poiana added size/S and removed size/M labels Feb 2, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 2, 2023

Dropped last 2 commits as they were breaking proc_startupdate_2 filler :/
We have a very tight balance; every change we make to our fillers the chance of breaking on some kernels/clangs are really high...

Andreagit97
Andreagit97 previously approved these changes Feb 6, 2023
@Andreagit97
Copy link
Member

Andreagit97 commented Feb 6, 2023

/hold waiting for #809

Signed-off-by: Federico Di Pierro <[email protected]>

Co-authored-by: Andrea Terzolo <[email protected]>
Copy link
Contributor

@hbrueckner hbrueckner left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @FedeDP !
/approve

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 Feb 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 7, 2023

/unhold

@poiana poiana merged commit 0e11bb0 into master Feb 7, 2023
@poiana poiana deleted the fix/clang15_bpf branch February 7, 2023 16:24
@FedeDP FedeDP mentioned this pull request Feb 23, 2023
3 tasks
@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 23, 2023

/milestone 4.0.1+driver

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.

5 participants