From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-180.mta0.migadu.com (out-180.mta0.migadu.com [91.218.175.180]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CBB85179BF for ; Mon, 20 May 2024 21:32:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.180 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716240730; cv=none; b=jGoqCg1Sr9cfxR5sRW0lAOO+iVg/Rgb/gencwvRi2pHEkQb6LHUhtZg/WguUR/COSEh58sXvH8ShmW8qck0dFIkLFz7bR+7VZA2+ldahMvq/ZZS6acIXptLBRGZF5wFGQkvhv0Az0S5bjQH2DT24bPlOSL75WwvvwIZFaU4jpLg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716240730; c=relaxed/simple; bh=+phe3Dn0kAJsEJKCKqC/u42l/w9QPlrwTCBb5jkGSDM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=pi4d/W7SscLiGAn5CDwjP+GOnOp83DXL5T42ZtohClygDf7BfBKW/bxtxcL/BNuxoVBAKeg5m6uGETfLGmARMqzca+mpQo+ig/1h6O+KQq2pmRs2jgf3L2n9WfKk1mHcF1TI9oexh4iMuyulJ575K7uZ8tNqNvf0Po6aPRdoc4k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=wbDcTEx/; arc=none smtp.client-ip=91.218.175.180 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="wbDcTEx/" X-Envelope-To: ivan@cloudflare.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1716240725; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=bcY+s6m5RT0WTmqxlbBbw/jl5u8VPujdIzVjdDRst3A=; b=wbDcTEx/QcBRXvd2RyiZqCIupe22EhKXZiTSwk75VXq4iqcIwm5rr8LFvEQNBn5SyqjLLf iWrQxi/Qe7po8jJKQcMkGy4V/CZS+3VFdXT/AMmk6fA+GzBDbyNZkw+KfPLDpT+ITYpSjy +J4c1f21mdCOWD2ikEhS6PcYOB4dE98= X-Envelope-To: alexei.starovoitov@gmail.com X-Envelope-To: bpf@vger.kernel.org X-Envelope-To: kernel-team@cloudflare.com X-Envelope-To: linux-kernel@vger.kernel.org X-Envelope-To: llvm@lists.linux.dev Message-ID: <80b405c5-4bab-4364-ba32-e3f6ab9a5d57@linux.dev> Date: Mon, 20 May 2024 14:31:59 -0700 Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: bpftool does not print full names with LLVM 17 and newer Content-Language: en-GB To: Ivan Babrou , Alexei Starovoitov Cc: bpf , kernel-team , linux-kernel , clang-built-linux References: X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yonghong Song In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT On 5/20/24 12:44 PM, Ivan Babrou wrote: > On Fri, May 17, 2024 at 4:33 PM Alexei Starovoitov > wrote: >> On Fri, May 17, 2024 at 2:51 PM Ivan Babrou wrote: >>> Hello, >>> >>> We recently bumped LLVM used for bpftool compilation from 15 to 18 and >>> our alerting system notified us about some unknown bpf programs. It >>> turns out, the names were truncated to 15 chars, whereas before they >>> were longer. >>> >>> After some investigation, I was able to see that the following code: >>> >>> diff --git a/src/common.c b/src/common.c >>> index 958e92a..ac38506 100644 >>> --- a/src/common.c >>> +++ b/src/common.c >>> @@ -435,7 +435,9 @@ void get_prog_full_name(const struct >>> bpf_prog_info *prog_info, int prog_fd, >>> if (!prog_btf) >>> goto copy_name; >>> >>> + printf("[0] finfo.type_id = %x\n", finfo.type_id); >>> func_type = btf__type_by_id(prog_btf, finfo.type_id); >>> + printf("[1] finfo.type_id = %x\n", finfo.type_id); >>> if (!func_type || !btf_is_func(func_type)) >>> goto copy_name; >>> >>> When ran under gdb, shows: >>> >>> (gdb) b common.c:439 >>> Breakpoint 1 at 0x16859: file common.c, line 439. >>> >>> (gdb) r >>> 3403: tracing [0] finfo.type_id = 0 >>> >>> Breakpoint 1, get_prog_full_name (prog_info=0x7fffffffe160, >>> prog_fd=3, name_buff=0x7fffffffe030 "", buff_len=128) at common.c:439 >>> 439 func_type = btf__type_by_id(prog_btf, finfo.type_id); >>> (gdb) print finfo >>> $1 = {insn_off = 0, type_id = 1547} >>> >>> >>> Notice that finfo.type_id is printed as zero, but in gdb it is in fact 1547. >>> >>> Disassembly difference looks like this: >>> >>> - 8b 75 cc mov -0x34(%rbp),%esi >>> - e8 47 8d 02 00 call 3f5b0 >>> + 31 f6 xor %esi,%esi >>> + e8 a9 8c 02 00 call 3f510 >>> >>> This can be avoided if one removes "const" during finfo initialization: >>> >>> const struct bpf_func_info finfo = {}; >>> >>> This seems like a pretty annoying miscompilation, and hopefully >>> there's a way to make clang complain about this loudly, but that's >>> outside of my expertise. There might be other places like this that we >>> just haven't noticed yet. >>> >>> I can send a patch to fix this particular issue, but I'm hoping for a >>> more comprehensive approach from people who know better. >> Wow. Great catch. Please send a patch to fix bpftool and, >> I agree, llvm should be warning about such footgun, >> but the way ptr_to_u64() is written is probably silencing it. >> We probably should drop 'const' from it: >> static inline __u64 ptr_to_u64(const void *ptr) >> >> and maybe add a flavor of ptr_to_u64 with extra check >> that the arg doesn't have a const modifier. >> __builtin_types_compatible_p(typeof(ptr), void *) >> should do the trick. > In bpftool there's just two call sites that are unhappy if I remove > "const" in the arguments: > > * this problematic one > * "GPL" literal passed > > I'll send the patch to drop "const" from the struct initialization Yes, this should work as we discussed earlier. Thanks! > today or tomorrow (it works great in our internal build), but I'll > leave the bigger change to you. There seem to be many places in libbpf > and I'm far from being a C expert to drive this change. As discussed in below link. It is not easy for compiler to deduce whether an undefined behavior is triggered or not. The additional const_ptr_to_u64() serves the purpose to force patch author and reviewer to double check whether the 'u64' value may eventually invalidate 'const' property or not. > > I managed to bisect clang to find the commit that introduced the change: > > * https://github.com/llvm/llvm-project/commit/0b2d5b967d98 > > I also mentioned the commit author and they have some ideas about > UBSAN catching this (it doesn't in the current state): > > * https://mastodon.ivan.computer/@mastodon/112465898861074834 >