netdev - Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20190624221558.GA41600@rdna-mbp.dhcp.thefacebook.com>
Date:   Mon, 24 Jun 2019 22:16:02 +0000
From:   Andrey Ignatov <rdna@...com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     Daniel Borkmann <daniel@...earbox.net>,
        Takshak Chahande <ctakshak@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        Kernel Team <Kernel-team@...com>,
        Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in
 bpftool cgroup [show|tree]

Jakub Kicinski <jakub.kicinski@...ronome.com> [Mon, 2019-06-24 14:51 -0700]:
> On Mon, 24 Jun 2019 16:22:25 +0200, Daniel Borkmann wrote:
> > On 06/22/2019 12:33 AM, Takshak Chahande wrote:
> > > With different bpf attach_flags available to attach bpf programs specially
> > > with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective
> > > bpf-programs available to any sub-cgroups really needs to be available for
> > > easy debugging.
> > > 
> > > Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached
> > > bpf-programs to a cgroup but also the inherited ones from parent cgroup.
> > > 
> > > So "-e" option is introduced to use BPF_F_QUERY_EFFECTIVE query flag here to
> > > list all the effective bpf-programs available for execution at a specified
> > > cgroup.
> > > 
> > > Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf:
> > >   # ./test_cgroup_attach
> > > 
> > > With old bpftool (without -e option):
> > > 
> > >   # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
> > >   ID       AttachType      AttachFlags     Name
> > >   271      egress          multi           pkt_cntr_1
> > >   272      egress          multi           pkt_cntr_2
> > > 
> > >   Attached new program pkt_cntr_4 in cg2 gives following:
> > > 
> > >   # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2
> > >   ID       AttachType      AttachFlags     Name
> > >   273      egress          override        pkt_cntr_4
> > > 
> > > And with new "-e" option it shows all effective programs for cg2:
> > > 
> > >   # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2
> > >   ID       AttachType      AttachFlags     Name
> > >   273      egress          override        pkt_cntr_4
> > >   271      egress          override        pkt_cntr_1
> > >   272      egress          override        pkt_cntr_2
> > > 
> > > Signed-off-by: Takshak Chahande <ctakshak@...com>
> > > Acked-by: Andrey Ignatov <rdna@...com>  
> > 
> > Applied, thanks!
> 
> This is a cgroup-specific flag, right?  It should be a parameter 
> to cgroup show, not a global flag.  Can we please drop this patch 
> from the tree?

Hey Jakub,

I had same thought about cgroup-specific flag while reviewing the patch,
but then found out that all flags in bpftool are now global, no mater if
they're sub-command-specific or not.

For example, --mapcompat is used only in prog-subcommand, but the option
is global; --bpffs is used in prog- and map-subcommands, but the option
is global as well, etc (there are more examples).

I agree that limiting the scope of an option is a good idea in the long
term and it'd be great to rework all existing options to be available
only for corresponding sub-commands, but I don't see how the new `-e`
options is different from existing options and why it should be dropped.

Or you were counfused by the example in the commit log? Since all
options are global they can be specific anywhere on the command line,
i.e. instead of:
  # bpftool -e cgroup show /path/to/cgroup

it can be:
  # bpftool cgroup show -e /path/to/cgroup


-- 
Andrey Ignatov