netdev - Re: [RFC bpf-next] bpf: allow users to opt out of releasing objects when urefs are gone

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] [day] [month] [year] [list]

Date:   Thu, 24 Jan 2019 11:28:53 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     alexei.starovoitov@...il.com, yhs@...com, sdf@...gle.com,
        brouer@...hat.com, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [RFC bpf-next] bpf: allow users to opt out of releasing objects
 when urefs are gone

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.kicinski@...ronome.com>
> > Reviewed-by: Quentin Monnet <quentin.monnet@...ronome.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 :)