Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c26de68d-4a56-03a0-2625-25c7e2997d45@meta.com>
Date:   Mon, 19 Jun 2023 11:17:08 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.12.0
Subject: Re: [PATCH bpf] bpf/btf: Accept function names that contain dots
To:     Florent Revest <revest@chromium.org>,
        Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        llvm@lists.linux.dev, martin.lau@linux.dev, ast@kernel.org,
        daniel@iogearbox.net, andrii@kernel.org, song@kernel.org,
        yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org,
        nathan@kernel.org, ndesaulniers@google.com, trix@redhat.com,
        stable@vger.kernel.org
References: <20230615145607.3469985-1-revest@chromium.org>
 <CAEf4BzbjCt3tKJ40tg12rMjCLXrm7UoGuOdC62vGnpTTt8-buw@mail.gmail.com>
 <CABRcYmK=yXDumZj3tdW7341+sSV1zmZw1UpQkfSF6RFgnBQjew@mail.gmail.com>
Content-Language: en-US
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <CABRcYmK=yXDumZj3tdW7341+sSV1zmZw1UpQkfSF6RFgnBQjew@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?V0wyQ3NVbzRjMHd3VXdPbGl5UDEwckh4bjE0dWhQc1hVS3J0a21hUmk2eld5?=
 =?utf-8?B?RzJ2NXpybisxTC9LdmVKYlNMNHI2RExWOGNsamlkRXp0OVJDRGRDRTZIcEY5?=
 =?utf-8?B?M1U4K0VzMUl1dVU5RFVQT1FWSHRNVElFU3hTdE1KZmhZVTlYK1d2bGFrNWpY?=
 =?utf-8?B?Z0syTTlPOFJKbVA0bVhWNFZ3emtBQ2dwN0w2dUY2U0x0MWNUSG4wUlJROWdJ?=
 =?utf-8?B?OEl3NE8zZ2VJS1BINFVVanVOTUxvQm1Id2VadU5XMW9PTGFoaEJ6RGVkeGxR?=
 =?utf-8?B?WkxYU0xUaDhaM1NIcUswYXRSZHFQWW1sM1BRNEdrdm5BL1BmalFjWk9UcW82?=
 =?utf-8?B?SmZJdWE1M1JwK0czdjdRcVBCNUJqMm12a0p0dUVzVmw5ZnZQY09DRzl4N25X?=
 =?utf-8?B?Y3lMUVRSMkNWN1VMYnU5R2ZlRnEzWURDKzRtYkRkNzMzaS85Y0FIQjg4aTVI?=
 =?utf-8?B?Zk5jYVdmb3JKRXFPSW9GbnU5NVNFZkRFUXNKbzl6UEwwVGd6b1lwT2FCaWdz?=
 =?utf-8?B?QVRpc1pzRVFpTlVFR3ovVEVXcWd2QTlXblpOOGJyZUdPeXBYUnFBZ1ZtaERP?=
 =?utf-8?B?czNCa1VSS0RHUVREQzJHZEJlaXNLWTI3c3h2VUdQbUp2VDhnMWNHN1NOMXZ0?=
 =?utf-8?B?YVRHcytZUEtBTW1nQjIvbVUyTFRVSzErYmZZd3J3dlFaYXpTKzJiR21lWW1N?=
 =?utf-8?B?NnhyOEdRZUlXT20rbXF6TnVOY2dTR2RHUGJ3OHRhYVQ2NThFaTdRUXdMNjJX?=
 =?utf-8?B?TnVDRC83c0QxMXlpSlJDU1M2N2lZK1dzbmVDR25NRjFOMkNrbjFvQm96eU8w?=
 =?utf-8?B?RFNUU3FWd1I0TSt1a05MS1BvY1dpMTg5MFhJRS8vSjNwbVVGbG5pREZrTmpO?=
 =?utf-8?B?eUIxUzRINGxTZFRrVWdXZThpSVNnVkNuekNNTFpNM050NEE0anNUeWJMdlpZ?=
 =?utf-8?B?YnVEZUE1cURMRkhXbEt2SGUyV09oUk1KUUUwN0s5dHVSSGRZUHNWY2YzYmJz?=
 =?utf-8?B?eTc4bDBKQ3B6MTdUK3Y3eTlqdS9vSVdPSTg2T3ZIZm5PZTlpU24zamNNczRU?=
 =?utf-8?B?TVl4REEzRjcwTnhseW8zcThCTE1lcWFtWit5ay9KbEVxQmxFbDluUjBNYzlZ?=
 =?utf-8?B?L3d6S1lIYkorU1BYVElNU1pEcm9Qd2NQdUtzdjZmb0dLdmtsVThhaHBDejhq?=
 =?utf-8?B?Y2xHZy9ZTXlObjVsWVN6Z3lHMGpJdWtqdW1ZcWtmL09LRWM1Y2UwVFlXUXQr?=
 =?utf-8?B?ZnJrVk1HT1pMb2VyY0VXN0lrSWVuR2lWcmtoOTRSUGRuWWhhc1ZYNkk1VVlo?=
 =?utf-8?B?TUx5dmNsRG1YcGlDS0YrcUI2VHlxbW5RSjdmcjRkeGVmUjFNbFpmdlZzc2hQ?=
 =?utf-8?B?WlcvM1Y3MnF6c1IvTGY0dC9GbURQb1ZSSHBDN0tkcklsLzFjYVBJQnpudk9m?=
 =?utf-8?B?T2dvMlFlWjVPUTVCdHpxWG0wR21LeEpVcUxoZ1BPZlc4aDQxVzJOalpYYTRu?=
 =?utf-8?B?TndNRjBDTk5kSUt1K3JHbFZMaFEzL095S3pGaU82eFpFRmk4ekswZ09sSmU1?=
 =?utf-8?B?Mm5pTWtXUm8zU2VtT3FnOXhySGNJTitPRXpyaldtSHNoTXE3N3JKZEtEQ00r?=
 =?utf-8?B?Z1d3bVpQUnV0WEczR08zQzNhSjdCelBGVlpxemExTXRVTTYxbXJhK3g3YTRS?=
 =?utf-8?B?ekxaTk1kMzFtdEJOR1hIeVREM285a0R4Wnl0d1N5ekFrSTllQk5ZNkNDVnNO?=
 =?utf-8?B?d3dqaUd3MEJkdVFiaEIvU3N1K2ZwREJXelFOZkZad2Q0UTlibkM5TDVOM2p1?=
 =?utf-8?B?Mm9YSUdFdnozOVBYR3B2L3lQaTFiWGlkcWpWTWt0cnJqTHZ6cGkwSGlWZjhQ?=
 =?utf-8?B?NEhySm9ZZERVeU45dVpzQmM3cXRxODE2aHFhNWR5UktxalRWTk00ZmMwVzAz?=
 =?utf-8?B?ZUhmZXk4UkN2WE1kWlFIZ1A2bGdNbHRzaDBTMFcwWWJxR2MzQ1MrT3lFcmtE?=
 =?utf-8?B?bzVMd2pVWDlKUS9sbnA0UzdtajFZTFRDRTd5ZXo0bVZKKzJiZ0xJVUkxalJ1?=
 =?utf-8?B?VmZYYU16NmxLZ2NubVdhazFXUzBQQ1dZcEpDNnVJVkRsTmpxZEY0Vm1KcW9G?=
 =?utf-8?B?Q1JsNmJXaUgxV2JzZGF2OWlvazR3RG5NN3pRLzhoaE9HbG9jTUZJVTcxeDZJ?=
 =?utf-8?B?NkE9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 782510dc-1f95-43eb-2202-08db70f16815
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jun 2023 18:17:14.0312
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iWKOVp4PX0NJijqb5wqdfnod8283qujngzlwEwhOrQuvtQIro1D65/y3RBW/Gqab
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR15MB5509
X-Proofpoint-GUID: GokTkZ2IQAVECOiOtQIn_8QRzBbLSmJh
X-Proofpoint-ORIG-GUID: GokTkZ2IQAVECOiOtQIn_8QRzBbLSmJh
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-06-19_11,2023-06-16_01,2023-05-22_02
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/19/23 7:03 AM, Florent Revest wrote:
> On Fri, Jun 16, 2023 at 6:57 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Thu, Jun 15, 2023 at 7:56 AM Florent Revest <revest@chromium.org> wrote:
>>>
>>> When building a kernel with LLVM=1, LLVM_IAS=0 and CONFIG_KASAN=y, LLVM
>>> leaves DWARF tags for the "asan.module_ctor" & co symbols. In turn,
>>> pahole creates BTF_KIND_FUNC entries for these and this makes the BTF
>>> metadata validation fail because they contain a dot.
>>>
>>> In a dramatic turn of event, this BTF verification failure can cause
>>> the netfilter_bpf initialization to fail, causing netfilter_core to
>>> free the netfilter_helper hashmap and netfilter_ftp to trigger a
>>> use-after-free. The risk of u-a-f in netfilter will be addressed
>>> separately but the existence of "asan.module_ctor" debug info under some
>>> build conditions sounds like a good enough reason to accept functions
>>> that contain dots in BTF.
>>
>> I don't see much harm in allowing dots. There are also all those .isra
>> and other modifications to functions that we currently don't have in
>> BTF, but with the discussions about recording function addrs we might
>> eventually have those as well. So:
>>
>> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> Thanks Andrii! :)
> 
>>> Cc: stable@vger.kernel.org
>>> Fixes: 1dc92851849c ("bpf: kernel side support for BTF Var and DataSec")
> 
> So do you think these trailers should be kept ? I suppose we can
> either see this as a "new feature" to accommodate .isra that should go
> through bpf-next or as a bug fix that goes through bpf and gets
> backported to stable (without this, BTF wouldn't work on old kernels
> built under a new clang and with LLVM_IAS=0 and CONFIG_KASAN=y so this
> sounds like a legitimate bug fix to me, I just wanted to double check)

How many people really build the kernel with
    LLVM=1 LLVM_IAS=0
which uses clang compiler ans gcc 'as'.
I think distro most likely won't do this if they intend to
build the kernel with clang.

Note that
    LLVM=1
implies to use both clang compiler and clang assembler.

Using clang17 and 'LLVM=1 LLVM_IAS=0', with latest bpf-next,
I actually hit some build errors like:

/tmp/video-bios-59fa52.s: Assembler messages:
/tmp/video-bios-59fa52.s:4: Error: junk at end of line, first 
unrecognized character is `"'
/tmp/video-bios-59fa52.s:4: Error: file number less than one
/tmp/video-bios-59fa52.s:5: Error: junk at end of line, first 
unrecognized character is `"'
/tmp/video-bios-59fa52.s:6: Error: junk at end of line, first 
unrecognized character is `"'
/tmp/video-bios-59fa52.s:7: Error: junk at end of line, first 
unrecognized character is `"'
/tmp/video-bios-59fa52.s:8: Error: junk at end of line, first 
unrecognized character is `"'
/tmp/video-bios-59fa52.s:9: Error: junk at end of line, first 
unrecognized character is `"'
/tmp/video-bios-59fa52.s:10: Error: junk at end of line, first 
unrecognized character is `"'
/tmp/video-bios-59fa52.s:68: Error: junk at end of line, first 
unrecognized character is `"'
clang: error: assembler command failed with exit code 1 (use -v to see 
invocation)
make[4]: *** [/home/yhs/work/bpf-next/scripts/Makefile.build:252: 
arch/x86/realmode/rm/video-bios.o] Error 1
make[4]: *** Waiting for unfinished jobs....
/tmp/wakemain-88777c.s: Assembler messages:
/tmp/wakemain-88777c.s:4: Error: junk at end of line, first unrecognized 
character is `"'
/tmp/wakemain-88777c.s:4: Error: file number less than one
/tmp/wakemain-88777c.s:5: Error: junk at end of line, first unrecognized 
character is `"'
/tmp/wakemain-88777c.s:6: Error: junk at end of line, first unrecognized 
character is `"'
/tmp/wakemain-88777c.s:7: Error: junk at end of line, first unrecognized 
character is `"'
/tmp/wakemain-88777c.s:8: Error: junk at end of line, first unrecognized 
character is `"'
/tmp/wakemain-88777c.s:81: Error: junk at end of line, first 
unrecognized character is `"'
/tmp/wakemain-88777c.s:312: Error: junk at end of line, first 
unrecognized character is `"'
clang: error: assembler command failed with exit code 1 (use -v to see 
invocation)

Potentially because of my local gnu assembler 2.30-120.el8 won't work
with some syntax generated by clang. Mixing clang compiler and arbitrary
gnu assembler are not a good idea (see the above example). It might
work with close-to-latest gnu assembler.

To support function name like '<fname>.isra', some llvm work will be 
needed, and it may take some time.

So in my opinion, this patch is NOT a bug fix. It won't affect distro.
Whether we should backport to the old kernel, I am not sure whether it
is absolutely necessary as casual build can always remove LLVM_IAS=0 or
hack the kernel source itself.