Implement override_return
#966
Conversation
fbs
force-pushed
the
720_override_return
branch
3 times, most recently
from
78a57a1
to
3c292be
Compare
src/ast/codegen_llvm.cpp
Outdated
| @@ -681,7 +681,19 @@ void CodegenLLVM::visit(Call &call) | |||
| } | |||
| expr_ = stackid; | |||
| } | |||
|
|
|||
| else if (call.func == "override_return") { | |||
There was a problem hiding this comment.
Could you move the { onto a separate line to match the surrounding code please? (here and below)
Nitpicky, but I think I might go through and reformat all the code one day to be consistent so I'm trying to minimise my future changes ;)
There was a problem hiding this comment.
Please do, I always mess it up. Would be good to get #639 done :)
tests/CMakeLists.txt
Outdated
| @@ -47,6 +47,9 @@ add_executable(bpftrace_test | |||
| ${CMAKE_SOURCE_DIR}/src/types.cpp | |||
| ${CMAKE_SOURCE_DIR}/src/utils.cpp | |||
| ${CMAKE_SOURCE_DIR}/src/disasm.cpp | |||
| ${CMAKE_SOURCE_DIR}/src/ast/semantic_analyser.cpp | |||
| ${CMAKE_SOURCE_DIR}/src/ast/codegen_llvm.cpp | |||
| ${CMAKE_SOURCE_DIR}/src/ast/irbuilderbpf.cpp | |||
There was a problem hiding this comment.
We're already doing target_link_libraries(bpftrace_test ast) - doesn't that cover what we need without this change?
There was a problem hiding this comment.
I wanted the semantic and codegen tests to always run, regardless of whether the kernel supports override_return or not.
The way cgroupid solves that is by putting the #ifdef in the codegen, but then you loose the option to fail nicely (with line number and source code) and instead have to abort out, and you cannot test codegen.
The "hack" I did here is to always define HAVE_OVERRIDE_RETURN in the test suite, regardless whether the kernel supports it. And define BPF_FUNC_override_return when it is not supported by the system. As those flags are used in the semantic and codegen phases those have to be recompiled for the tests but not the actual bpftrace build.
This hack got already merged in with the merge of signal but I'll happily fix that if we can find a better way to do this. I'm not entirely happy with it.
There was a problem hiding this comment.
Makes sense. I can't think of a better way
fbs
force-pushed
the
720_override_return
branch
2 times, most recently
from
7cb8cec
to
c75aea1
Compare
|
Fix the comments @ajor, thanks! #966 (comment) got a bit hidden now but it is still relevant. |
fbs
force-pushed
the
720_override_return
branch
4 times, most recently
from
a94c859
to
b90494a
Compare
fbs
force-pushed
the
720_override_return
branch
from
b90494a
to
ec0a3f2
Compare
This allows the user to modify the return value for a kprobe. It's use
is quite limited as the kernel puts heavy restrictions on it:
> This helper has security implications, and thus is subject to
> restrictions. It is only available if the kernel was compiled
> with the **CONFIG_BPF_KPROBE_OVERRIDE** configuration
> option, and in this case it only works on functions tagged with
> **ALLOW_ERROR_INJECTION** in the kernel code.
>
> Also, the helper is only available for the architectures having
> the CONFIG_FUNCTION_ERROR_INJECTION option. As of this writing,
> x86 architecture is the only one to support this feature.
Example usage:
```
bpftrace -e 'k:__x64_sys_openat /comm == "cat"/ { override_return(-4); }'
cat lex.yy.cc
cat: error while loading shared libraries: libc.so.6: cannot open shared object file: Error 4
```
A badly inserted override has the possibility to break your system. One
such example is `kprobe:do_openat { override_return(-1); }` which will
prevent your whole system from opening files.
fbs
force-pushed
the
720_override_return
branch
from
ec0a3f2
to
7945af8
Compare
|
I've rebased it onto master, the feature detecting should solve all CI issues it had. |
|
Sorry for the late comment: did we want it named override_return()? It's a long function name and includes an underscore, and doesn't sound like the other bpftrace functions (printf(), ksym(), str(), etc). What about just override() or return()? |
The implementation of this turned out quite straight forward.
I've chosen to skip testing whether the probed function can actually be overridden (using
/sys/kernel/debug/error_injection/list) as I didn't see that much value in it. It is not a function that will be used often and the people that do use it are probably aware of the limitations. If someone has a strong opinion about it I'll happily implement it however.This helper is not yet available in the CI so I've skipped the runtime tests.
Example:
Fixes #720