[{"id":1798332,"web_url":"http://patchwork.ozlabs.org/comment/1798332/","msgid":"<59FBA64D.1050400@iogearbox.net>","list_archive_url":null,"date":"2017-11-02T23:12:13","subject":"Re: [PATCH 1/2] bpf: add a bpf_override_function helper","submitter":{"id":65705,"url":"http://patchwork.ozlabs.org/api/people/65705/","name":"Daniel Borkmann","email":"daniel@iogearbox.net"},"content":"Hi Josef,\n\none more issue I just noticed, see comment below:\n\nOn 11/02/2017 03:37 PM, Josef Bacik wrote:\n[...]\n> diff --git a/include/linux/filter.h b/include/linux/filter.h\n> index cdd78a7beaae..dfa44fd74bae 100644\n> --- a/include/linux/filter.h\n> +++ b/include/linux/filter.h\n> @@ -458,7 +458,8 @@ struct bpf_prog {\n>   \t\t\t\tlocked:1,\t/* Program image locked? */\n>   \t\t\t\tgpl_compatible:1, /* Is filter GPL compatible? */\n>   \t\t\t\tcb_access:1,\t/* Is control block accessed? */\n> -\t\t\t\tdst_needed:1;\t/* Do we need dst entry? */\n> +\t\t\t\tdst_needed:1,\t/* Do we need dst entry? */\n> +\t\t\t\tkprobe_override:1; /* Do we override a kprobe? */\n>   \tkmemcheck_bitfield_end(meta);\n>   \tenum bpf_prog_type\ttype;\t\t/* Type of BPF program */\n>   \tu32\t\t\tlen;\t\t/* Number of filter blocks */\n[...]\n> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c\n> index d906775e12c1..f8f7927a9152 100644\n> --- a/kernel/bpf/verifier.c\n> +++ b/kernel/bpf/verifier.c\n> @@ -4189,6 +4189,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)\n>   \t\t\tprog->dst_needed = 1;\n>   \t\tif (insn->imm == BPF_FUNC_get_prandom_u32)\n>   \t\t\tbpf_user_rnd_init_once();\n> +\t\tif (insn->imm == BPF_FUNC_override_return)\n> +\t\t\tprog->kprobe_override = 1;\n>   \t\tif (insn->imm == BPF_FUNC_tail_call) {\n>   \t\t\t/* If we tail call into other programs, we\n>   \t\t\t * cannot make any assumptions since they can\n> diff --git a/kernel/events/core.c b/kernel/events/core.c\n> index 9660ee65fbef..0d7fce52391d 100644\n> --- a/kernel/events/core.c\n> +++ b/kernel/events/core.c\n> @@ -8169,6 +8169,13 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)\n>   \t\treturn -EINVAL;\n>   \t}\n>\n> +\t/* Kprobe override only works for kprobes, not uprobes. */\n> +\tif (prog->kprobe_override &&\n> +\t    !(event->tp_event->flags & TRACE_EVENT_FL_KPROBE)) {\n> +\t\tbpf_prog_put(prog);\n> +\t\treturn -EINVAL;\n> +\t}\n\nCan we somehow avoid the prog->kprobe_override flag here completely\nand also same in the perf_event_attach_bpf_prog() handler?\n\nReason is that it's not reliable for bailing out this way: Think of\nthe main program you're attaching doesn't use bpf_override_return()\nhelper, but it tail-calls into other BPF progs that make use of it\ninstead. So above check would be useless and will fail and we continue\nto attach the prog for probes where it's not intended to be used.\n\nWe've had similar issues in the past e.g. c2002f983767 (\"bpf: fix\nchecking xdp_adjust_head on tail calls\") is just one of those. Thus,\ncan we avoid the flag altogether and handle such error case differently?\n\n>   \tif (is_tracepoint || is_syscall_tp) {\n>   \t\tint off = trace_event_get_offsets(event->tp_event);\n\nThanks,\nDaniel","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","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=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3ySgpr6xDzz9s5L\n\tfor <patchwork-incoming@ozlabs.org>;\n\tFri,  3 Nov 2017 10:12:36 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S964915AbdKBXMY (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tThu, 2 Nov 2017 19:12:24 -0400","from www62.your-server.de ([213.133.104.62]:44106 \"EHLO\n\twww62.your-server.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S964895AbdKBXMW (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Thu, 2 Nov 2017 19:12:22 -0400","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 <daniel@iogearbox.net>)\n\tid 1eAOej-0004lR-Oe; Fri, 03 Nov 2017 00:12:13 +0100"],"Message-ID":"<59FBA64D.1050400@iogearbox.net>","Date":"Fri, 03 Nov 2017 00:12:13 +0100","From":"Daniel Borkmann <daniel@iogearbox.net>","User-Agent":"Mozilla/5.0 (X11; Linux x86_64;\n\trv:31.0) Gecko/20100101 Thunderbird/31.7.0","MIME-Version":"1.0","To":"Josef Bacik <josef@toxicpanda.com>, rostedt@goodmis.org,\n\tmingo@redhat.com, davem@davemloft.net, netdev@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, ast@kernel.org, kernel-team@fb.com","CC":"Josef Bacik <jbacik@fb.com>","Subject":"Re: [PATCH 1/2] bpf: add a bpf_override_function helper","References":"<1509633431-2184-1-git-send-email-josef@toxicpanda.com>\n\t<1509633431-2184-2-git-send-email-josef@toxicpanda.com>","In-Reply-To":"<1509633431-2184-2-git-send-email-josef@toxicpanda.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/24010/Thu Nov  2 13:07:00 2017)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1798651,"web_url":"http://patchwork.ozlabs.org/comment/1798651/","msgid":"<20171103143135.bnlwu7hmtgmgjdri@destiny>","list_archive_url":null,"date":"2017-11-03T14:31:36","subject":"Re: [PATCH 1/2] bpf: add a bpf_override_function helper","submitter":{"id":72383,"url":"http://patchwork.ozlabs.org/api/people/72383/","name":"Josef Bacik","email":"josef@toxicpanda.com"},"content":"On Fri, Nov 03, 2017 at 12:12:13AM +0100, Daniel Borkmann wrote:\n> Hi Josef,\n> \n> one more issue I just noticed, see comment below:\n> \n> On 11/02/2017 03:37 PM, Josef Bacik wrote:\n> [...]\n> > diff --git a/include/linux/filter.h b/include/linux/filter.h\n> > index cdd78a7beaae..dfa44fd74bae 100644\n> > --- a/include/linux/filter.h\n> > +++ b/include/linux/filter.h\n> > @@ -458,7 +458,8 @@ struct bpf_prog {\n> >   \t\t\t\tlocked:1,\t/* Program image locked? */\n> >   \t\t\t\tgpl_compatible:1, /* Is filter GPL compatible? */\n> >   \t\t\t\tcb_access:1,\t/* Is control block accessed? */\n> > -\t\t\t\tdst_needed:1;\t/* Do we need dst entry? */\n> > +\t\t\t\tdst_needed:1,\t/* Do we need dst entry? */\n> > +\t\t\t\tkprobe_override:1; /* Do we override a kprobe? */\n> >   \tkmemcheck_bitfield_end(meta);\n> >   \tenum bpf_prog_type\ttype;\t\t/* Type of BPF program */\n> >   \tu32\t\t\tlen;\t\t/* Number of filter blocks */\n> [...]\n> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c\n> > index d906775e12c1..f8f7927a9152 100644\n> > --- a/kernel/bpf/verifier.c\n> > +++ b/kernel/bpf/verifier.c\n> > @@ -4189,6 +4189,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)\n> >   \t\t\tprog->dst_needed = 1;\n> >   \t\tif (insn->imm == BPF_FUNC_get_prandom_u32)\n> >   \t\t\tbpf_user_rnd_init_once();\n> > +\t\tif (insn->imm == BPF_FUNC_override_return)\n> > +\t\t\tprog->kprobe_override = 1;\n> >   \t\tif (insn->imm == BPF_FUNC_tail_call) {\n> >   \t\t\t/* If we tail call into other programs, we\n> >   \t\t\t * cannot make any assumptions since they can\n> > diff --git a/kernel/events/core.c b/kernel/events/core.c\n> > index 9660ee65fbef..0d7fce52391d 100644\n> > --- a/kernel/events/core.c\n> > +++ b/kernel/events/core.c\n> > @@ -8169,6 +8169,13 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)\n> >   \t\treturn -EINVAL;\n> >   \t}\n> > \n> > +\t/* Kprobe override only works for kprobes, not uprobes. */\n> > +\tif (prog->kprobe_override &&\n> > +\t    !(event->tp_event->flags & TRACE_EVENT_FL_KPROBE)) {\n> > +\t\tbpf_prog_put(prog);\n> > +\t\treturn -EINVAL;\n> > +\t}\n> \n> Can we somehow avoid the prog->kprobe_override flag here completely\n> and also same in the perf_event_attach_bpf_prog() handler?\n> \n> Reason is that it's not reliable for bailing out this way: Think of\n> the main program you're attaching doesn't use bpf_override_return()\n> helper, but it tail-calls into other BPF progs that make use of it\n> instead. So above check would be useless and will fail and we continue\n> to attach the prog for probes where it's not intended to be used.\n> \n> We've had similar issues in the past e.g. c2002f983767 (\"bpf: fix\n> checking xdp_adjust_head on tail calls\") is just one of those. Thus,\n> can we avoid the flag altogether and handle such error case differently?\n> \n\nSo if I'm reading this right there's no way to know what we'll tail call at any\ngiven point, so I need to go back to my previous iteration of this patch and\nalways save the state of the kprobe in the per-cpu variable to make sure we\ndon't use bpf_override_return in the wrong case?\n\nThe tail call functions won't be in the BPF_PROG_ARRAY right?  It'll be just\nsome other arbitrary function?  If that's the case then we really need something\nlike this\n\nhttps://patchwork.kernel.org/patch/10034815/\n\nand I need to bring that back right?  Thanks,\n\nJosef","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","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=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=toxicpanda-com.20150623.gappssmtp.com\n\theader.i=@toxicpanda-com.20150623.gappssmtp.com\n\theader.b=\"ZhKtd29i\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yT4FH21bsz9s8J\n\tfor <patchwork-incoming@ozlabs.org>;\n\tSat,  4 Nov 2017 01:33:23 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753966AbdKCObl (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tFri, 3 Nov 2017 10:31:41 -0400","from mail-qt0-f195.google.com ([209.85.216.195]:50161 \"EHLO\n\tmail-qt0-f195.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751415AbdKCObj (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Fri, 3 Nov 2017 10:31:39 -0400","by mail-qt0-f195.google.com with SMTP id k31so3438010qta.6\n\tfor <netdev@vger.kernel.org>; Fri, 03 Nov 2017 07:31:39 -0700 (PDT)","from localhost\n\t(cpe-2606-A000-4381-1201-225-22FF-FEB3-E51A.dyn6.twc.com.\n\t[2606:a000:4381:1201:225:22ff:feb3:e51a])\n\tby smtp.gmail.com with ESMTPSA id\n\ty7sm3811330qke.58.2017.11.03.07.31.37\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 03 Nov 2017 07:31:37 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=toxicpanda-com.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=wvuKcyP0zFgWOrGXHo8z3t3R6d5oiaoCdLm/+NdPG5Q=;\n\tb=ZhKtd29iUK0YWY3oOCw2icDJb49iiJySE6i4soU9cQmdbitMlzn40C8C0TulCa8vcr\n\t/wYnI2T2OEwyTVX45Gk+dnlxTyn3SdKv/fWv3rt+nEtJJH6nNsaGfP2B1dcgLR4G2m4U\n\tVUoh9Tgp+wYhNS/+N8tSgSC2xJWbxu4/aTsNmQ9wy4WAZIuCxl/Ati3I2E5FCrYXIT20\n\tNd0WwuizoVlkXBTRQ4P1UpMuJGvhab0Hqxu6N75lSWTFoYGMUYm5onjSR+JJkIP0Usyz\n\tJ2I7rJxAVtf+WEzVXlxHhYXYr0yn3uTdDD33xGF1m6qHxfh4a/TfvJgkg9sK5QYmkAhq\n\tuz3w==","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:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=wvuKcyP0zFgWOrGXHo8z3t3R6d5oiaoCdLm/+NdPG5Q=;\n\tb=Y/GHYYoe8lAwjnAPv9Z5IEghsGOH4rbyA93GVDvlcZc5KsT8yVC+E7FPJkR9u1ki4o\n\toNxCLTDplQGTeTXldTJ/lT0KzDfv0rmHMC70dk2KM0eNZvn6rOiI9gQ4Fr5BnDUek29e\n\tL+916nc2mZ+MrS2viuJqh4G6+z3XaJQTf1KAFES6R7ynKO3HruBGZ4LJNbjRkJJiSm9X\n\tFr6Ln0ukcvvCnv4Tpi/jos8BVLLJPr1PfmK527KHklEu1utqd0ePAj++wKkMc3b/UpdZ\n\tZ1zhSdza9fmLeJreTI1hOpZBYP0B8HPdp6EkbmDaKgJm1lKCNEHURD+pFhiiKhsz3fwv\n\tYgvQ==","X-Gm-Message-State":"AMCzsaWl/O9Zhn2bzMNhmz2666QimCahW6k4tZc0G9eQhOBALmvy5vOU\n\tvT6ghK781J/Zv6IMQnXoFdvhgg==","X-Google-Smtp-Source":"ABhQp+S3N+YlIjThk3PHskro7dJXfpCggooNXzl9ZRTxoeEfXKlALYiZXwii5XpTxetGrwoE1DgccA==","X-Received":"by 10.200.25.112 with SMTP id g45mr10546456qtk.40.1509719498553; \n\tFri, 03 Nov 2017 07:31:38 -0700 (PDT)","Date":"Fri, 3 Nov 2017 10:31:36 -0400","From":"Josef Bacik <josef@toxicpanda.com>","To":"Daniel Borkmann <daniel@iogearbox.net>","Cc":"Josef Bacik <josef@toxicpanda.com>, rostedt@goodmis.org,\n\tmingo@redhat.com, davem@davemloft.net, netdev@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, ast@kernel.org, kernel-team@fb.com,\n\tJosef Bacik <jbacik@fb.com>","Subject":"Re: [PATCH 1/2] bpf: add a bpf_override_function helper","Message-ID":"<20171103143135.bnlwu7hmtgmgjdri@destiny>","References":"<1509633431-2184-1-git-send-email-josef@toxicpanda.com>\n\t<1509633431-2184-2-git-send-email-josef@toxicpanda.com>\n\t<59FBA64D.1050400@iogearbox.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<59FBA64D.1050400@iogearbox.net>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1798742,"web_url":"http://patchwork.ozlabs.org/comment/1798742/","msgid":"<59FC9EC6.3060900@iogearbox.net>","list_archive_url":null,"date":"2017-11-03T16:52:22","subject":"Re: [PATCH 1/2] bpf: add a bpf_override_function helper","submitter":{"id":65705,"url":"http://patchwork.ozlabs.org/api/people/65705/","name":"Daniel Borkmann","email":"daniel@iogearbox.net"},"content":"On 11/03/2017 03:31 PM, Josef Bacik wrote:\n> On Fri, Nov 03, 2017 at 12:12:13AM +0100, Daniel Borkmann wrote:\n>> Hi Josef,\n>>\n>> one more issue I just noticed, see comment below:\n>>\n>> On 11/02/2017 03:37 PM, Josef Bacik wrote:\n>> [...]\n>>> diff --git a/include/linux/filter.h b/include/linux/filter.h\n>>> index cdd78a7beaae..dfa44fd74bae 100644\n>>> --- a/include/linux/filter.h\n>>> +++ b/include/linux/filter.h\n>>> @@ -458,7 +458,8 @@ struct bpf_prog {\n>>>    \t\t\t\tlocked:1,\t/* Program image locked? */\n>>>    \t\t\t\tgpl_compatible:1, /* Is filter GPL compatible? */\n>>>    \t\t\t\tcb_access:1,\t/* Is control block accessed? */\n>>> -\t\t\t\tdst_needed:1;\t/* Do we need dst entry? */\n>>> +\t\t\t\tdst_needed:1,\t/* Do we need dst entry? */\n>>> +\t\t\t\tkprobe_override:1; /* Do we override a kprobe? */\n>>>    \tkmemcheck_bitfield_end(meta);\n>>>    \tenum bpf_prog_type\ttype;\t\t/* Type of BPF program */\n>>>    \tu32\t\t\tlen;\t\t/* Number of filter blocks */\n>> [...]\n>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c\n>>> index d906775e12c1..f8f7927a9152 100644\n>>> --- a/kernel/bpf/verifier.c\n>>> +++ b/kernel/bpf/verifier.c\n>>> @@ -4189,6 +4189,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)\n>>>    \t\t\tprog->dst_needed = 1;\n>>>    \t\tif (insn->imm == BPF_FUNC_get_prandom_u32)\n>>>    \t\t\tbpf_user_rnd_init_once();\n>>> +\t\tif (insn->imm == BPF_FUNC_override_return)\n>>> +\t\t\tprog->kprobe_override = 1;\n>>>    \t\tif (insn->imm == BPF_FUNC_tail_call) {\n>>>    \t\t\t/* If we tail call into other programs, we\n>>>    \t\t\t * cannot make any assumptions since they can\n>>> diff --git a/kernel/events/core.c b/kernel/events/core.c\n>>> index 9660ee65fbef..0d7fce52391d 100644\n>>> --- a/kernel/events/core.c\n>>> +++ b/kernel/events/core.c\n>>> @@ -8169,6 +8169,13 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)\n>>>    \t\treturn -EINVAL;\n>>>    \t}\n>>>\n>>> +\t/* Kprobe override only works for kprobes, not uprobes. */\n>>> +\tif (prog->kprobe_override &&\n>>> +\t    !(event->tp_event->flags & TRACE_EVENT_FL_KPROBE)) {\n>>> +\t\tbpf_prog_put(prog);\n>>> +\t\treturn -EINVAL;\n>>> +\t}\n>>\n>> Can we somehow avoid the prog->kprobe_override flag here completely\n>> and also same in the perf_event_attach_bpf_prog() handler?\n>>\n>> Reason is that it's not reliable for bailing out this way: Think of\n>> the main program you're attaching doesn't use bpf_override_return()\n>> helper, but it tail-calls into other BPF progs that make use of it\n>> instead. So above check would be useless and will fail and we continue\n>> to attach the prog for probes where it's not intended to be used.\n>>\n>> We've had similar issues in the past e.g. c2002f983767 (\"bpf: fix\n>> checking xdp_adjust_head on tail calls\") is just one of those. Thus,\n>> can we avoid the flag altogether and handle such error case differently?\n>\n> So if I'm reading this right there's no way to know what we'll tail call at any\n> given point, so I need to go back to my previous iteration of this patch and\n> always save the state of the kprobe in the per-cpu variable to make sure we\n> don't use bpf_override_return in the wrong case?\n\nYeah.\n\n> The tail call functions won't be in the BPF_PROG_ARRAY right?  It'll be just\n> some other arbitrary function?  If that's the case then we really need something\n> like this\n\nWith BPF_PROG_ARRAY you mean BPF_MAP_TYPE_PROG_ARRAY or the prog array\nfor the tracing/multiprog attach point? The program you're calling into\nis inside the BPF_MAP_TYPE_PROG_ARRAY map, but can change at any time\nand can have nesting as well.\n\n> https://patchwork.kernel.org/patch/10034815/\n>\n> and I need to bring that back right?  Thanks,\n\nI'm afraid so. The thing with skb cb_access which was brought up there is\nthat once you have a tail call in the prog you cannot make any assumptions\nanymore, therefore the cb_access flag is set to 1 so we save/restore for\nthose cases precautionary since it could be accessed or not later on. In\nyour case I think this wouldn't work since legitimate bpf kprobes progs could\nuse tail calls today, so setting prog->kprobe_override there would prevent\nattaching for non-kprobes due to subsequent flags & TRACE_EVENT_FL_KPROBE\ncheck.","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","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=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yT7N82sCgz9sRW\n\tfor <patchwork-incoming@ozlabs.org>;\n\tSat,  4 Nov 2017 03:54:32 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755168AbdKCQwf (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tFri, 3 Nov 2017 12:52:35 -0400","from www62.your-server.de ([213.133.104.62]:58864 \"EHLO\n\twww62.your-server.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1754486AbdKCQwe (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Fri, 3 Nov 2017 12:52:34 -0400","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 <daniel@iogearbox.net>)\n\tid 1eAfCl-0008Sa-Ie; Fri, 03 Nov 2017 17:52:27 +0100"],"Message-ID":"<59FC9EC6.3060900@iogearbox.net>","Date":"Fri, 03 Nov 2017 17:52:22 +0100","From":"Daniel Borkmann <daniel@iogearbox.net>","User-Agent":"Mozilla/5.0 (X11; Linux x86_64;\n\trv:31.0) Gecko/20100101 Thunderbird/31.7.0","MIME-Version":"1.0","To":"Josef Bacik <josef@toxicpanda.com>","CC":"rostedt@goodmis.org, mingo@redhat.com, davem@davemloft.net,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tast@kernel.org, kernel-team@fb.com, Josef Bacik <jbacik@fb.com>","Subject":"Re: [PATCH 1/2] bpf: add a bpf_override_function helper","References":"<1509633431-2184-1-git-send-email-josef@toxicpanda.com>\n\t<1509633431-2184-2-git-send-email-josef@toxicpanda.com>\n\t<59FBA64D.1050400@iogearbox.net>\n\t<20171103143135.bnlwu7hmtgmgjdri@destiny>","In-Reply-To":"<20171103143135.bnlwu7hmtgmgjdri@destiny>","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/24010/Thu Nov  2 13:07:00 2017)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1798928,"web_url":"http://patchwork.ozlabs.org/comment/1798928/","msgid":"<20171103210753.odvacnyh56krj7zn@ast-mbp>","list_archive_url":null,"date":"2017-11-03T21:07:56","subject":"Re: [PATCH 1/2] bpf: add a bpf_override_function helper","submitter":{"id":42586,"url":"http://patchwork.ozlabs.org/api/people/42586/","name":"Alexei Starovoitov","email":"alexei.starovoitov@gmail.com"},"content":"On Fri, Nov 03, 2017 at 05:52:22PM +0100, Daniel Borkmann wrote:\n> On 11/03/2017 03:31 PM, Josef Bacik wrote:\n> > On Fri, Nov 03, 2017 at 12:12:13AM +0100, Daniel Borkmann wrote:\n> > > Hi Josef,\n> > > \n> > > one more issue I just noticed, see comment below:\n> > > \n> > > On 11/02/2017 03:37 PM, Josef Bacik wrote:\n> > > [...]\n> > > > diff --git a/include/linux/filter.h b/include/linux/filter.h\n> > > > index cdd78a7beaae..dfa44fd74bae 100644\n> > > > --- a/include/linux/filter.h\n> > > > +++ b/include/linux/filter.h\n> > > > @@ -458,7 +458,8 @@ struct bpf_prog {\n> > > >    \t\t\t\tlocked:1,\t/* Program image locked? */\n> > > >    \t\t\t\tgpl_compatible:1, /* Is filter GPL compatible? */\n> > > >    \t\t\t\tcb_access:1,\t/* Is control block accessed? */\n> > > > -\t\t\t\tdst_needed:1;\t/* Do we need dst entry? */\n> > > > +\t\t\t\tdst_needed:1,\t/* Do we need dst entry? */\n> > > > +\t\t\t\tkprobe_override:1; /* Do we override a kprobe? */\n> > > >    \tkmemcheck_bitfield_end(meta);\n> > > >    \tenum bpf_prog_type\ttype;\t\t/* Type of BPF program */\n> > > >    \tu32\t\t\tlen;\t\t/* Number of filter blocks */\n> > > [...]\n> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c\n> > > > index d906775e12c1..f8f7927a9152 100644\n> > > > --- a/kernel/bpf/verifier.c\n> > > > +++ b/kernel/bpf/verifier.c\n> > > > @@ -4189,6 +4189,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)\n> > > >    \t\t\tprog->dst_needed = 1;\n> > > >    \t\tif (insn->imm == BPF_FUNC_get_prandom_u32)\n> > > >    \t\t\tbpf_user_rnd_init_once();\n> > > > +\t\tif (insn->imm == BPF_FUNC_override_return)\n> > > > +\t\t\tprog->kprobe_override = 1;\n> > > >    \t\tif (insn->imm == BPF_FUNC_tail_call) {\n> > > >    \t\t\t/* If we tail call into other programs, we\n> > > >    \t\t\t * cannot make any assumptions since they can\n> > > > diff --git a/kernel/events/core.c b/kernel/events/core.c\n> > > > index 9660ee65fbef..0d7fce52391d 100644\n> > > > --- a/kernel/events/core.c\n> > > > +++ b/kernel/events/core.c\n> > > > @@ -8169,6 +8169,13 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)\n> > > >    \t\treturn -EINVAL;\n> > > >    \t}\n> > > > \n> > > > +\t/* Kprobe override only works for kprobes, not uprobes. */\n> > > > +\tif (prog->kprobe_override &&\n> > > > +\t    !(event->tp_event->flags & TRACE_EVENT_FL_KPROBE)) {\n> > > > +\t\tbpf_prog_put(prog);\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > \n> > > Can we somehow avoid the prog->kprobe_override flag here completely\n> > > and also same in the perf_event_attach_bpf_prog() handler?\n> > > \n> > > Reason is that it's not reliable for bailing out this way: Think of\n> > > the main program you're attaching doesn't use bpf_override_return()\n> > > helper, but it tail-calls into other BPF progs that make use of it\n> > > instead. So above check would be useless and will fail and we continue\n> > > to attach the prog for probes where it's not intended to be used.\n> > > \n> > > We've had similar issues in the past e.g. c2002f983767 (\"bpf: fix\n> > > checking xdp_adjust_head on tail calls\") is just one of those. Thus,\n> > > can we avoid the flag altogether and handle such error case differently?\n> > \n> > So if I'm reading this right there's no way to know what we'll tail call at any\n> > given point, so I need to go back to my previous iteration of this patch and\n> > always save the state of the kprobe in the per-cpu variable to make sure we\n> > don't use bpf_override_return in the wrong case?\n> \n> Yeah.\n> \n> > The tail call functions won't be in the BPF_PROG_ARRAY right?  It'll be just\n> > some other arbitrary function?  If that's the case then we really need something\n> > like this\n> \n> With BPF_PROG_ARRAY you mean BPF_MAP_TYPE_PROG_ARRAY or the prog array\n> for the tracing/multiprog attach point? The program you're calling into\n> is inside the BPF_MAP_TYPE_PROG_ARRAY map, but can change at any time\n> and can have nesting as well.\n> \n> > https://patchwork.kernel.org/patch/10034815/\n> > \n> > and I need to bring that back right?  Thanks,\n> \n> I'm afraid so. The thing with skb cb_access which was brought up there is\n> that once you have a tail call in the prog you cannot make any assumptions\n> anymore, therefore the cb_access flag is set to 1 so we save/restore for\n> those cases precautionary since it could be accessed or not later on. In\n> your case I think this wouldn't work since legitimate bpf kprobes progs could\n> use tail calls today, so setting prog->kprobe_override there would prevent\n> attaching for non-kprobes due to subsequent flags & TRACE_EVENT_FL_KPROBE\n> check.\n\nhow about preventing programs that use bpf_override_return to be\nstore into prog_array? imo that would be cleaner solution.\ndoing tail_call into them is kinda useless anyway.\nThen we can keep the bit and fast path.","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","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=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"CRHNixzW\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yTF3H25hDz9s81\n\tfor <patchwork-incoming@ozlabs.org>;\n\tSat,  4 Nov 2017 08:10:19 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754811AbdKCVKG (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tFri, 3 Nov 2017 17:10:06 -0400","from mail-pf0-f195.google.com ([209.85.192.195]:45290 \"EHLO\n\tmail-pf0-f195.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752114AbdKCVKE (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Fri, 3 Nov 2017 17:10:04 -0400","by mail-pf0-f195.google.com with SMTP id d28so3061124pfe.2;\n\tFri, 03 Nov 2017 14:10:03 -0700 (PDT)","from ast-mbp ([61.40.109.130]) by smtp.gmail.com with ESMTPSA id\n\tf191sm9986517pgc.32.2017.11.03.14.09.59\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 03 Nov 2017 14:10:02 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=RA7Ins3cLx+T4+rXnTfvLtSlOdl0stSdcGgOCDfJndE=;\n\tb=CRHNixzWChJsnVQe9aDjFj+AC1zbU33kQDI7Aju+R7wAHOn/1PajmSjudHxT3kmOOw\n\tgcDOrpTqrbaUy3U9HUvg6lpmthWi4fYE6INMtEW06ywyMIw2jeReY8QNgOyH3WxIYov+\n\tWgMGuui/zrZWQTQP24vOWNbIm/8QwBFFvqwQWgP1YE600sjn5/bi9+QdhfTNZk5ICRvo\n\tfnOIc75GVjiatarpg00C7KE6rHULFnHst6RepU7Ndavuo4GoW3rB4NhTTU5lGJ52q4l7\n\t5Ov/Np8HVIytOUPfPuNkRq3D4a80twl9uaqyMDKTTqL8yzGOdx9yWie5iblqUg2SRYMZ\n\tcRhQ==","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:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=RA7Ins3cLx+T4+rXnTfvLtSlOdl0stSdcGgOCDfJndE=;\n\tb=PQCWZN3GcbxFbR3icQw9Z5kJkOHiYDecLgMYgKQURKoZS4/5T/hLnSdg+5ubEpONhD\n\t3YLSzcIYIXIZSqfnMpWkQTYWy3Lfe+MdAjMWO2rYBFo2kzHzVdqGJBFZq2FItmUNeqlt\n\tSc79stNxZ5x2uGmrRGGW3cqrKfxHPTVh5Rd6HY7GIDEVlr3e7cdkKhJVwhjqpoOA51vY\n\tclpQ1m8YZK6OLuXHs8QQvioujYbLtGJzVesyoxCqLoGh5ucF2PalWAVHinuCgahm+cNl\n\tZIApkJcuxBePfXXCjj3s36zOIdU6DYR6hoixHMOvJ+moVQtQdiKwtQVvhKvs874RMOsp\n\tviQg==","X-Gm-Message-State":"AMCzsaVqhTV0C6W3ZOZlHNop1nA7qGqmzOMR1eApQigZayjncQP9kuA5\n\twmQEjdituA4jHnbnfiULg+4=","X-Google-Smtp-Source":"ABhQp+SLn1ad7Gk1OmnKNGhC5O22lTpIaZzyg+xh3TYsWogo+Pwwz8gdKzsBz/b1MjFJ11/bsWFCLA==","X-Received":"by 10.84.211.137 with SMTP id c9mr8108206pli.218.1509743403186; \n\tFri, 03 Nov 2017 14:10:03 -0700 (PDT)","Date":"Sat, 4 Nov 2017 06:07:56 +0900","From":"Alexei Starovoitov <alexei.starovoitov@gmail.com>","To":"Daniel Borkmann <daniel@iogearbox.net>","Cc":"Josef Bacik <josef@toxicpanda.com>, rostedt@goodmis.org,\n\tmingo@redhat.com, davem@davemloft.net, netdev@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, ast@kernel.org, kernel-team@fb.com,\n\tJosef Bacik <jbacik@fb.com>","Subject":"Re: [PATCH 1/2] bpf: add a bpf_override_function helper","Message-ID":"<20171103210753.odvacnyh56krj7zn@ast-mbp>","References":"<1509633431-2184-1-git-send-email-josef@toxicpanda.com>\n\t<1509633431-2184-2-git-send-email-josef@toxicpanda.com>\n\t<59FBA64D.1050400@iogearbox.net>\n\t<20171103143135.bnlwu7hmtgmgjdri@destiny>\n\t<59FC9EC6.3060900@iogearbox.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<59FC9EC6.3060900@iogearbox.net>","User-Agent":"NeoMutt/20170421 (1.8.2)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}}]