netdev - Re: [RFC PATCH bpf-next 0/2] Improve prog array uref semantics

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAADnVQ+T2nSCA8Tcddh8eD27CnvD1E3vPK0zutDt8Boz7MURQA@mail.gmail.com>
Date: Sat, 16 Nov 2024 14:17:25 -0800
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Daniel Xu <dxu@...uu.xyz>
Cc: "bpf@...r.kernel.org" <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, 
	Network Development <netdev@...r.kernel.org>, Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [RFC PATCH bpf-next 0/2] Improve prog array uref semantics

On Tue, Oct 29, 2024 at 11:36 PM Daniel Xu <dxu@...uu.xyz> wrote:
>
> Hey Daniel,
>
> On Wed, Aug 23, 2023, at 9:08 AM, Daniel Xu wrote:
> > This patchset changes the behavior of TC and XDP hooks during attachment
> > such that any BPF_MAP_TYPE_PROG_ARRAY that the prog uses has an extra
> > uref taken.
> >
> > The goal behind this change is to try and prevent confusion for the
> > majority of use cases. The current behavior where when the last uref is
> > dropped the prog array map is emptied is quite confusing. Confusing
> > enough for there to be multiple references to it in ebpf-go [0][1].
> >
> > Completely solving the problem is difficult. As stated in c9da161c6517
> > ("bpf: fix clearing on persistent program array maps"), it is
> > difficult-to-impossible to walk the full dependency graph b/c it is too
> > dynamic.
> >
> > However in practice, I've found that all progs in a tailcall chain
> > share the same prog array map. Knowing that, if we take a uref on any
> > used prog array map when the program is attached, we can simplify the
> > majority use case and make it more ergonomic.

Are you proposing to inc map uref when prog is attached?

But that re-adds the circular dependency that uref concept is solving.
When prog is inserted into prog array prog refcnt is incremented.
So if prog also incremented uref. The user space can exit
but prog array and progs will stay there though nothing is using them.
I guess I'm missing the idea.

> >
> > I'll be the first to admit this is not a very clean solution. It does
> > not fully solve the problem. Nor does it make overall logic any simpler.
> > But I do think it makes a pretty big usability hole slightly smaller.
> >
> > I've done some basic testing using a repro program [3] I wrote to debug
> > the original issue that eventually led me to this patchset. If we wanna
> > move forward with this approach, I'll resend with selftests.
> >
> > [0]:
> > https://github.com/cilium/ebpf/blob/01ebd4c1e2b9f8b3dd4fd2382aa1092c3c9bfc9d/doc.go#L22-L24
> > [1]:
> > https://github.com/cilium/ebpf/blob/d1a52333f2c0fed085f8d742a5a3c164795d8492/collection.go#L320-L321
> > [2]: https://github.com/danobi/tc_tailcall_repro
>
> I recently remembered about this again. Was suggested I poke you in case you're interested.
> I looked again and I think this is kinda a neat hack. I probably won't have time to pick this back
> up either way.
>