netdev - Re: [PATCH bpf-next v4 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: <20230711002320.bp4mlb4at45vkrqt@MacBook-Pro-8.local>
Date: Mon, 10 Jul 2023 17:23:20 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: ast@...nel.org, andrii@...nel.org, martin.lau@...ux.dev,
	razor@...ckwall.org, sdf@...gle.com, 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 v4 1/8] bpf: Add generic attach/detach/query API
 for multi-progs

On Mon, Jul 10, 2023 at 10:12:11PM +0200, Daniel Borkmann wrote:
> + *
> + *   struct bpf_mprog_entry *entry, *peer;
> + *   int ret;
> + *
> + *   // bpf_mprog user-side lock
> + *   // fetch active @entry from attach location
> + *   [...]
> + *   ret = bpf_mprog_attach(entry, [...]);
> + *   if (ret >= 0) {
> + *       peer = bpf_mprog_peer(entry);
> + *       if (bpf_mprog_swap_entries(ret))
> + *           // swap @entry to @peer at attach location
> + *       bpf_mprog_commit(entry);
> + *       ret = 0;
> + *   } else {
> + *       // error path, bail out, propagate @ret
> + *   }
> + *   // bpf_mprog user-side unlock
> + *
> + *  Detach case:
> + *
> + *   struct bpf_mprog_entry *entry, *peer;
> + *   bool release;
> + *   int ret;
> + *
> + *   // bpf_mprog user-side lock
> + *   // fetch active @entry from attach location
> + *   [...]
> + *   ret = bpf_mprog_detach(entry, [...]);
> + *   if (ret >= 0) {
> + *       release = ret == BPF_MPROG_FREE;
> + *       peer = release ? NULL : bpf_mprog_peer(entry);
> + *       if (bpf_mprog_swap_entries(ret))
> + *           // swap @entry to @peer at attach location
> + *       bpf_mprog_commit(entry);
> + *       if (release)
> + *           // free bpf_mprog_bundle
> + *       ret = 0;
> + *   } else {
> + *       // error path, bail out, propagate @ret
> + *   }
> + *   // bpf_mprog user-side unlock

Thanks for the doc. It helped a lot.
And when it's contained like this it's easier to discuss api.
It seems bpf_mprog_swap_entries() is trying to abstract the error code
away, but BPF_MPROG_FREE leaks out and tcx_entry_needs_release()
captures it with extra miniq_active twist, which I don't understand yet.
bpf_mprog_peer() is also leaking a bit of implementation detail.
Can we abstract it further, like:

ret = bpf_mprog_detach(entry, [...], &new_entry);
if (ret >= 0) {
   if (entry != new_entry)
     // swap @entry to @new_entry at attach location
   bpf_mprog_commit(entry);
   if (!new_entry)
     // free bpf_mprog_bundle
}
and make bpf_mprog_peer internal to mprog. It will also allow removing
BPF_MPROG_FREE vs SWAP distinction. peer is hidden.
   if (entry != new_entry)
      // update
also will be easier to read inside tcx code without looking into mprog details.