linux-kernel - Re: [PATCH net-next 0/5] bpftool: cgroup bpf operations

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: <20171130190454.26723962@cakuba.netronome.com>
Date:   Thu, 30 Nov 2017 19:04:54 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Roman Gushchin <guro@...com>
Cc:     <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <kernel-team@...com>, <ast@...nel.org>, <daniel@...earbox.net>,
        <kafai@...com>
Subject: Re: [PATCH net-next 0/5] bpftool: cgroup bpf operations

Hi Roman!

On Thu, 30 Nov 2017 13:42:57 +0000, Roman Gushchin wrote:
> This patchset adds basic cgroup bpf operations to bpftool.
> 
> Right now there is no convenient way to perform these operations.
> The /samples/bpf/load_sock_ops.c implements attach/detacg operations,
> but only for BPF_CGROUP_SOCK_OPS programs. Bps (part of bcc) implements
> bpf introspection, but lacks any cgroup-related specific.
> 
> I find having a tool to perform these basic operations in the kernel tree
> very useful, as it can be used in the corresponding bpf documentation
> without creating additional dependencies. And bpftool seems to be
> a right tool to extend with such functionality.

Could you place your code in a new file and add a new "object level"?
I.e. 
bpftool cgroup list 
bpftool cgroup attach ...
bpftool cgroup help
etc?  Note that you probably want the list to be first, so if someone
types "bpftool cg" it runs list by default.

Does it make sense to support pinned files and specifying programs by
id?  I used the "id"/"pinned" keywords so that users can choose to use
either.  Perhaps you should at least prefix the file to with "file"?
So:
$ bpftool cgattach file ./mybpfprog.o /sys/fs/cgroup/user.slice/ ingress
$ bpftool cgattach id 19 /sys/fs/cgroup/user.slice/ ingress
$ bpftool cgattach pin /bpf/prog /sys/fs/cgroup/user.slice/ ingress
Would this make sense?

Smaller nits on the coding style:
 - please try to run checkpatch, perhaps you did, but some people
   forget tools are in the kernel tree :)
 - please keep includes in alphabetical order;
 - please keep variable declarations in functions ordered longest to
   shortest, if that's impossible because of dependency between
   initializers - move the initializers to the code.

Please also don't forget to update/create new man page.

Thanks! :)