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

Bug in Aya log #808

Closed
qjerome opened this issue Oct 9, 2023 · 8 comments
Closed

Bug in Aya log #808

qjerome opened this issue Oct 9, 2023 · 8 comments

Comments

@qjerome
Copy link
Contributor

qjerome commented Oct 9, 2023 •
edited
Loading

The verifier fails at loading BPF program because of (supposed) DisplayHint::write function.

Error: the BPF_PROG_LOAD syscall failed. Verifier output: Validating write() func#1...
Global function write() doesn't return scalar. Only those are supported.
verification time 40 usec
stack depth 0+0
processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

This bug appears only under some conditions:

  • bpf-linker branch feature/fix-diis used
  • debug-info = 2 is present in eBPF's Cargo.toml file

Supporting files:

A repro project can be found here: https://github.com/0xrawsec/poc-bug-aya-log
IMPORTANT: a compiled version of bpf-linker is provided with the project and is located under the eBPF's directory.
@tamird
Copy link
Member

tamird commented Oct 9, 2023

Can you reproduce this with a test in this repo? I understand if bpf-linker needs to be built from the feature/fix-di branch.

@qjerome
Copy link
Contributor Author

qjerome commented Oct 9, 2023

It is mandatory to have a bpf-linker feature/fix-di build and to be built with debug-info = 2. I am not familiar with the Aya integration tests, and I don't know whether those conditions can be met in the current implementation.
If it is possible, please let me know how to do it.

@tamird
Copy link
Member

tamird commented Oct 9, 2023

debug-info = 2 is easy to do - just put it in the appropriate manifest or .cargo/config.toml. Having a custom bpf-linker is not easy to do, but just leave that part out. The tests use whatever bpf-linker is installed on the machine, if you have a repro that requires me to install bpf-linker from the DI branch, that is good enough.

@qjerome qjerome mentioned this issue Oct 10, 2023
Closed
@qjerome
Copy link
Contributor Author

qjerome commented Oct 10, 2023

Have you managed to reproduce the issue ?
I actually shipped a compiled version of bpf-linker (nothing else than latest build of feature/fix-di build commit=60ce107a15333946ba0116297af30c9fd27ae6f4 ) with the project to be in the exact same conditions I was when the bug occurred.

@tamird
Copy link
Member

tamird commented Oct 10, 2023

Sorry, did you write a test that I missed?

@qjerome
Copy link
Contributor Author

qjerome commented Oct 10, 2023

I guess I miss-understood your last post. I thought you meant the repro project as it is was fine !
Should I write the test in: https://github.com/aya-rs/aya/tree/main/test/integration-ebpf/src ?

@tamird
Copy link
Member

tamird commented Oct 10, 2023

Yes. You can adapt https://github.com/aya-rs/aya/blob/main/test/integration-ebpf/src/log.rs if you like.

qjerome added a commit to 0xrawsec/aya-dev that referenced this issue Oct 10, 2023
@qjerome
repro issue aya-rs#808
ca9dd66 
Signed-off-by: Quentin JEROME <qjerome@users.noreply.github.com>
@qjerome qjerome mentioned this issue Oct 10, 2023
Closed
@qjerome
Copy link
Contributor Author

qjerome commented Oct 10, 2023

Test implemented in #810

qjerome added a commit to 0xrawsec/aya-dev that referenced this issue Oct 10, 2023
@qjerome
repro issue aya-rs#808
5c90f48 
Signed-off-by: Quentin JEROME <qjerome@users.noreply.github.com>
qjerome added a commit to 0xrawsec/aya-dev that referenced this issue Oct 12, 2023
@qjerome
repro issue aya-rs#808
024def8 
Signed-off-by: Quentin JEROME <qjerome@users.noreply.github.com>
alessandrod added a commit to alessandrod/bpf-linker that referenced this issue Jan 28, 2024
@alessandrod
di: btf: only exported symbols (bpf programs) should have linkage=global
eca03f1 
We only want exported symbols (programs marked as #[no_mangle]) to have
linkage=global. This avoid issues like:

    Global function write() doesn't return scalar. Only those are supported.
    verification time 18 usec
    stack depth 0+0
    ...

The error above happens when aya-log's WriteBuf::write doesn't get
inlined, and ends up having linkage=global. Global functions are
verified independently from their callers, so the verifier has less
context, and as a result globals are harder to verify.

In clang one makes a function global by not making it `static`. That
model doesn't work for rust, where all symbols exported by dependencies
are non-static, and therefore end up being emitted as global.

This commit implements a custom pass which marks functions as
linkage=static unless they've been explicitly exported.

Fixes aya-rs/aya#808
@alessandrod alessandrod mentioned this issue Jan 28, 2024
Merged
alessandrod added a commit to alessandrod/bpf-linker that referenced this issue Jan 29, 2024
@alessandrod
di: btf: only exported symbols (bpf programs) should have linkage=global
23fda89 
We only want exported symbols (programs marked as #[no_mangle]) to have
linkage=global. This avoid issues like:

    Global function write() doesn't return scalar. Only those are supported.
    verification time 18 usec
    stack depth 0+0
    ...

The error above happens when aya-log's WriteBuf::write doesn't get
inlined, and ends up having linkage=global. Global functions are
verified independently from their callers, so the verifier has less
context, and as a result globals are harder to verify.

In clang one makes a function global by not making it `static`. That
model doesn't work for rust, where all symbols exported by dependencies
are non-static, and therefore end up being emitted as global.

This commit implements a custom pass which marks functions as
linkage=static unless they've been explicitly exported.

Fixes aya-rs/aya#808
alessandrod added a commit to alessandrod/bpf-linker that referenced this issue Jan 29, 2024
@alessandrod
di: btf: only exported symbols (bpf programs) should have linkage=global
94de1fa 
We only want exported symbols (programs marked as #[no_mangle]) to have
linkage=global. This avoid issues like:

    Global function write() doesn't return scalar. Only those are supported.
    verification time 18 usec
    stack depth 0+0
    ...

The error above happens when aya-log's WriteBuf::write doesn't get
inlined, and ends up having linkage=global. Global functions are
verified independently from their callers, so the verifier has less
context, and as a result globals are harder to verify.

In clang one makes a function global by not making it `static`. That
model doesn't work for rust, where all symbols exported by dependencies
are non-static, and therefore end up being emitted as global.

This commit implements a custom pass which marks functions as
linkage=static unless they've been explicitly exported.

Fixes aya-rs/aya#808
alessandrod added a commit to alessandrod/bpf-linker that referenced this issue Jan 30, 2024
@alessandrod
di: btf: only exported symbols (bpf programs) should have linkage=global
3d6b141 
We only want exported symbols (programs marked as #[no_mangle]) to have
linkage=global. This avoid issues like:

    Global function write() doesn't return scalar. Only those are supported.
    verification time 18 usec
    stack depth 0+0
    ...

The error above happens when aya-log's WriteBuf::write doesn't get
inlined, and ends up having linkage=global. Global functions are
verified independently from their callers, so the verifier has less
context, and as a result globals are harder to verify.

In clang one makes a function global by not making it `static`. That
model doesn't work for rust, where all symbols exported by dependencies
are non-static, and therefore end up being emitted as global.

This commit implements a custom pass which marks functions as
linkage=static unless they've been explicitly exported.

Fixes aya-rs/aya#808
vadorovsky pushed a commit to vadorovsky/bpf-linker that referenced this issue Feb 8, 2024
@alessandrod @vadorovsky
di: btf: only exported symbols (bpf programs) should have linkage=global
b584e09 
We only want exported symbols (programs marked as #[no_mangle]) to have
linkage=global. This avoid issues like:

    Global function write() doesn't return scalar. Only those are supported.
    verification time 18 usec
    stack depth 0+0
    ...

The error above happens when aya-log's WriteBuf::write doesn't get
inlined, and ends up having linkage=global. Global functions are
verified independently from their callers, so the verifier has less
context, and as a result globals are harder to verify.

In clang one makes a function global by not making it `static`. That
model doesn't work for rust, where all symbols exported by dependencies
are non-static, and therefore end up being emitted as global.

This commit implements a custom pass which marks functions as
linkage=static unless they've been explicitly exported.

Fixes aya-rs/aya#808
vadorovsky pushed a commit to vadorovsky/bpf-linker that referenced this issue Feb 13, 2024
@alessandrod @vadorovsky
di: btf: only exported symbols (bpf programs) should have linkage=global
65a834a 
We only want exported symbols (programs marked as #[no_mangle]) to have
linkage=global. This avoid issues like:

    Global function write() doesn't return scalar. Only those are supported.
    verification time 18 usec
    stack depth 0+0
    ...

The error above happens when aya-log's WriteBuf::write doesn't get
inlined, and ends up having linkage=global. Global functions are
verified independently from their callers, so the verifier has less
context, and as a result globals are harder to verify.

In clang one makes a function global by not making it `static`. That
model doesn't work for rust, where all symbols exported by dependencies
are non-static, and therefore end up being emitted as global.

This commit implements a custom pass which marks functions as
linkage=static unless they've been explicitly exported.

Fixes aya-rs/aya#808
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tamird @qjerome