From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 92924C433E0 for ; Fri, 31 Jul 2020 20:34:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6EADB2087C for ; Fri, 31 Jul 2020 20:34:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727796AbgGaUef (ORCPT ); Fri, 31 Jul 2020 16:34:35 -0400 Received: from www62.your-server.de ([213.133.104.62]:38544 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727093AbgGaUef (ORCPT ); Fri, 31 Jul 2020 16:34:35 -0400 Received: from sslproxy02.your-server.de ([78.47.166.47]) by www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89_1) (envelope-from ) id 1k1bjZ-0001Bw-24; Fri, 31 Jul 2020 22:34:29 +0200 Received: from [178.196.57.75] (helo=pc-9.home) by sslproxy02.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k1bjY-0000mO-R7; Fri, 31 Jul 2020 22:34:28 +0200 Subject: Re: [PATCH bpf-next 3/3] libbpf: Use bpf_probe_read_kernel To: Andrii Nakryiko Cc: Ilya Leoshkevich , Alexei Starovoitov , bpf , Heiko Carstens , Vasily Gorbik References: <20200728120059.132256-1-iii@linux.ibm.com> <20200728120059.132256-4-iii@linux.ibm.com> <6177128b-bef5-7445-bf00-8051f8efa3bc@iogearbox.net> From: Daniel Borkmann Message-ID: Date: Fri, 31 Jul 2020 22:34:27 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Authenticated-Sender: daniel@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.102.3/25890/Fri Jul 31 17:04:57 2020) Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On 7/31/20 7:41 PM, Andrii Nakryiko wrote: > On Wed, Jul 29, 2020 at 3:12 PM Daniel Borkmann wrote: >> On 7/30/20 12:05 AM, Andrii Nakryiko wrote: >>> On Wed, Jul 29, 2020 at 2:54 PM Daniel Borkmann wrote: >>>> On 7/29/20 11:36 PM, Andrii Nakryiko wrote: >>>>> On Wed, Jul 29, 2020 at 2:01 PM Daniel Borkmann wrote: >>>>>> On 7/29/20 6:06 AM, Andrii Nakryiko wrote: >>>>>>> On Tue, Jul 28, 2020 at 2:16 PM Daniel Borkmann wrote: >>>>>>>> On 7/28/20 9:11 PM, Andrii Nakryiko wrote: >>>>>>>>> On Tue, Jul 28, 2020 at 5:15 AM Ilya Leoshkevich wrote: >>>>>>>>>> >>>>>>>>>> Yet another adaptation to commit 0ebeea8ca8a4 ("bpf: Restrict >>>>>>>>>> bpf_probe_read{, str}() only to archs where they work") that makes more >>>>>>>>>> samples compile on s390. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Ilya Leoshkevich >>>>>>>>> >>>>>>>>> Sorry, we can't do this yet. This will break on older kernels that >>>>>>>>> don't yet have bpf_probe_read_kernel() implemented. Met and Yonghong >>>>>>>>> are working on extending a set of CO-RE relocations, that would allow >>>>>>>>> to do bpf_probe_read_kernel() detection on BPF side, transparently for >>>>>>>>> an application, and will pick either bpf_probe_read() or >>>>>>>>> bpf_probe_read_kernel(). It should be ready soon (this or next week, >>>>>>>>> most probably), though it will have dependency on the latest Clang. >>>>>>>>> But for now, please don't change this. >>>>>>>> >>>>>>>> Could you elaborate what this means wrt dependency on latest clang? Given clang >>>>>>>> releases have a rather long cadence, what about existing users with current clang >>>>>>>> releases? >>>>>>> >>>>>>> So the overall idea is to use something like this to do kernel reads: >>>>>>> >>>>>>> static __always_inline int bpf_probe_read_universal(void *dst, u32 sz, >>>>>>> const void *src) >>>>>>> { >>>>>>> if (bpf_core_type_exists(btf_bpf_probe_read_kernel)) >>>>>>> return bpf_probe_read_kernel(dst, sz, src); >>>>>>> else >>>>>>> return bpf_probe_read(dst, sz, src); >>>>>>> } >>>>>>> >>>>>>> And then use bpf_probe_read_universal() in BPF_CORE_READ and family. >>>>>>> >>>>>>> This approach relies on few things: >>>>>>> >>>>>>> 1. each BPF helper has a corresponding btf_ type defined for it >>>>>>> 2. bpf_core_type_exists(some_type) returns 0 or 1, depending if >>>>>>> specified type is found in kernel BTF (so needs kernel BTF, of >>>>>>> course). This is the part me and Yonghong are working on at the >>>>>>> moment. >>>>>>> 3. verifier's dead code elimination, which will leave only >>>>>>> bpf_probe_read() or bpf_probe_read_kernel() calls and will remove the >>>>>>> other one. So on older kernels, there will never be unsupoorted call >>>>>>> to bpf_probe_read_kernel(). >>>>>>> >>>>>>> The new type existence relocation requires the latest Clang. So the >>>>>>> way to deal with older Clangs would be to just fallback to >>>>>>> bpf_probe_read, if we detect that Clang is too old and can't emit >>>>>>> necessary relocation. >>>>>> >>>>>> Okay, seems reasonable overall. One question though: couldn't libbpf transparently >>>>>> fix up the selection of bpf_probe_read() vs bpf_probe_read_kernel()? E.g. it would >>>>>> probe the kernel whether bpf_probe_read_kernel() is available and if it is then it >>>>>> would rewrite the raw call number from the instruction from bpf_probe_read() into >>>>>> the one for bpf_probe_read_kernel()? I guess the question then becomes whether the >>>>>> original use for bpf_probe_read() was related to CO-RE. But I think this could also >>>>>> be overcome by adding a fake helper signature in libbpf with a unreasonable high >>>>>> number that is dedicated to probing mem via CO-RE and then libbpf picks the right >>>>>> underlying helper call number for the insn. That avoids fiddling with macros and >>>>>> need for new clang version, no (unless I'm missing something)? >>>>> >>>>> Libbpf could do it, but I'm a bit worried that unconditionally >>>>> changing bpf_probe_read() into bpf_probe_read_kernel() is going to be >>>>> wrong in some cases. If that wasn't the case, why wouldn't we just >>>>> re-purpose bpf_probe_read() into bpf_probe_read_kernel() in kernel >>>>> itself, right? >>>> >>>> Yes, that is correct, but I mentioned above that this new 'fake' helper call number >>>> that libbpf would be fixing up would only be used for bpf_probe_read{,str}() inside >>>> bpf_core_read.h. >>>> >>>> Small example, bpf_core_read.h would be changed to (just an extract): >>>> >>>> diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h >>>> index eae5cccff761..4bddb2ddf3f0 100644 >>>> --- a/tools/lib/bpf/bpf_core_read.h >>>> +++ b/tools/lib/bpf/bpf_core_read.h >>>> @@ -115,7 +115,7 @@ enum bpf_field_info_kind { >>>> * (local) BTF, used to record relocation. >>>> */ >>>> #define bpf_core_read(dst, sz, src) \ >>>> - bpf_probe_read(dst, sz, \ >>>> + bpf_probe_read_selector(dst, sz, \ >>>> (const void *)__builtin_preserve_access_index(src)) >>>> >>>> /* >>>> @@ -124,7 +124,7 @@ enum bpf_field_info_kind { >>>> * argument. >>>> */ >>>> #define bpf_core_read_str(dst, sz, src) \ >>>> - bpf_probe_read_str(dst, sz, \ >>>> + bpf_probe_read_str_selector(dst, sz, \ >>>> (const void *)__builtin_preserve_access_index(src)) >>>> >>>> #define ___concat(a, b) a ## b >>>> >>>> And bpf_probe_read_{,str_}selector would be defined as e.g. ... >>>> >>>> static long (*bpf_probe_read_selector)(void *dst, __u32 size, const void *unsafe_ptr) = (void *) -1; >>>> static long (*bpf_probe_read_str_selector)(void *dst, __u32 size, const void *unsafe_ptr) = (void *) -2; >>>> >>>> ... where libbpf would do the fix up to either 4 or 45 for insn->imm. But it's still >>>> confined to usage in bpf_core_read.h when the CO-RE macros are used. >>> >>> Ah, I see. Yeah, I suppose that would work as well. Do you prefer me >>> to go this way? >> >> I would suggest we should try this path given this can be used with any clang version >> that has the BPF backend, not just latest upstream git. > > I have an even better solution, I think. Convert everything to > bpf_probe_read_kernel() or bpf_probe_read_user() unconditionally, but > let libbpf switch those two to bpf_probe_read() if _kernel()/_user() > variants are not yet in the kernel. That should handle both CO-RE > helpers and just pretty much any use case that was converted. Yes, agree, that is an even cleaner solution and avoids to 'pollute' the helper ID space with such remapping. The user intent with bpf_probe_read_kernel() or bpf_probe_read_user() is rather clear so bpf_probe_read() can be a fallback for this direction. Lets go with that. Thanks, Daniel