On 5/15/23 10:49 PM, Masami Hiramatsu (Google) wrote:
On Fri, 12 May 2023 07:29:02 -0700
Yonghong Song <yhs@xxxxxxxx> wrote:
On 5/11/23 10:53 PM, Ze Gao wrote:
Yes, Jiri. Thanks for pointing it out. It's true that not all probe
blacklisted functions should be banned from bpf_kprobe.
I tried some of them, and all kprobe blacklisted symbols I hooked
works fine except preempt_count_{sub, add}.
so the takeaway here is preempt_cout_{sub, add} must be rejected at
least for now since kprobe_multi_link_prog_run
( i.e., the fprobe handler) and rethook_trampoline_handler( i.e. the
rethook handler) calls preempt_cout_{sub, add}.
I'm considering providing a general fprobe_blacklist framework just
like what kprobe does to allow others to mark
functions used inside fprobe handler or rethook handler as NOFPROBE to
avoid potential stack recursion. But only after
I figure out how ftrace handles recursion problems currently and why
it fails in the case I ran into.
A fprobe_blacklist might make sense indeed as fprobe and kprobe are
quite different... Thanks for working on this.
No, I don't like fprobe_blacklist, because you can filter user given
function with <tracefs>/available_filter_functions :)
If the function is not listed there, you can not put fprobe on it.
IOW, kprobe_multi_link_prog_run only covers those functions. (white-list)
At the tooling side, it should check whether the probe is defined for
single function or multiple functions, and use kprobe-blacklist (single)
or available_filter_functions (multiple).
Thanks for clarification. So basically fprobe blacklist is similar to
fentry, not able to trace functions marked with notrace. So agree,
the checking scheme could be:
- user space to check available_filter_functions
- a few other tracable functions but may have recursion effect
handled by infrastructure for fprobe case. fentry case is already
covered by verifier to deny a few functions like preempt_count_sub
etc.
Thank you,
Thanks
Ze
On Thu, May 11, 2023 at 1:28 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
On Wed, May 10, 2023 at 07:13:58AM -0700, Yonghong Song wrote:
On 5/10/23 5:20 AM, Ze Gao wrote:
BPF_LINK_TYPE_KPROBE_MULTI attaches kprobe programs through fprobe,
however it does not takes those kprobe blacklisted into consideration,
which likely introduce recursive traps and blows up stacks.
this patch adds simple check and remove those are in kprobe_blacklist
from one fprobe during bpf_kprobe_multi_link_attach. And also
check_kprobe_address_safe is open for more future checks.
note that ftrace provides recursion detection mechanism, but for kprobe
only, we can directly reject those cases early without turning to ftrace.
Signed-off-by: Ze Gao <zegao@xxxxxxxxxxx>
---
kernel/trace/bpf_trace.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 9a050e36dc6c..44c68bc06bbd 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2764,6 +2764,37 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
return arr.mods_cnt;
}
+static inline int check_kprobe_address_safe(unsigned long addr)
+{
+ if (within_kprobe_blacklist(addr))
+ return -EINVAL;
+ else
+ return 0;
+}
+
+static int check_bpf_kprobe_addrs_safe(unsigned long *addrs, int num)
+{
+ int i, cnt;
+ char symname[KSYM_NAME_LEN];
+
+ for (i = 0; i < num; ++i) {
+ if (check_kprobe_address_safe((unsigned long)addrs[i])) {
+ lookup_symbol_name(addrs[i], symname);
+ pr_warn("bpf_kprobe: %s at %lx is blacklisted\n", symname, addrs[i]);
So user request cannot be fulfilled and a warning is issued and some
of user requests are discarded and the rest is proceeded. Does not
sound a good idea.
Maybe we should do filtering in user space, e.g., in libbpf, check
/sys/kernel/debug/kprobes/blacklist and return error
earlier? bpftrace/libbpf-tools/bcc-tools all do filtering before
requesting kprobe in the kernel.
also fprobe uses ftrace drectly without paths in kprobe, so I wonder
some of the kprobe blacklisted functions are actually safe
jirka
+ /* mark blacklisted symbol for remove */
+ addrs[i] = 0;
+ }
+ }
+
+ /* remove blacklisted symbol from addrs */
+ for (i = 0, cnt = 0; i < num; ++i) {
+ if (addrs[i])
+ addrs[cnt++] = addrs[i];
+ }
+
+ return cnt;
+}
+
int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
{
struct bpf_kprobe_multi_link *link = NULL;
@@ -2859,6 +2890,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
else
link->fp.entry_handler = kprobe_multi_link_handler;
+ cnt = check_bpf_kprobe_addrs_safe(addrs, cnt);
+ if (!cnt) {
+ err = -EINVAL;
+ goto error;
+ }
+
link->addrs = addrs;
link->cookies = cookies;
link->cnt = cnt;
