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]
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! :)

Powered by blists - more mailing lists