linux-kernel - Re: [PATCH bpf-next 1/4] fprobe: use rhashtable

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADxym3YdOkcr-Jr-NkDUu4HqU7x_5Rwgiz6PS32BktGuLcBJRw@mail.gmail.com>
Date: Mon, 28 Jul 2025 22:44:07 +0800
From: Menglong Dong <menglong8.dong@...il.com>
To: Jiri Olsa <olsajiri@...il.com>
Cc: alexei.starovoitov@...il.com, mhiramat@...nel.org, 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 1/4] fprobe: use rhashtable

On Mon, Jul 28, 2025 at 9:13 PM Jiri Olsa <olsajiri@...il.com> wrote:
>
> On Mon, Jul 28, 2025 at 12:12:48PM +0800, Menglong Dong wrote:
>
> SNIP
>
> > +static const struct rhashtable_params fprobe_rht_params = {
> > +     .head_offset            = offsetof(struct fprobe_hlist_node, hlist),
> > +     .key_offset             = offsetof(struct fprobe_hlist_node, addr),
> > +     .key_len                = sizeof_field(struct fprobe_hlist_node, addr),
> > +     .hashfn                 = fprobe_node_hashfn,
> > +     .obj_hashfn             = fprobe_node_obj_hashfn,
> > +     .obj_cmpfn              = fprobe_node_cmp,
> > +     .automatic_shrinking    = true,
> > +};
> >
> >  /* Node insertion and deletion requires the fprobe_mutex */
> >  static void insert_fprobe_node(struct fprobe_hlist_node *node)
> >  {
> > -     unsigned long ip = node->addr;
> > -     struct fprobe_hlist_node *next;
> > -     struct hlist_head *head;
> > -
> >       lockdep_assert_held(&fprobe_mutex);
> >
> > -     next = find_first_fprobe_node(ip);
> > -     if (next) {
> > -             hlist_add_before_rcu(&node->hlist, &next->hlist);
> > -             return;
> > -     }
> > -     head = &fprobe_ip_table[hash_ptr((void *)ip, FPROBE_IP_HASH_BITS)];
> > -     hlist_add_head_rcu(&node->hlist, head);
> > +     rhashtable_insert_fast(&fprobe_ip_table, &node->hlist,
> > +                            fprobe_rht_params);
>
> onw that rhashtable_insert_fast can fail, I think insert_fprobe_node
> needs to be able to fail as well

You are right, the insert_fprobe_node should return a error and
be handled properly.

>
> >  }
> >
> >  /* Return true if there are synonims */
> > @@ -92,9 +93,11 @@ static bool delete_fprobe_node(struct fprobe_hlist_node *node)
> >       /* Avoid double deleting */
> >       if (READ_ONCE(node->fp) != NULL) {
> >               WRITE_ONCE(node->fp, NULL);
> > -             hlist_del_rcu(&node->hlist);
> > +             rhashtable_remove_fast(&fprobe_ip_table, &node->hlist,
> > +                                    fprobe_rht_params);
>
> I guess this one can't fail in here.. ?

Yeah, the only failure is the entry doesn't exist in the hash
table.

BTW, the usage of rhltable is similar to rhashtable, and the
comment here is valid in the V2 too.

Thanks!
Menglong Dong

>
> jirka
>
> >       }
> > -     return !!find_first_fprobe_node(node->addr);
> > +     return !!rhashtable_lookup_fast(&fprobe_ip_table, &node->addr,
> > +                                     fprobe_rht_params);
> >  }
> >
>
> SNIP