Date:   Thu, 16 Apr 2020 12:35:07 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Yonghong Song <yhs@...com>, Andrii Nakryiko <andriin@...com>,
        bpf <bpf@...r.kernel.org>, Martin KaFai Lau <kafai@...com>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [RFC PATCH bpf-next 05/16] bpf: create file or anonymous dumpers

On Thu, Apr 16, 2020 at 10:04 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, Apr 15, 2020 at 06:48:13PM -0700, Andrii Nakryiko wrote:
> > On Wed, Apr 15, 2020 at 9:46 AM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > On Tue, Apr 14, 2020 at 09:45:08PM -0700, Andrii Nakryiko wrote:
> > > > >
> > > > > > FD is closed, dumper program is detached and dumper is destroyed
> > > > > > (unless pinned in bpffs, just like with any other bpf_link.
> > > > > > 3. At this point bpf_dumper_link can be treated like a factory of
> > > > > > seq_files. We can add a new BPF_DUMPER_OPEN_FILE (all names are for
> > > > > > illustration purposes) command, that accepts dumper link FD and
> > > > > > returns a new seq_file FD, which can be read() normally (or, e.g.,
> > > > > > cat'ed from shell).
> > > > >
> > > > > In this case, link_query may not be accurate if a bpf_dumper_link
> > > > > is created but no corresponding bpf_dumper_open_file. What we really
> > > > > need to iterate through all dumper seq_file FDs.
> > > >
> > > > If the goal is to iterate all the open seq_files (i.e., bpfdump active
> > > > sessions), then bpf_link is clearly not the right approach. But I
> > > > thought we are talking about iterating all the bpfdump programs
> > > > attachments, not **sessions**, in which case bpf_link is exactly the
> > > > right approach.
> > >
> > > That's an important point. What is the pinned /sys/kernel/bpfdump/tasks/foo ?
> >
> > Assuming it's not a rhetorical question, foo is a pinned bpf_dumper
> > link (in my interpretation of all this).
>
> It wasn't rhetorical question and your answer is differrent from mine :)
> It's not a link. It's a template of seq_file. It's the same as
> $ stat /proc/net/ipv6_route
>   File: ‘/proc/net/ipv6_route’
>   Size: 0               Blocks: 0          IO Block: 1024   regular empty file

I don't see a contradiction. Pinning bpfdumper link in bpfdumpfs will
create a direntry and corresponding inode. That inode's i_private
field will contain a pointer to that link. When that direntry is
open()'ed, seq_file is going to be created. That seq_file will
probably need to take refcnt on underlying bpf_link and store it in
its private data. I was *not* implying that
/sys/kernel/bpfdump/tasks/foo is same as bpf_link pinned in bpffs,
which you can restore by doing BPF_OBJ_GET. It's more of a "backed by
bpf_link", if that helps to clarify.

But in your terminology, bpfdumper bpf_link *is* "a template of
seq_file", that I agree.

>
> > > Every time 'cat' opens it a new seq_file is created with new FD, right ?
> >
> > yes
> >
> > > Reading of that file can take infinite amount of time, since 'cat' can be
> > > paused in the middle.
> >
> > yep, correct (though most use case probably going to be very short-lived)
> >
> > > I think we're dealing with several different kinds of objects here.
> > > 1. "template" of seq_file that is seen with 'ls' in /sys/kernel/bpfdump/
> >
> > Let's clarify here again, because this can be interpreted differently.
> >
> > Are you talking about, e.g., /sys/fs/bpfdump/task directory that
> > defines what class of items should be iterated? Or you are talking
> > about named dumper: /sys/fs/bpfdump/task/my_dumper?
>
> the latter.
>
> >
> > If the former, I agree that it's not a link. If the latter, then
> > that's what we've been so far calling "a named bpfdumper". Which is
> > what I argue is a link, pinned in bpfdumpfs (*not bpffs*).
>
> It cannot be a link, since link is only a connection between
> kernel object and bpf prog.
> Whereas seq_file is such kernel object.

Not sure, but maybe that's where the misconnect is? seq_file instance
is derivative of bpf_prog + bpfdump provider. That couplin of bpf_prog
and provider is a link to me. That bpf_link can be used to "produce"
many independent seq_files then.

I do agree that link is a connection between prog and kernel object,
but I argue that "kernel object" in this case is bpfdumper provider
(e.g., what is backing /sys/fs/bpfdump/task), not any specific
seq_file.

>
> >
> > For named dumper:
> > 1. load bpfdump prog
> > 2. attach prog to bpfdump "provider" (/sys/fs/bpfdump/task), get
> > bpf_link anon FD back
> > 3. pin link in bpfdumpfs (e.g., /sys/fs/bpfdump/task/my_dumper)
> > 4. each open() of /sys/fs/bpfdump/task/my_dumper produces new
> > bpfdumper session/seq_file
> >
> > For anon dumper:
> > 1. load bpfdump prog
> > 2. attach prog to bpfdump "provider" (/sys/fs/bpfdump/task), get
> > bpf_link anon FD back
> > 3. give bpf_link FD to some new API (say, BPF_DUMP_NEW_SESSION or
> > whatever name) to create seq_file/bpfdumper session, which will create
> > FD that can be read(). One can do that many times, each time getting
> > its own bpfdumper session.
>
> I slept on it and still fundamentally disagree that seq_file + bpf_prog
> is a derivative of link. Or in OoO terms it's not a child class of bpf_link.
> seq_file is its own class that should contain bpf_link as one of its
> members, but it shouldn't be derived from 'class bpf_link'.

Referring to inheritance here doesn't seem necessary or helpful, I'd
rather not confuse and complicate all this further.

bpfdump provider/target + bpf_prog = bpf_link. bpf_link is "a factory"
of seq_files. That's it, no inheritance.


>
> In that sense Yonghong proposed api (raw_tp_open to create anon seq_file+prog
> and obj_pin to create a template of named seq_file+prog) are the best fit.
> Implementation wise his 'struct extra_priv_data' needs to include
> 'struct bpf_link' instead of 'struct bpf_prog *prog;' directly.
>
> So evertime 'cat' opens named seq_file there is bpf_link registered in IDR.
> Anon seq_file should have another bpf_link as well.

So that's where I disagree and don't see the point of having all those
short-lived bpf_links. cat opening seq_file doesn't create a bpf_link,
it creates a seq_file. If we want to associate some ID with it, it's
fine, but it's not a bpf_link ID (in my opinion, of course).

>
> My earlier suggestion to disallow get_fd_from_id for such links is wrong.
> It's fine to get an FD to such link, but it shouldn't prevent destruction

This is again some custom limitations and implementation, which again
I think is a sign of not ideal design for this. And now that we'll
have bpfdumper for iterate task/file, I also don't think that
everything should have ID to be "iterable" anymore.


> of seq_file. 'cat' will close named seq_file and 'struct extra_priv_data' class
> should do link_put. If some other process did get_fd_from_id then such link will
> become dangling. Just like removal of netdev will make dangling xdp links.

Powered by blists - more mailing lists