[{"id":1799867,"web_url":"http://patchwork.ozlabs.org/comment/1799867/","msgid":"<5A009CBD.1080800@iogearbox.net>","list_archive_url":null,"date":"2017-11-06T17:32:45","subject":"Re: [PATCH net-next v2 02/15] bpf: offload: add infrastructure for\n\tloading programs for a specific netdev","submitter":{"id":65705,"url":"http://patchwork.ozlabs.org/api/people/65705/","name":"Daniel Borkmann","email":"daniel@iogearbox.net"},"content":"On 11/03/2017 09:56 PM, Jakub Kicinski wrote:\n> The fact that we don't know which device the program is going\n> to be used on is quite limiting in current eBPF infrastructure.\n> We have to reverse or limit the changes which kernel makes to\n> the loaded bytecode if we want it to be offloaded to a networking\n> device. We also have to invent new APIs for debugging and\n> troubleshooting support.\n>\n> Make it possible to load programs for a specific netdev. This\n> helps us to bring the debug information closer to the core\n> eBPF infrastructure (e.g. we will be able to reuse the verifer\n> log in device JIT). It allows device JITs to perform translation\n> on the original bytecode.\n>\n> __bpf_prog_get() when called to get a reference for an attachment\n> point will now refuse to give it if program has a device assigned.\n> Following patches will add a version of that function which passes\n> the expected netdev in. @type argument in __bpf_prog_get() is\n> renamed to attach_type to make it clearer that it's only set on\n> attachment.\n>\n> All calls to ndo_bpf are protected by rtnl, only verifier callbacks\n> are not. We need a wait queue to make sure netdev doesn't get\n> destroyed while verifier is still running and calling its driver.\n>\n> Signed-off-by: Jakub Kicinski \n> Reviewed-by: Simon Horman \n> Reviewed-by: Quentin Monnet \n\nFirst of all, great work, I went over the series and I really like\nthe outcome. It's applied already anyway, but two minor comments\nfurther below.\n\n[...]\n> @@ -1549,6 +1555,8 @@ static void bpf_prog_free_deferred(struct work_struct *work)\n> \tstruct bpf_prog_aux *aux;\n>\n> \taux = container_of(work, struct bpf_prog_aux, work);\n> +\tif (bpf_prog_is_dev_bound(aux))\n> +\t\tbpf_prog_offload_destroy(aux->prog);\n> \tbpf_jit_free(aux->prog);\n> }\n[...]\n> +static int bpf_offload_notification(struct notifier_block *notifier,\n> +\t\t\t\t ulong event, void *ptr)\n> +{\n> +\tstruct net_device *netdev = netdev_notifier_info_to_dev(ptr);\n> +\tstruct bpf_dev_offload *offload, *tmp;\n> +\n> +\tASSERT_RTNL();\n> +\n> +\tswitch (event) {\n> +\tcase NETDEV_UNREGISTER:\n> +\t\tlist_for_each_entry_safe(offload, tmp, &bpf_prog_offload_devs,\n> +\t\t\t\t\t offloads) {\n> +\t\t\tif (offload->netdev == netdev)\n> +\t\t\t\t__bpf_prog_offload_destroy(offload->prog);\n\nWe would be calling this twice, right? Once here and then on prog\ndestruction again. __bpf_prog_offload_destroy() looks it will handle\nthis just fine, but we should probably add a comment to\n__bpf_prog_offload_destroy() such that when changes are made to it\nit's obvious that we need to be extra careful.\n\n[...]\n> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c\n> index 323be2473c4b..1574b9f0f24e 100644\n> --- a/kernel/bpf/syscall.c\n> +++ b/kernel/bpf/syscall.c\n> @@ -824,7 +824,10 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog)\n> \tif (type >= ARRAY_SIZE(bpf_prog_types) || !bpf_prog_types[type])\n> \t\treturn -EINVAL;\n>\n> -\tprog->aux->ops = bpf_prog_types[type];\n> +\tif (!bpf_prog_is_dev_bound(prog->aux))\n> +\t\tprog->aux->ops = bpf_prog_types[type];\n> +\telse\n> +\t\tprog->aux->ops = &bpf_offload_prog_ops;\n> \tprog->type = type;\n> \treturn 0;\n> }\n> @@ -1054,7 +1057,7 @@ struct bpf_prog *bpf_prog_inc_not_zero(struct bpf_prog *prog)\n> }\n> EXPORT_SYMBOL_GPL(bpf_prog_inc_not_zero);\n>\n> -static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *type)\n> +static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type)\n> {\n> \tstruct fd f = fdget(ufd);\n> \tstruct bpf_prog *prog;\n> @@ -1062,7 +1065,7 @@ static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *type)\n> \tprog = ____bpf_prog_get(f);\n> \tif (IS_ERR(prog))\n> \t\treturn prog;\n> -\tif (type && prog->type != *type) {\n> +\tif (attach_type && (prog->type != *attach_type || prog->aux->offload)) {\n> \t\tprog = ERR_PTR(-EINVAL);\n> \t\tgoto out;\n> \t}\n> @@ -1089,7 +1092,7 @@ struct bpf_prog *bpf_prog_get_type(u32 ufd, enum bpf_prog_type type)\n> EXPORT_SYMBOL_GPL(bpf_prog_get_type);\n>\n> /* last field in 'union bpf_attr' used by this command */\n> -#define\tBPF_PROG_LOAD_LAST_FIELD prog_name\n> +#define\tBPF_PROG_LOAD_LAST_FIELD prog_target_ifindex\n\nFor program types that are neither XDP nor cls_bpf, we should reject\nthe request if something calls bpf(2) with non-0 prog_target_ifindex.\n\nThat way, i) we don't burn the whole field and could perhaps reuse/union\nit for other prog types like tracing in future. Probably makes sense to\ndo anyway since ii) for types like tracing, we would want to reject this\nupfront here and not when later attach happens.\n\nI probably missed something when reading the code, but if I spotted\nthat correctly, we might otherwise even go and nfp-jit simple progs\nfor non-networking types (we would bail out later though on in\n__bpf_prog_get() ... but we shouldn't let syscall return in first\nplace)?\n\n> static int bpf_prog_load(union bpf_attr *attr)\n> {\n> @@ -1152,6 +1155,12 @@ static int bpf_prog_load(union bpf_attr *attr)\n> \tatomic_set(&prog->aux->refcnt, 1);\n> \tprog->gpl_compatible = is_gpl ? 1 : 0;\n>\n> +\tif (attr->prog_target_ifindex) {\n> +\t\terr = bpf_prog_offload_init(prog, attr);\n> +\t\tif (err)\n> +\t\t\tgoto free_prog;\n> +\t}\n> +\n> \t/* find program type: socket_filter vs tracing_filter */\n> \terr = find_prog_type(type, prog);\n[...]","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yW0526xHHz9sBZ\n\tfor ;\n\tTue, 7 Nov 2017 04:32:54 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753493AbdKFRcw (ORCPT );\n\tMon, 6 Nov 2017 12:32:52 -0500","from www62.your-server.de ([213.133.104.62]:45362 \"EHLO\n\twww62.your-server.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753430AbdKFRcu (ORCPT\n\t); Mon, 6 Nov 2017 12:32:50 -0500","from [194.230.159.142] (helo=localhost.localdomain)\n\tby www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-SHA:256)\n\t(Exim 4.85_2) (envelope-from )\n\tid 1eBlGQ-0007at-ML; Mon, 06 Nov 2017 18:32:46 +0100"],"Message-ID":"<5A009CBD.1080800@iogearbox.net>","Date":"Mon, 06 Nov 2017 18:32:45 +0100","From":"Daniel Borkmann ","User-Agent":"Mozilla/5.0 (X11; Linux x86_64;\n\trv:31.0) Gecko/20100101 Thunderbird/31.7.0","MIME-Version":"1.0","To":"Jakub Kicinski , netdev@vger.kernel.org","CC":"oss-drivers@netronome.com, alexei.starovoitov@gmail.com","Subject":"Re: [PATCH net-next v2 02/15] bpf: offload: add infrastructure for\n\tloading programs for a specific netdev","References":"<20171103205630.1083-1-jakub.kicinski@netronome.com>\n\t<20171103205630.1083-3-jakub.kicinski@netronome.com>","In-Reply-To":"<20171103205630.1083-3-jakub.kicinski@netronome.com>","Content-Type":"text/plain; charset=windows-1252; format=flowed","Content-Transfer-Encoding":"7bit","X-Authenticated-Sender":"daniel@iogearbox.net","X-Virus-Scanned":"Clear (ClamAV 0.99.2/24019/Mon Nov 6 14:06:54 2017)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1802488,"web_url":"http://patchwork.ozlabs.org/comment/1802488/","msgid":"<20171109175856.2426be2c@cakuba>","list_archive_url":null,"date":"2017-11-10T01:58:56","subject":"Re: [PATCH net-next v2 02/15] bpf: offload: add infrastructure for\n\tloading programs for a specific netdev","submitter":{"id":67484,"url":"http://patchwork.ozlabs.org/api/people/67484/","name":"Jakub Kicinski","email":"jakub.kicinski@netronome.com"},"content":"Hi!\n\nSorry for the delay!\n\nOn Mon, 06 Nov 2017 18:32:45 +0100, Daniel Borkmann wrote:\n> On 11/03/2017 09:56 PM, Jakub Kicinski wrote:\n> > @@ -1549,6 +1555,8 @@ static void bpf_prog_free_deferred(struct work_struct *work)\n> > \tstruct bpf_prog_aux *aux;\n> >\n> > \taux = container_of(work, struct bpf_prog_aux, work);\n> > +\tif (bpf_prog_is_dev_bound(aux))\n> > +\t\tbpf_prog_offload_destroy(aux->prog);\n> > \tbpf_jit_free(aux->prog);\n> > } \n> [...]\n> > +static int bpf_offload_notification(struct notifier_block *notifier,\n> > +\t\t\t\t ulong event, void *ptr)\n> > +{\n> > +\tstruct net_device *netdev = netdev_notifier_info_to_dev(ptr);\n> > +\tstruct bpf_dev_offload *offload, *tmp;\n> > +\n> > +\tASSERT_RTNL();\n> > +\n> > +\tswitch (event) {\n> > +\tcase NETDEV_UNREGISTER:\n> > +\t\tlist_for_each_entry_safe(offload, tmp, &bpf_prog_offload_devs,\n> > +\t\t\t\t\t offloads) {\n> > +\t\t\tif (offload->netdev == netdev)\n> > +\t\t\t\t__bpf_prog_offload_destroy(offload->prog); \n> \n> We would be calling this twice, right? Once here and then on prog\n> destruction again. __bpf_prog_offload_destroy() looks it will handle\n> this just fine, but we should probably add a comment to\n> __bpf_prog_offload_destroy() such that when changes are made to it\n> it's obvious that we need to be extra careful.\n\nGood point, I will add the comment.\n\n> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c\n> > index 323be2473c4b..1574b9f0f24e 100644\n> > --- a/kernel/bpf/syscall.c\n> > +++ b/kernel/bpf/syscall.c\n> > @@ -824,7 +824,10 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog)\n> > \tif (type >= ARRAY_SIZE(bpf_prog_types) || !bpf_prog_types[type])\n> > \t\treturn -EINVAL;\n> >\n> > -\tprog->aux->ops = bpf_prog_types[type];\n> > +\tif (!bpf_prog_is_dev_bound(prog->aux))\n> > +\t\tprog->aux->ops = bpf_prog_types[type];\n> > +\telse\n> > +\t\tprog->aux->ops = &bpf_offload_prog_ops;\n> > \tprog->type = type;\n> > \treturn 0;\n> > }\n> > @@ -1054,7 +1057,7 @@ struct bpf_prog *bpf_prog_inc_not_zero(struct bpf_prog *prog)\n> > }\n> > EXPORT_SYMBOL_GPL(bpf_prog_inc_not_zero);\n> >\n> > -static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *type)\n> > +static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type)\n> > {\n> > \tstruct fd f = fdget(ufd);\n> > \tstruct bpf_prog *prog;\n> > @@ -1062,7 +1065,7 @@ static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *type)\n> > \tprog = ____bpf_prog_get(f);\n> > \tif (IS_ERR(prog))\n> > \t\treturn prog;\n> > -\tif (type && prog->type != *type) {\n> > +\tif (attach_type && (prog->type != *attach_type || prog->aux->offload)) {\n> > \t\tprog = ERR_PTR(-EINVAL);\n> > \t\tgoto out;\n> > \t}\n> > @@ -1089,7 +1092,7 @@ struct bpf_prog *bpf_prog_get_type(u32 ufd, enum bpf_prog_type type)\n> > EXPORT_SYMBOL_GPL(bpf_prog_get_type);\n> >\n> > /* last field in 'union bpf_attr' used by this command */\n> > -#define\tBPF_PROG_LOAD_LAST_FIELD prog_name\n> > +#define\tBPF_PROG_LOAD_LAST_FIELD prog_target_ifindex \n> \n> For program types that are neither XDP nor cls_bpf, we should reject\n> the request if something calls bpf(2) with non-0 prog_target_ifindex.\n> \n> That way, i) we don't burn the whole field and could perhaps reuse/union\n> it for other prog types like tracing in future. Probably makes sense to\n> do anyway since ii) for types like tracing, we would want to reject this\n> upfront here and not when later attach happens.\n> \n> I probably missed something when reading the code, but if I spotted\n> that correctly, we might otherwise even go and nfp-jit simple progs\n> for non-networking types (we would bail out later though on in\n> __bpf_prog_get() ... but we shouldn't let syscall return in first\n> place)?\n\nAgreed, I will fix this.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=netronome-com.20150623.gappssmtp.com\n\theader.i=@netronome-com.20150623.gappssmtp.com\n\theader.b=\"o3tb25KU\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yY39q2T4jz9t2M\n\tfor ;\n\tFri, 10 Nov 2017 12:59:11 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755675AbdKJB7H (ORCPT );\n\tThu, 9 Nov 2017 20:59:07 -0500","from mail-pg0-f65.google.com ([74.125.83.65]:48344 \"EHLO\n\tmail-pg0-f65.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1755550AbdKJB7F (ORCPT\n\t); Thu, 9 Nov 2017 20:59:05 -0500","by mail-pg0-f65.google.com with SMTP id s11so787983pgc.5\n\tfor ; Thu, 09 Nov 2017 17:59:05 -0800 (PST)","from cakuba ([121.135.231.177]) by smtp.gmail.com with ESMTPSA id\n\t132sm9642839pga.80.2017.11.09.17.59.02\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 09 Nov 2017 17:59:04 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=netronome-com.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:in-reply-to:references\n\t:organization:mime-version:content-transfer-encoding;\n\tbh=q2DhAX5mYd1+Vm8OG49VyZfQ4vbFMpO6K8NPha3cqq0=;\n\tb=o3tb25KUWZEQDAAJAR7hFD7Eb5ExyrPHAvwyhmwiXv12q7sD13FUuvpMWRS24RbKHF\n\tnk7Z31spIeb/z1xXy2I4R8aCgbrlsYE5dFcf7IR0l15yw+M7BviW1YJoVRZ4n9wtCdrT\n\tjIdLoIbNMVMSKOAE04JzbT83R0U6fHylcBosMne0NyolHiaPqkVNkLfojvHgLfD6xp70\n\tGXCRyfXPcqvTmF3XrJZ1JSuxBF87WkVaBjJzVTDPgAI/kfD7dXCc8+/A5voEM2xuzFZv\n\tFEI7UoeDTEbF9p/JSEXtU14YnIqLqFg/zF01mBrt50C8jyPtSCcZUqVkd2pnaf+rN9P8\n\tM7YQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to\n\t:references:organization:mime-version:content-transfer-encoding;\n\tbh=q2DhAX5mYd1+Vm8OG49VyZfQ4vbFMpO6K8NPha3cqq0=;\n\tb=KvVTJW0J4ZoiwPqHsPEEIyMxX/tOx9bAGsHJQOskSRssPakQqNQZJLRDiQnAfiHGen\n\tqxmpmygAOfvDLc/p+m4GWxN5uEUA3EI7dmBoPbpYHxvi2OK9z7Le7LigzQBvdlyYzL3P\n\tw6lYilVJN1oXWaAr/1TurO/LDxeuCV0HifMpFWdGz09jOzmYdK1WEewzn4WxkHIP8M6X\n\te1fhNwIhY2ccTFJadbJ6+as/88e5NRwLISsM0hOM4yKFkfcde1K67TslPdUd+MewYdhC\n\tFs7NXtF7BA3LBEYBO5eFxXs/nrwxf2Q7XFU2724p4GsMnBfykUIaXnHyAUzltyxDr0Sj\n\tCbVw==","X-Gm-Message-State":"AJaThX56p0hgMvnANj9O1xSpDiwDgcYcHa6uKyLN399Vt0KmJevSC0+d\n\tZhOdR1J9GMW57x3+NlN66G6T3A==","X-Google-Smtp-Source":"ABhQp+R0ylwuBmkx46EvX7Lb4Do0MBB13cU/om0Gu7qfR6iJzlWot/GHCk/qHZbKpK38At4iGOI5/Q==","X-Received":"by 10.101.100.216 with SMTP id t24mr2339926pgv.439.1510279144668;\n\tThu, 09 Nov 2017 17:59:04 -0800 (PST)","Date":"Thu, 9 Nov 2017 17:58:56 -0800","From":"Jakub Kicinski ","To":"Daniel Borkmann ","Cc":"netdev@vger.kernel.org, oss-drivers@netronome.com,\n\talexei.starovoitov@gmail.com","Subject":"Re: [PATCH net-next v2 02/15] bpf: offload: add infrastructure for\n\tloading programs for a specific netdev","Message-ID":"<20171109175856.2426be2c@cakuba>","In-Reply-To":"<5A009CBD.1080800@iogearbox.net>","References":"<20171103205630.1083-1-jakub.kicinski@netronome.com>\n\t<20171103205630.1083-3-jakub.kicinski@netronome.com>\n\t<5A009CBD.1080800@iogearbox.net>","Organization":"Netronome Systems, Ltd.","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]