From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753815AbdLHOSa (ORCPT ); Fri, 8 Dec 2017 09:18:30 -0500 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:41896 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753331AbdLHOSZ (ORCPT ); Fri, 8 Dec 2017 09:18:25 -0500 Date: Fri, 8 Dec 2017 14:17:43 +0000 From: Roman Gushchin To: Jakub Kicinski CC: , , , , , , Quentin Monnet , David Ahern Subject: Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations Message-ID: <20171208141737.GA9912@castle> References: <20171207183909.16240-1-guro@fb.com> <20171207183909.16240-5-guro@fb.com> <20171207142306.285be14c@cakuba.netronome.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20171207142306.285be14c@cakuba.netronome.com> User-Agent: Mutt/1.9.1 (2017-09-22) X-Originating-IP: [2620:10d:c092:180::1:a531] X-ClientProxiedBy: AM5P190CA0010.EURP190.PROD.OUTLOOK.COM (2603:10a6:206:14::23) To CO1PR15MB1080.namprd15.prod.outlook.com (2a01:111:e400:7b66::10) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: e022e643-087e-4fbb-dc72-08d53e467bbe X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(5600026)(4604075)(2017052603307);SRVR:CO1PR15MB1080; X-Microsoft-Exchange-Diagnostics: 1;CO1PR15MB1080;3:FKgyCtS+4+Ce7yNYEZmd8eJvLCwspEFfExsAD8079tqe8wguuzdio3t8SxAbQtLP7/iLLpIOmp1NDN6gdsaoMZ69khEmDgk9deJxQhd4QOTBB1AXRdXTkwx8sHW5MxwnzOjdoo2zeotHglEKDjsUFtZnhuCVrz4KG1qZ1x7Vya5VOBBWAloXHBMJL0Fb2BYSJEe2Vw37SiP6ycFy/K5lNjQxzp1k8eiJKWZ2IYckDfDhozbHD+aeHu/J7yXAJ4Lj;25:+G7MdfZcrrrIT/0tCsO/qggfSSfpRBGiwvaD7q0Rr/r77wRIDMubsbAr0rXyRulsdd02czoaBdWricF3riUwW2XyADcJs1DWG8ltyDg/TiUm1lKDqet56eYYY+Z+77vEKL0zLohvLOxozYvqRIGAdUH9BffFVirkNnkv+g5fxOCI7P6NBrLfXW6fW2K1rsc/gwTE740apBj4czhTcK2Hk4Bm2Ab5LhHe6HUUeKpQfq0Og4GNenneBQkmLWXXkifeGe6JW6zG4OEr/2kvliN4iiqkfbsXdl6qIrYBRRiQIaturadawL71Dh79S/Gitp1BpZ/WZ1i96X+2hxPz8G8aWA==;31:o9GuUP0hOQ2irEr9EejsF7a6v3h2BXKQ+JapkXTfyZzXnUel6iUpJ1CoiE5TVU2w/tjj0qPrJ45pTKelLF2eTcRiur3NN6HWtCdnjzC04EL8E/iq9zTOYx7raL+zpgkIrVbsUDw8k6cvMTBWXdwaQooJVS+ANvdwe3ioGj8flOVrjCKJ27bRbr1/UhYHI3wNbk+OuVm++/+jf9Ev5pzJKBQr6QFMdae2rS8u534WsVg= X-MS-TrafficTypeDiagnostic: CO1PR15MB1080: X-Microsoft-Exchange-Diagnostics: 1;CO1PR15MB1080;20:IdZNeCvU+KA6JcIPo+h8qNQgAkUSYwMB3VnbatsWgpJGDlhVp2Fa6SGDpkYIhi7AErofet1lWU7NnpInAqVIaQ7t4p7J9m3ukVo9lusZyQ8Gv5z4dheeoq91wHiNpJsazjiBxFf/DKWKe6v42ACggqx2LCmcc2IR0RYAupn3JRHvVaa12k9YV2JnPxFhjEnQNHek7FA3ZfSrgJ5ZwQB/+CI1LyTRO0T5LDiV7g4R1YDiNlsmR4jms5o68VhsiiEWuvcvjbrOMSQjBqyv7hfvMK4QjtcEgjAwoKH54sMA9RVrkd6NC2v4wMFRmqNhxrNM68b6TKIUr4vcoXFfiY00vLPP8TI81tIK5xEnGVaicxzkp1Z6siag3scdsETHh5V0or+4NPmT7Tb+snGQOKD8PuohOpaaolVjdPEJ0crq81yZ+r4bv+YqLAcywiJE5WwYP6/uBMHaZQziz6ZW83VK26xDkkMu41AKqNWMVYi0/7p/y3yiHVJgDcvtX0c+8BfL;4:5B72QZi6Y0DYfFKO4z+PmEGzNMl5Z4jMXLX0Horct7KeSb4w52c4T/YJ/tpggW56C+NILfD37xXnVhIiP20rM+HXeAOTXOBR5axgsdUIrWAu0GkeQ9xrOptdQmBgDYO2d5CDHceOyMWHVw4/2MTE/FOm0j5tyCRYVuCKtE19dZ4V0LwsGYKscg5OZ5+kwPuKUGCG4KbF5O8RKErb3A5v7SUYsCEfCZgYsKVkLokzpfRx4CY+D/AiAAhtXtZ0i0i9lZvx+Kt7e/Cl4Doe5I9n4WyTu7W0mrtRvpMSgdggVRM8GkfYcklNHug5w1cYvQfSxkqrk4KMfAnqyk68YwsTzHn3O758nWHwU6WV7EKvUNY= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(67672495146484)(81227570615382); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(11241501159)(6040450)(2401047)(5005006)(8121501046)(10201501046)(3002001)(93006095)(93001095)(3231022)(6041248)(20161123555025)(20161123564025)(20161123558100)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123560025)(6072148)(201708071742011);SRVR:CO1PR15MB1080;BCL:0;PCL:0;RULEID:(100000803101)(100110400095);SRVR:CO1PR15MB1080; X-Forefront-PRVS: 0515208626 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(376002)(366004)(346002)(189003)(199004)(24454002)(106356001)(105586002)(9686003)(55016002)(6246003)(39060400002)(6116002)(53936002)(2906002)(50466002)(25786009)(86362001)(68736007)(23726003)(345774005)(8936002)(478600001)(1076002)(97736004)(33716001)(33656002)(52116002)(7736002)(305945005)(6496006)(52396003)(47776003)(33896004)(5890100001)(76176011)(81166006)(8676002)(81156014)(6666003)(2950100002)(83506002)(5660300001)(316002)(4326008)(58126008)(16586007)(6916009)(54906003)(229853002)(18370500001)(2004002)(42262002);DIR:OUT;SFP:1102;SCL:1;SRVR:CO1PR15MB1080;H:castle;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;CO1PR15MB1080;23:bq3F93g3nyV6tpsbYJ+YqiY41HflTx+mT2d8HPA4Z?= =?us-ascii?Q?dtpAzQpC1mZ/Nqc/s7RRwiT17csCTu04mmKSsVeP0yKzGP5stXWaaaXOo1WH?= =?us-ascii?Q?6qSJROdyjACaMurDC4DNExDsXWpFWHEc49CSOIxu81HSbk5pMDC2qgDA9K0O?= =?us-ascii?Q?BE68BSETJLP0DFmO/T3HBJEX0iPYq2rD9vptahJhMrniqb4gO+T6KkbQABXG?= =?us-ascii?Q?dP/YCmLgLbPOZ2yAahZWs5PczRsAOE57GFbg9sxVbsjtuymmeBN7M3ElZ0M4?= =?us-ascii?Q?OUPDpIWteBwKmnOSf+0mxx5NAv5hFVPYd476xAhxGvvFiflQL2ndJlQUEQsZ?= =?us-ascii?Q?s9HQKOffYDLLjjGhERGjJEDBabI7+VLvbwyMrJU+3bcpqqtmq5mJxKdubczP?= =?us-ascii?Q?8VnPtRvvIF9H6CfiM4XbwmSzZnC60BAcmVJfIGY29b/JpX2/xFSzcmlu6vkO?= =?us-ascii?Q?e5vWHXADydSHiEe5b4DZD+/iza+a5Q7ceOPgu4Zl2jcEQdL7FY1XptmHgBxa?= =?us-ascii?Q?u80249k/gOZjDqVbB7eFhA1+ALzbLFK9AyyxiOLij/yOJCRALQlSgr0kBwh0?= =?us-ascii?Q?skyGhaSjU/6hnKSaj6Z2OR2CCwiXw6Ppv+rnAUK9GPnu3veEef2M/mlX+Skm?= =?us-ascii?Q?DWnnzrADkpbG+62E5ylMNMkvijUynbGvcMcj6DHjlpKLasdc1o2m1PSeeFHE?= =?us-ascii?Q?0DiOJw992M8P21rsKyvGTPyoA2e3q9h8OQ/Xt+uRivfwAeL9L88sNF3Dy/3y?= =?us-ascii?Q?YaiXW7fA6OcSczA/ljmKaJWRnwA2+LWQLswzeWzCzXHF3FrHPq6VosOexSFw?= =?us-ascii?Q?suPh9hHKjoA3Dp74RFB6q3zf0UaSLxHRDrlmN2xK7Ggoa9ZqF9w1+7x2hEwZ?= =?us-ascii?Q?ovKcGqNv894T3JPyHze4wiG3gGwjXcv8IToAPNaHXV4PYZK/+gUMXagEUKLr?= =?us-ascii?Q?BLBoqUWS2CzT8ZbWFJqewYQzOlrRlfqKKdbJK7IRkE9c2h4/CN5gKkWMDNab?= =?us-ascii?Q?MNPJna6niOosCpmPLFWlaQvUmuTmdwUao9iezthq6Il6SCf2Tqd6/P4dUTDZ?= =?us-ascii?Q?A0KPpthFu7GNZVWfrR0ar2He0Spuwys1cxDI8CDER9AYlM2hHq4NTO8P7dGM?= =?us-ascii?Q?SzNXH0GTtWFlvDkVecMZ6g8q6C0DUM/VLhIQuJIdOqbzt9ssdfAxVZbHwx/u?= =?us-ascii?Q?nOFNLNWG1lKd94G+nO1v8eb4UzUjpR/K4dTgS8DfnUdd59NnfFDHPoQdiNUM?= =?us-ascii?Q?DefvEgUZFQvbqoIVyc=3D?= X-Microsoft-Exchange-Diagnostics: 1;CO1PR15MB1080;6:sCgO7/TqEJ35VQo5qwqw378eLK6NgWsYJXomMyJF4J8TltzvTR3nGW8XevxLj1XsuNM4vewIUcXaGuIA25IjwqfRn7hvL95sXRiOg9WJ3LzYNXDRc6OccrX64+Jcj+QXjFS7DEOteLWkzr5PJ5cxQIGO4nDVRV7fa4qhnzG2a5OdNvXzPMnCRjUy+N+PttyljIx908hvg0SI53WVD05DJN0MncKWLwQJLhjfu+vSRWOw2e2ZeZhfjOvTAdJcF6ht8CvMe/ioPlrQHjMb6Wn7ouctzWbtZROp+g1Kuixc1iHl/jJsFQBreisk+ekfGbPZ7S4sicB/2vNl+l0YBbaHkybzMivuozGAYLc1AFURFr0=;5:mlSH1KnHeHbRNTHK539dCeE0lEmoPOi2yabbzjOUUSqaUQAA1McuqBHKZdO0qf0P4erTQT57DwEu1/noTvfDOHyi8JCe03+eK0kf7j1zRTEeoODjoRia0wYaC6SXe+RA4X8oRg3zoV1CKGcVFGHlmTKr+j861zXpUtDk8evSksI=;24:8xTckDv2pYj0cItDiE4P3C1ou/jjDvtfPMglErjzMaSS4gXyutxThZ8auRs23XX25OsdfyhWgSTdln1TfY3sqWyB3LkP/GwfHfPr7GoTbgA=;7:uzJ0EGN4I8GmTsYHG+HgMTGv40jaU+GZg+eEp1nOwElFPVIuCJ7SVjnQrgpDAdqxzJgWLa+s9M0F7IxfPGTqn1L2CDq2xbUyS9GD5NkmN1ulMePG3dmDhefck7gscBpXkUQ5eZW4JLhFjYXAK6SouYZK6BSeZlCdPTxlUuYpfQ1xoFQX40rBzid30NaBwKLfJ0VGCd5Iw8LwbJXtNGSfDG58jIIEkSJdpyoTG3QOfTorSXuY+qcLQWW1UoMSoxV2 SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;CO1PR15MB1080;20:EJGB3qxRkJOF+SOiBIY4VPsuU9E2kA2Jg3C5nRmwXt6COtXg/Y7YpAzTXrVHZZWqkAzQ9vBtugER+URTOvkpHoR/wCRMwaOg/Vhftt+48J/L+cBFUsBJCqKhrpQTjjYXxT7siWa/dVXPKWe05Fk/Z/nMCVTWyuAfZIS6poYVx1c= X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2017 14:17:56.7089 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: e022e643-087e-4fbb-dc72-08d53e467bbe X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2 X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR15MB1080 X-OriginatorOrg: fb.com X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-08_08:,, signatures=0 X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 07, 2017 at 02:23:06PM -0800, Jakub Kicinski wrote: > On Thu, 7 Dec 2017 18:39:09 +0000, Roman Gushchin wrote: > > This patch adds basic cgroup bpf operations to bpftool: > > cgroup list, attach and detach commands. > > > > Usage is described in the corresponding man pages, > > and examples are provided. > > > > Syntax: > > $ bpftool cgroup list CGROUP > > $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS] > > $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG > > > > Signed-off-by: Roman Gushchin > > Looks good, a few very minor nits/questions below. > > > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c > > new file mode 100644 > > index 000000000000..88d67f74313f > > --- /dev/null > > +++ b/tools/bpf/bpftool/cgroup.c > > @@ -0,0 +1,305 @@ > > +/* > > + * Copyright (C) 2017 Facebook > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * as published by the Free Software Foundation; either version > > + * 2 of the License, or (at your option) any later version. > > + * > > + * Author: Roman Gushchin > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > + > > +#include "main.h" > > + > > +static const char * const attach_type_strings[] = { > > + [BPF_CGROUP_INET_INGRESS] = "ingress", > > + [BPF_CGROUP_INET_EGRESS] = "egress", > > + [BPF_CGROUP_INET_SOCK_CREATE] = "sock_create", > > + [BPF_CGROUP_SOCK_OPS] = "sock_ops", > > + [BPF_CGROUP_DEVICE] = "device", > > + [__MAX_BPF_ATTACH_TYPE] = NULL, > > +}; > > + > > +static enum bpf_attach_type parse_attach_type(const char *str) > > +{ > > + enum bpf_attach_type type; > > + > > + for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) { > > + if (attach_type_strings[type] && > > + strcmp(str, attach_type_strings[type]) == 0) > > Here and for matching flags you use straight strcmp(), not our locally > defined is_prefix(), is this intentional? is_prefix() allows > abbreviations, like in iproute2 commands. E.g. this would work: Fixed in v3. > > # bpftool cg att /sys/fs/cgroup/test.slice/ dev id 1 allow_multi > > > + return type; > > + } > > + > > + return __MAX_BPF_ATTACH_TYPE; > > +} > > > +static int list_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type) > > +{ > > + __u32 attach_flags; > > + __u32 prog_ids[1024] = {0}; > > + __u32 prog_cnt, iter; > > + char *attach_flags_str; > > + int ret; > > nit: could you reorder the variables so they're listed longest to > shortest (reverse christmas tree)? > > > + prog_cnt = ARRAY_SIZE(prog_ids); > > + ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids, > > + &prog_cnt); > > + if (ret) > > + return ret; > > + > > + if (prog_cnt == 0) > > + return 0; > > + > > + switch (attach_flags) { > > + case BPF_F_ALLOW_MULTI: > > + attach_flags_str = "allow_multi"; > > + break; > > + case BPF_F_ALLOW_OVERRIDE: > > + attach_flags_str = "allow_override"; > > + break; > > + case 0: > > + attach_flags_str = ""; > > + break; > > + default: > > + attach_flags_str = "unknown"; > > nit: would it make sense to perhaps print flags in hex format in this > case? > > > + } > > + > > + for (iter = 0; iter < prog_cnt; iter++) > > + list_bpf_prog(prog_ids[iter], attach_type_strings[type], > > + attach_flags_str); > > + > > + return 0; > > +} > > > +static int do_attach(int argc, char **argv) > > +{ > > + int cgroup_fd, prog_fd; > > + enum bpf_attach_type attach_type; > > + int attach_flags = 0; > > + int i; > > + int ret = -1; > > + > > + if (argc < 4) { > > + p_err("too few parameters for cgroup attach\n"); > > + goto exit; > > + } > > + > > + cgroup_fd = open(argv[0], O_RDONLY); > > + if (cgroup_fd < 0) { > > + p_err("can't open cgroup %s\n", argv[1]); > > + goto exit; > > + } > > + > > + attach_type = parse_attach_type(argv[1]); > > + if (attach_type == __MAX_BPF_ATTACH_TYPE) { > > + p_err("invalid attach type\n"); > > + goto exit_cgroup; > > + } > > + > > + argc -= 2; > > + argv = &argv[2]; > > + prog_fd = prog_parse_fd(&argc, &argv); > > + if (prog_fd < 0) > > + goto exit_cgroup; > > + > > + for (i = 0; i < argc; i++) { > > + if (strcmp(argv[i], "allow_multi") == 0) { > > + attach_flags |= BPF_F_ALLOW_MULTI; > > + } else if (strcmp(argv[i], "allow_override") == 0) { > > + attach_flags |= BPF_F_ALLOW_OVERRIDE; > > This is the other potential place for is_prefix() I referred to. Not sure about this case, as allow_multi and allow_override have a common "allow_" prefix, so it might be confusing. > > That reminds me - would you care to also update Quentin's bash > completions (tools/bpf/bpftool/bash-completion/bpftool)? They > are _very_ nice to have in day to day use! Sure, will do separately. > Otherwise looks really nice, thanks for working on it! Thank you for reviewing!