On Fri, Apr 10, 2020 at 4:41 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 4/10/20 3:51 PM, Andrii Nakryiko wrote:
> > On Wed, Apr 8, 2020 at 4:26 PM Yonghong Song <yhs@xxxxxx> wrote:
> >>
> >> Given a loaded dumper bpf program, which already
> >> knows which target it should bind to, there
> >> two ways to create a dumper:
> >> - a file based dumper under hierarchy of
> >> /sys/kernel/bpfdump/ which uses can
> >> "cat" to print out the output.
> >> - an anonymous dumper which user application
> >> can "read" the dumping output.
> >>
> >> For file based dumper, BPF_OBJ_PIN syscall interface
> >> is used. For anonymous dumper, BPF_PROG_ATTACH
> >> syscall interface is used.
> >>
> >> To facilitate target seq_ops->show() to get the
> >> bpf program easily, dumper creation increased
> >> the target-provided seq_file private data size
> >> so bpf program pointer is also stored in seq_file
> >> private data.
> >>
> >> Further, a seq_num which represents how many
> >> bpf_dump_get_prog() has been called is also
> >> available to the target seq_ops->show().
> >> Such information can be used to e.g., print
> >> banner before printing out actual data.
> >>
> >> Note the seq_num does not represent the num
> >> of unique kernel objects the bpf program has
> >> seen. But it should be a good approximate.
> >>
> >> A target feature BPF_DUMP_SEQ_NET_PRIVATE
> >> is implemented specifically useful for
> >> net based dumpers. It sets net namespace
> >> as the current process net namespace.
> >> This avoids changing existing net seq_ops
> >> in order to retrieve net namespace from
> >> the seq_file pointer.
> >>
> >> For open dumper files, anonymous or not, the
> >> fdinfo will show the target and prog_id associated
> >> with that file descriptor. For dumper file itself,
> >> a kernel interface will be provided to retrieve the
> >> prog_id in one of the later patches.
> >>
> >> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> >> ---
> >> include/linux/bpf.h | 5 +
> >> include/uapi/linux/bpf.h | 6 +-
> >> kernel/bpf/dump.c | 338 ++++++++++++++++++++++++++++++++-
> >> kernel/bpf/syscall.c | 11 +-
> >> tools/include/uapi/linux/bpf.h | 6 +-
> >> 5 files changed, 362 insertions(+), 4 deletions(-)
> >>
> >
> > [...]
> >
> >>
> >> +struct dumper_inode_info {
> >> + struct bpfdump_target_info *tinfo;
> >> + struct bpf_prog *prog;
> >> +};
> >> +
> >> +struct dumper_info {
> >> + struct list_head list;
> >> + /* file to identify an anon dumper,
> >> + * dentry to identify a file dumper.
> >> + */
> >> + union {
> >> + struct file *file;
> >> + struct dentry *dentry;
> >> + };
> >> + struct bpfdump_target_info *tinfo;
> >> + struct bpf_prog *prog;
> >> +};
> >
> > This is essentially a bpf_link. Why not do it as a bpf_link from the
> > get go? Instead of having all this duplication for anonymous and
>
> This is a good question. Maybe part of bpf-link can be used and
> I have to implement others. I will check.
>
> > pinned dumpers, it would always be a bpf_link-based dumper, but for
> > those pinned bpf_link itself is going to be pinned. You also get a
> > benefit of being able to list all dumpers through existing bpf_link
> > API (also see my RFC patches with bpf_link_prime/bpf_link_settle,
> > which makes using bpf_link safe and simple).
>
> Agree. Alternative is to use BPF_OBJ_GET_INFO_BY_FD to query individual
> dumper as directory tree walk can be easily done at user space.
But BPF_OBJ_GET_INFO_BY_FD won't work well for anonymous dumpers,
because it's not so easy to iterate over them (possible, but not
easy)?
>
>
> >
> > [...]
> >
> >> +
> >> +static void anon_dumper_show_fdinfo(struct seq_file *m, struct file *filp)
> >> +{
> >> + struct dumper_info *dinfo;
> >> +
> >> + mutex_lock(&anon_dumpers.dumper_mutex);
> >> + list_for_each_entry(dinfo, &anon_dumpers.dumpers, list) {
> >
> > this (and few other places where you search in a loop) would also be
> > simplified, because struct file* would point to bpf_dumper_link, which
> > then would have a pointer to bpf_prog, dentry (if pinned), etc. No
> > searching at all.
>
> This is a reason for this. the same as bpflink, bpfdump already has
> the full information about file, inode, etc.
I think (if I understand what you are saying), this is my point. What
you have in struct dumper_info is already a custom bpf_link. You are
just missing `struct bpf_link link;` field there and plugging it into
overall bpf_link infrastructure (bpf_link__init + bpf_link__prime +
bpf_link__settle, from my RFC) to gain benefits of bpf_link infra.
> The file private_data actually points to seq_file. The seq_file private
> data is used in the target. That is exactly why we try to have this
> mapping to keep track. bpf_link won't help here.
I need to go and re-read all the code again carefully with who stores
what in their private_data field...
>
> >
> >> + if (dinfo->file == filp) {
> >> + seq_printf(m, "target:\t%s\n"
> >> + "prog_id:\t%u\n",
> >> + dinfo->tinfo->target,
> >> + dinfo->prog->aux->id);
> >> + break;
> >> + }
> >> + }
> >> + mutex_unlock(&anon_dumpers.dumper_mutex);
> >> +}
> >> +
> >> +#endif
> >> +
> >
> > [...]
> >
