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: <20190624154309.5ef3357b@cakuba.netronome.com>
Date:   Mon, 24 Jun 2019 15:43:09 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Andrey Ignatov <rdna@...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]

On Mon, 24 Jun 2019 22:16:02 +0000, Andrey Ignatov wrote:
> Jakub Kicinski <jakub.kicinski@...ronome.com> [Mon, 2019-06-24 14:51 -0700]:
> > 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 don't think these are equivalent.  BPF_F_QUERY_EFFECTIVE is a flag
for a syscall corresponding to a subcommand quite clearly.

> 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.

Agreed, TBH, but we can't change existing options, people may be using
them.  Let's drop the patch and make sure we're not making this mistake
again :)

Thanks!