[{"id":2365141,"web_url":"http://patchwork.ozlabs.org/comment/2365141/","msgid":"","list_archive_url":null,"date":"2020-02-18T23:03:49","subject":"Re: [PATCH 03/18] bpf: Add struct bpf_ksym","submitter":{"id":65705,"url":"http://patchwork.ozlabs.org/api/people/65705/","name":"Daniel Borkmann","email":"daniel@iogearbox.net"},"content":"On 2/16/20 8:29 PM, Jiri Olsa wrote:\n> Adding 'struct bpf_ksym' object that will carry the\n> kallsym information for bpf symbol. Adding the start\n> and end address to begin with. It will be used by\n> bpf_prog, bpf_trampoline, bpf_dispatcher.\n> \n> Using the bpf_func for program symbol start instead\n> of the image start, because it will be used later for\n> kallsyms program value and it makes no difference\n> (compared to the image start) for sorting bpf programs.\n> \n> Signed-off-by: Jiri Olsa \n> ---\n> include/linux/bpf.h | 6 ++++++\n> kernel/bpf/core.c | 26 +++++++++++---------------\n> 2 files changed, 17 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/linux/bpf.h b/include/linux/bpf.h\n> index be7afccc9459..5ad8eea1cd37 100644\n> --- a/include/linux/bpf.h\n> +++ b/include/linux/bpf.h\n> @@ -462,6 +462,11 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,\n> u64 notrace __bpf_prog_enter(void);\n> void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);\n> \n> +struct bpf_ksym {\n> +\tunsigned long\t\t start;\n> +\tunsigned long\t\t end;\n> +};\n> +\n> enum bpf_tramp_prog_type {\n> \tBPF_TRAMP_FENTRY,\n> \tBPF_TRAMP_FEXIT,\n> @@ -643,6 +648,7 @@ struct bpf_prog_aux {\n> \tu32 size_poke_tab;\n> \tstruct latch_tree_node ksym_tnode;\n> \tstruct list_head ksym_lnode;\n> +\tstruct bpf_ksym ksym;\n> \tconst struct bpf_prog_ops *ops;\n> \tstruct bpf_map **used_maps;\n> \tstruct bpf_prog *prog;\n> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c\n> index 973a20d49749..39a9e4184900 100644\n> --- a/kernel/bpf/core.c\n> +++ b/kernel/bpf/core.c\n> @@ -524,17 +524,15 @@ int bpf_jit_harden __read_mostly;\n> long bpf_jit_limit __read_mostly;\n> \n> static __always_inline void\n> -bpf_get_prog_addr_region(const struct bpf_prog *prog,\n> -\t\t\t unsigned long *symbol_start,\n> -\t\t\t unsigned long *symbol_end)\n> +bpf_get_prog_addr_region(const struct bpf_prog *prog)\n> {\n> \tconst struct bpf_binary_header *hdr = bpf_jit_binary_hdr(prog);\n> \tunsigned long addr = (unsigned long)hdr;\n> \n> \tWARN_ON_ONCE(!bpf_prog_ebpf_jited(prog));\n> \n> -\t*symbol_start = addr;\n> -\t*symbol_end = addr + hdr->pages * PAGE_SIZE;\n> +\tprog->aux->ksym.start = (unsigned long) prog->bpf_func;\n\nYour commit descriptions are too terse. :/ What does \"because it will be used\nlater for kallsyms program value\" mean exactly compared to how it's used today\nfor programs?\n\nIs this a requirement to have them point exactly to prog->bpf_func and if so\nwhy? My concern is that bpf_func has a random offset from hdr, so even if the\n/proc/kallsyms would be readable with concrete addresses for !cap_sys_admin\nusers, it's still not the concrete start address being exposed there, but the\nallocated range instead.\n\n> +\tprog->aux->ksym.end = addr + hdr->pages * PAGE_SIZE;\n> }\n>","headers":{"Return-Path":"","X-Original-To":"incoming-bpf@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-bpf@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (no SPF record)\n\tsmtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67;\n\thelo=vger.kernel.org; envelope-from=bpf-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dmarc=none (p=none dis=none)\n\theader.from=iogearbox.net"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 48Mby30VfPz9sRk\n\tfor ;\n\tWed, 19 Feb 2020 10:03:55 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1727206AbgBRXDy (ORCPT\n\t);\n\tTue, 18 Feb 2020 18:03:54 -0500","from www62.your-server.de ([213.133.104.62]:56932 \"EHLO\n\twww62.your-server.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1727211AbgBRXDx (ORCPT ); \n\tTue, 18 Feb 2020 18:03:53 -0500","from sslproxy05.your-server.de ([78.46.172.2])\n\tby www62.your-server.de with esmtpsa\n\t(TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89_1)\n\t(envelope-from )\n\tid 1j4BuA-0008E5-MW; Wed, 19 Feb 2020 00:03:50 +0100","from [85.7.42.192] (helo=pc-9.home)\n\tby sslproxy05.your-server.de with esmtpsa\n\t(TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92)\n\t(envelope-from )\n\tid 1j4BuA-00072U-7Q; Wed, 19 Feb 2020 00:03:50 +0100"],"Subject":"Re: [PATCH 03/18] bpf: Add struct bpf_ksym","To":"Jiri Olsa , Alexei Starovoitov ","Cc":"netdev@vger.kernel.org, bpf@vger.kernel.org, Andrii Nakryiko\n\t, Yonghong Song , Song Liu\n\t, Martin KaFai Lau ,\n\tJakub Kicinski , David Miller ,\n\t=?utf-8?b?QmrDtnJuIFTDtnBlbA==?= ,\n\tJohn Fastabend , Jesper Dangaard Brouer\n\t, Arnaldo Carvalho de Melo ","References":"<20200216193005.144157-1-jolsa@kernel.org>\n\t<20200216193005.144157-4-jolsa@kernel.org>","From":"Daniel Borkmann ","Message-ID":"","Date":"Wed, 19 Feb 2020 00:03:49 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.2","MIME-Version":"1.0","In-Reply-To":"<20200216193005.144157-4-jolsa@kernel.org>","Content-Type":"text/plain; charset=windows-1252; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Authenticated-Sender":"daniel@iogearbox.net","X-Virus-Scanned":"Clear (ClamAV 0.102.1/25727/Tue Feb 18 15:05:00 2020)","Sender":"bpf-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"bpf@vger.kernel.org"}},{"id":2365434,"web_url":"http://patchwork.ozlabs.org/comment/2365434/","msgid":"<20200219091711.GD439238@krava>","list_archive_url":null,"date":"2020-02-19T09:17:11","subject":"Re: [PATCH 03/18] bpf: Add struct bpf_ksym","submitter":{"id":2492,"url":"http://patchwork.ozlabs.org/api/people/2492/","name":"Jiri Olsa","email":"jolsa@redhat.com"},"content":"On Wed, Feb 19, 2020 at 12:03:49AM +0100, Daniel Borkmann wrote:\n> On 2/16/20 8:29 PM, Jiri Olsa wrote:\n> > Adding 'struct bpf_ksym' object that will carry the\n> > kallsym information for bpf symbol. Adding the start\n> > and end address to begin with. It will be used by\n> > bpf_prog, bpf_trampoline, bpf_dispatcher.\n> > \n> > Using the bpf_func for program symbol start instead\n> > of the image start, because it will be used later for\n> > kallsyms program value and it makes no difference\n> > (compared to the image start) for sorting bpf programs.\n> > \n> > Signed-off-by: Jiri Olsa \n> > ---\n> > include/linux/bpf.h | 6 ++++++\n> > kernel/bpf/core.c | 26 +++++++++++---------------\n> > 2 files changed, 17 insertions(+), 15 deletions(-)\n> > \n> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h\n> > index be7afccc9459..5ad8eea1cd37 100644\n> > --- a/include/linux/bpf.h\n> > +++ b/include/linux/bpf.h\n> > @@ -462,6 +462,11 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,\n> > u64 notrace __bpf_prog_enter(void);\n> > void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);\n> > +struct bpf_ksym {\n> > +\tunsigned long\t\t start;\n> > +\tunsigned long\t\t end;\n> > +};\n> > +\n> > enum bpf_tramp_prog_type {\n> > \tBPF_TRAMP_FENTRY,\n> > \tBPF_TRAMP_FEXIT,\n> > @@ -643,6 +648,7 @@ struct bpf_prog_aux {\n> > \tu32 size_poke_tab;\n> > \tstruct latch_tree_node ksym_tnode;\n> > \tstruct list_head ksym_lnode;\n> > +\tstruct bpf_ksym ksym;\n> > \tconst struct bpf_prog_ops *ops;\n> > \tstruct bpf_map **used_maps;\n> > \tstruct bpf_prog *prog;\n> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c\n> > index 973a20d49749..39a9e4184900 100644\n> > --- a/kernel/bpf/core.c\n> > +++ b/kernel/bpf/core.c\n> > @@ -524,17 +524,15 @@ int bpf_jit_harden __read_mostly;\n> > long bpf_jit_limit __read_mostly;\n> > static __always_inline void\n> > -bpf_get_prog_addr_region(const struct bpf_prog *prog,\n> > -\t\t\t unsigned long *symbol_start,\n> > -\t\t\t unsigned long *symbol_end)\n> > +bpf_get_prog_addr_region(const struct bpf_prog *prog)\n> > {\n> > \tconst struct bpf_binary_header *hdr = bpf_jit_binary_hdr(prog);\n> > \tunsigned long addr = (unsigned long)hdr;\n> > \tWARN_ON_ONCE(!bpf_prog_ebpf_jited(prog));\n> > -\t*symbol_start = addr;\n> > -\t*symbol_end = addr + hdr->pages * PAGE_SIZE;\n> > +\tprog->aux->ksym.start = (unsigned long) prog->bpf_func;\n> \n> Your commit descriptions are too terse. :/ What does \"because it will be used\n> later for kallsyms program value\" mean exactly compared to how it's used today\n> for programs?\n\nthere's symbol_start/symbol_end values originally used to sort\nbpf_prog objects, and there's prog->bpf_func value used as address\nthat is displayed in the /proc/kallsyms\n\nI'm putting prog->bpf_func to bpf_ksym->start, so it's later on\ndisplayed as bpf_prog address in /proc/kallsyms in this patch:\n\n\tbpf: Add lnode list node to struct bpf_ksym\n\t---\n\t@@ -736,13 +736,13 @@ int bpf_get_kallsym(unsigned int symnum, unsigned long *value, char *type,\n\t\t\treturn ret;\n\t \n\t\trcu_read_lock();\n\t- list_for_each_entry_rcu(aux, &bpf_kallsyms, ksym_lnode) {\n\t+ list_for_each_entry_rcu(ksym, &bpf_kallsyms, lnode) {\n\t\t\tif (it++ != symnum)\n\t\t\t\tcontinue;\n\t \n\t- strncpy(sym, aux->ksym.name, KSYM_NAME_LEN);\n\t+ strncpy(sym, ksym->name, KSYM_NAME_LEN);\n\t \n\t- *value = (unsigned long)aux->prog->bpf_func;\n\t+ *value = ksym->start;\n\t\t\t*type = BPF_SYM_ELF_TYPE;\n\n\nand also the prog->bpf_func value is now used as memory 'start' to\nsort bpf_prog objects, which will do the same job as symbol_start\n\nbut maybe we could have 'kallsym' value in 'bpf_ksym' which would be\nused as value to display in /proc/kallsyms kallsyms, like:\n\n struct bpf_ksym {\n unsigned long start;\n unsigned long end;\n unsigned long kallsyms;\n }\n\nand keep 'start/end' to be the whole memory bounds for sorting to\navoid any confusion and surprises in future\n\n> \n> Is this a requirement to have them point exactly to prog->bpf_func and if so\n> why? My concern is that bpf_func has a random offset from hdr, so even if the\n> /proc/kallsyms would be readable with concrete addresses for !cap_sys_admin\n> users, it's still not the concrete start address being exposed there, but the\n> allocated range instead.\n\nthere was last review suggestion from Andrii to display the address\nof the actual code start for trampolines and dispatchers instead\nof the start of the who;e memory image, which is actually what we\nneed for perf\n\njirka","headers":{"Return-Path":"","X-Original-To":"incoming-bpf@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-bpf@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (no SPF record)\n\tsmtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67;\n\thelo=vger.kernel.org; envelope-from=bpf-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdmarc=pass (p=none dis=none) header.from=redhat.com","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.a=rsa-sha256 header.s=mimecast20190719\n\theader.b=jD57zBS0; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 48MsZ35TQRz9sRG\n\tfor ;\n\tWed, 19 Feb 2020 20:17:31 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1726469AbgBSJR3 (ORCPT\n\t);\n\tWed, 19 Feb 2020 04:17:29 -0500","from us-smtp-2.mimecast.com ([207.211.31.81]:52541 \"EHLO\n\tus-smtp-delivery-1.mimecast.com\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1726202AbgBSJR3 (ORCPT\n\t); Wed, 19 Feb 2020 04:17:29 -0500","from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com\n\t[209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id\n\tus-mta-263-dYGvu_u4PTCxgY7AnTj_Cg-1; Wed, 19 Feb 2020 04:17:18 -0500","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8BE3C101FC60;\n\tWed, 19 Feb 2020 09:17:16 +0000 (UTC)","from krava (unknown [10.43.17.9])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id B29E062660;\n\tWed, 19 Feb 2020 09:17:13 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1582103847;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=9H+8g76ZRLHoi0QfrBdHaCd9NKX1hy9Hlg4zvECqkTw=;\n\tb=jD57zBS02bvfFfawTAaFS5TTxmEpdKcChaca96jXMJnHWSKzT3p9CZCmeWPcy5DbM552XP\n\t/zcjkn2FJm8tt23liRxHPtp19AUXjNHK3Uq5YTp5KC6koiFN/L3dpnpfTM60nV41XVz84h\n\tPKcypiCJeTo9ast1fuXGDxzVhoKSpzM=","X-MC-Unique":"dYGvu_u4PTCxgY7AnTj_Cg-1","Date":"Wed, 19 Feb 2020 10:17:11 +0100","From":"Jiri Olsa ","To":"Daniel Borkmann ","Cc":"Jiri Olsa , Alexei Starovoitov ,\n\tnetdev@vger.kernel.org, bpf@vger.kernel.org, Andrii Nakryiko\n\t, Yonghong Song , Song Liu\n\t, Martin KaFai Lau ,\n\tJakub Kicinski , David Miller ,\n\t=?iso-8859-1?q?Bj=F6rn_T=F6pel?= ,\n\tJohn Fastabend , Jesper Dangaard Brouer\n\t, Arnaldo Carvalho de Melo ","Subject":"Re: [PATCH 03/18] bpf: Add struct bpf_ksym","Message-ID":"<20200219091711.GD439238@krava>","References":"<20200216193005.144157-1-jolsa@kernel.org>\n\t<20200216193005.144157-4-jolsa@kernel.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","Sender":"bpf-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"bpf@vger.kernel.org"}}]