netdev - Re: [PATCH bpf-next v1 2/3] selftests/bpf: Add multi_st

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: <CAMB2axNQ3kXp-LbRsqc_ob61-a_YArgt-LSZMuMke_O0B=-exw@mail.gmail.com>
Date: Tue, 5 Aug 2025 08:27:03 -0700
From: Amery Hung <ameryhung@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: netdev@...r.kernel.org, alexei.starovoitov@...il.com, andrii@...nel.org, 
	daniel@...earbox.net, tj@...nel.org, memxor@...il.com, martin.lau@...nel.org, 
	kernel-team@...a.com, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v1 2/3] selftests/bpf: Add multi_st_ops that
 supports multiple instances

On Mon, Aug 4, 2025 at 4:43 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 7/31/25 2:09 PM, Amery Hung wrote:
> > +static int multi_st_ops_reg(void *kdata, struct bpf_link *link)
> > +{
> > +     struct bpf_testmod_multi_st_ops *st_ops =
> > +             (struct bpf_testmod_multi_st_ops *)kdata;
> > +     struct bpf_map *map;
> > +     unsigned long flags;
> > +     int err = 0;
> > +
> > +     map = bpf_struct_ops_get(kdata);
>
> The bpf_struct_ops_get returns a map pointer and also inc_not_zero() the map
> which we know it won't fail at this point, so no check is needed.
>
> > +
> > +     spin_lock_irqsave(&multi_st_ops_lock, flags);
> > +     if (multi_st_ops_find_nolock(map->id)) {
> > +             pr_err("multi_st_ops(id:%d) has already been registered\n", map->id);
> > +             err = -EEXIST;
> > +             goto unlock;
> > +     }
> > +
> > +     st_ops->id = map->id;
> > +     hlist_add_head(&st_ops->node, &multi_st_ops_list);
> > +unlock:
> > +     bpf_struct_ops_put(kdata);
>
> To get an id, it needs a get and an immediate put. No concern on the performance
>   but just feels not easy to use. e.g. For the subsystem supporting link_update,
> it will need to do this get/put twice. One on the old kdata and another on the
> new kdata. Take a look at the bpf_struct_ops_map_link_update().
>
> To create a id->struct_ops mapping, the subsystem needs neither the map pointer
> nor incrementing the map refcnt. How about create a new helper to only return
> the id of the kdata?
>

Make sense. I will create a new helper as you suggested. I was
thinking about repurposing existing helpers. There is indeed no need
to increase the refcount as kdata is protected under lock during
map_update, link_create and link_update.

Thanks,
Amery

> Uncompiled code:
>
> u32 bpf_struct_ops_id(const void *kdata)
> {
>         struct bpf_struct_ops_value *kvalue;
>         struct bpf_struct_ops_map *st_map;
>
>         kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
>         st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
>
>         return st_map->map.id;
> }
>
> > +     spin_unlock_irqrestore(&multi_st_ops_lock, flags);
> > +
> > +     return err;
> > +}
> > +
> > +static void multi_st_ops_unreg(void *kdata, struct bpf_link *link)
> > +{
> > +     struct bpf_testmod_multi_st_ops *st_ops;
> > +     struct bpf_map *map;
> > +     unsigned long flags;
> > +
> > +     map = bpf_struct_ops_get(kdata);
> > +
> > +     spin_lock_irqsave(&multi_st_ops_lock, flags);
> > +     st_ops = multi_st_ops_find_nolock(map->id);
> > +     if (st_ops)
> > +             hlist_del(&st_ops->node);
> > +     spin_unlock_irqrestore(&multi_st_ops_lock, flags);
> > +
> > +     bpf_struct_ops_put(kdata);
> > +}
>