linux-kernel - Re: [PATCH bpf-next v3 1/4] fprobe: use rhltable for fprobe_ip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CADxym3YxHOYxZxrdd4vkU_sj8p7VNW=HjLOo9nQgO3jkAAfjng@mail.gmail.com>
Date: Fri, 15 Aug 2025 14:23:58 +0800
From: Menglong Dong <menglong8.dong@...il.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: olsajiri@...il.com, rostedt@...dmis.org, mathieu.desnoyers@...icios.com, 
	hca@...ux.ibm.com, revest@...omium.org, linux-kernel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v3 1/4] fprobe: use rhltable for fprobe_ip_table

On Thu, Aug 14, 2025 at 11:40 PM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> On Thu, 31 Jul 2025 17:24:24 +0800
> Menglong Dong <menglong8.dong@...il.com> wrote:
>
> > For now, all the kernel functions who are hooked by the fprobe will be
> > added to the hash table "fprobe_ip_table". The key of it is the function
> > address, and the value of it is "struct fprobe_hlist_node".
> >
> > The budget of the hash table is FPROBE_IP_TABLE_SIZE, which is 256. And
> > this means the overhead of the hash table lookup will grow linearly if
> > the count of the functions in the fprobe more than 256. When we try to
> > hook all the kernel functions, the overhead will be huge.
> >
> > Therefore, replace the hash table with rhltable to reduce the overhead.
> >
>
> Hi Menglong,
>
> Thanks for update, I have just some nitpicks.
>
> > Signed-off-by: Menglong Dong <dongml2@...natelecom.cn>
> > ---
> > v3:
> > - some format optimization
> > - handle the error that returned from rhltable_insert in
> >   insert_fprobe_node
> > ---
> >  include/linux/fprobe.h |   3 +-
> >  kernel/trace/fprobe.c  | 154 +++++++++++++++++++++++------------------
> >  2 files changed, 90 insertions(+), 67 deletions(-)
> >
> > diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> > index 702099f08929..f5d8982392b9 100644
> > --- a/include/linux/fprobe.h
> > +++ b/include/linux/fprobe.h
> > @@ -7,6 +7,7 @@
> >  #include <linux/ftrace.h>
> >  #include <linux/rcupdate.h>
> >  #include <linux/refcount.h>
> > +#include <linux/rhashtable.h>
>
> nit: can you also include this header file in fprobe.c ?

OK!

>
> >  #include <linux/slab.h>
> >
[......]
> >
> >       mutex_lock(&fprobe_mutex);
> > -     for (i = 0; i < FPROBE_IP_TABLE_SIZE; i++)
> > -             fprobe_remove_node_in_module(mod, &fprobe_ip_table[i], &alist);
> > +     rhashtable_walk_enter(&fprobe_ip_table.ht, &iter);
>
> nit: Use rhltable_walk_enter() instead.

OK!

I'll send the V4 later with these nitpicks fixed.

Thanks!
Menglong Dong

>
> Others looks good to me.
>
> Thank you,
>
> > +     do {
> > +             rhashtable_walk_start(&iter);
> > +
> > +             while ((node = rhashtable_walk_next(&iter)) && !IS_ERR(node))
> > +                     fprobe_remove_node_in_module(mod, node, &alist);
> > +
> > +             rhashtable_walk_stop(&iter);
> > +     } while (node == ERR_PTR(-EAGAIN));
> > +     rhashtable_walk_exit(&iter);
> >
> >       if (alist.index < alist.size && alist.index > 0)
> >               ftrace_set_filter_ips(&fprobe_graph_ops.ops,
> > @@ -722,8 +729,16 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num)
> >       ret = fprobe_graph_add_ips(addrs, num);
> >       if (!ret) {
> >               add_fprobe_hash(fp);
> > -             for (i = 0; i < hlist_array->size; i++)
> > -                     insert_fprobe_node(&hlist_array->array[i]);
> > +             for (i = 0; i < hlist_array->size; i++) {
> > +                     ret = insert_fprobe_node(&hlist_array->array[i]);
> > +                     if (ret)
> > +                             break;
> > +             }
> > +             /* fallback on insert error */
> > +             if (ret) {
> > +                     for (i--; i >= 0; i--)
> > +                             delete_fprobe_node(&hlist_array->array[i]);
> > +             }
> >       }
> >       mutex_unlock(&fprobe_mutex);
> >
> > @@ -819,3 +834,10 @@ int unregister_fprobe(struct fprobe *fp)
> >       return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(unregister_fprobe);
> > +
> > +static int __init fprobe_initcall(void)
> > +{
> > +     rhltable_init(&fprobe_ip_table, &fprobe_rht_params);
> > +     return 0;
> > +}
> > +late_initcall(fprobe_initcall);
> > --
> > 2.50.1
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@...nel.org>