On Fri, May 12, 2023 at 07:29:02AM -0700, Yonghong Song 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}.
check BTF_SET_START(btf_id_deny) list for functions that we do not allow to
attach for tracing programs.. the direct ftrace interface used by trampolines
has likely similar limitations as fptrace_ops API used by fprobe
> >
> > 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.
+1
jirka
>
> >
> > 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;
