netdev - Re: [PATCH bpf-next v3 1/8] bpf: Add generic attach/detach/query API for multi-progs

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: <c378dd03-2950-ab2f-26bb-65e2757433b7@iogearbox.net>
Date: Mon, 10 Jul 2023 16:26:20 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Stanislav Fomichev <sdf@...gle.com>
Cc: ast@...nel.org, andrii@...nel.org, martin.lau@...ux.dev,
 razor@...ckwall.org, john.fastabend@...il.com, kuba@...nel.org,
 dxu@...uu.xyz, joe@...ium.io, toke@...nel.org, davem@...emloft.net,
 bpf@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v3 1/8] bpf: Add generic attach/detach/query API
 for multi-progs

On 7/10/23 9:42 AM, Daniel Borkmann wrote:
> On 7/7/23 11:27 PM, Stanislav Fomichev wrote:
>> On 07/07, Daniel Borkmann wrote:
> [...]
>>> +static inline struct bpf_mprog_entry *
>>> +bpf_mprog_create(const size_t size, const off_t off)
>>> +{
>>> +    struct bpf_mprog_bundle *bundle;
>>> +    void *ptr;
>>> +
>>> +    BUILD_BUG_ON(size < sizeof(*bundle) + off);
>>> +    BUILD_BUG_ON(sizeof(bundle->a.fp_items[0]) > sizeof(u64));
>>> +    BUILD_BUG_ON(ARRAY_SIZE(bundle->a.fp_items) !=
>>> +             ARRAY_SIZE(bundle->cp_items));
>>> +
>>> +    ptr = kzalloc(size, GFP_KERNEL);
>>> +    if (ptr) {
>>> +        bundle = ptr + off;
>>> +        atomic64_set(&bundle->revision, 1);
>>> +        bundle->off = off;
>>> +        bundle->a.parent = bundle;
>>> +        bundle->b.parent = bundle;
>>> +        return &bundle->a;
>>> +    }
>>> +    return NULL;
>>> +}
>>> +
>>> +void bpf_mprog_free_rcu(struct rcu_head *rcu);
>>> +
>>> +static inline void bpf_mprog_free(struct bpf_mprog_entry *entry)
>>> +{
>>> +    struct bpf_mprog_bundle *bundle = entry->parent;
>>> +
>>> +    call_rcu(&bundle->rcu, bpf_mprog_free_rcu);
>>> +}
>>
>> Any reason we're doing allocation here? Why not do
>> bpf_mprog_init(struct bpf_mprog_bundle *) instead that simply initializes
>> the fields? Then we can move allocation/free part to the caller (tcx) along
>> with rcu_head.
>> Feels like it would be a bit more conventional/readable? bpf_mprog_free{,_rcu}
>> will also become tcx_free{,_rcu}..
>>
>> I guess current approach works, but it took me awhile to figure it out..
>> (maybe it's just me)
> 
> I found this approach quite useful for tcx case since we only fetch the
> bpf_mprog_entry for tcx_link_prog_attach et al, but I can take a look to
> see if this looks better and if it does I'll include it.

Ok, I moved this into tcx and only left bpf_mprog_bundle_init() in mprog.
Looks better indeed, thanks Stan!