On Thu, 24 Jan 2019 12:17:27 +0100, Daniel Borkmann wrote:
> On 01/21/2019 09:10 PM, Jakub Kicinski wrote:
> > Commit c9da161c6517 ("bpf: fix clearing on persistent program array maps")
> > added protection against dependency loops between programs and maps leading
> > to zombie objects which would never be freed. FD maps are flushed when
> > last user reference is gone.
> >
> > This is confusing to users of bpftool, as updates to tail call maps have
> > no effect, unless those maps are pinned (or open by another entity).
>
> Probably would make sense to only allow updates by providing the pinned
> map path in case of tail call map (and e.g. not by id)?
Hm, yes, or at least print a warning.
> > Today, flushing should not be a requirement for privileged user, as root
> > has access to GET_FD_BY_ID commends, and therefore can manually flush
> > the maps. Add a flag to opt out of automatic flushing.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
> > Reviewed-by: Quentin Monnet <quentin.mon...@netronome.com>
> > ---
> > Hi!
> >
> > I wonder what the reactions to this (untested) patch would be? I don't
> > really care strongly, but perhaps others experienced a similar issue?
>
> My main worry with this is that this might lead to hard to debug memory
> consumption issues, for example, once apps start to make wider use of
> this flag (and even worse would load maps with BPF progs with circular
> references), then they won't be cleaned up on detach and keep holding
> potentially huge amount of kernel memory. It's true that similar issues
> would happen when long running app leaks tail call map fd or the map
> is a stale leftover node in bpf fs, but it feels there's still better
> tooling out of user space for tracing back their correlation (e.g. fds,
> file access to pinned entry) and given the fact that then there's a
> guaranteed flush once urefs are fully dropped. Not having it and scanning
> through GET_FD_BY_ID range for such cases to determine a stale tail call
> map, its potential origin, and breaking up dependencies seems much harder
> (or at least there's proper tooling missing for it today).
Makes sense, I'll toss the patch with clear conscience :)
