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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 48BD2C77B7A for ; Wed, 7 Jun 2023 21:04:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236271AbjFGVEs (ORCPT ); Wed, 7 Jun 2023 17:04:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34420 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236208AbjFGVE1 (ORCPT ); Wed, 7 Jun 2023 17:04:27 -0400 Received: from tiger.tulip.relay.mailchannels.net (tiger.tulip.relay.mailchannels.net [23.83.218.248]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6AFCC2710 for ; Wed, 7 Jun 2023 14:04:08 -0700 (PDT) X-Sender-Id: dreamhost|x-authsender|kjlx@templeofstupid.com Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id BFFA276174C for ; Wed, 7 Jun 2023 21:04:03 +0000 (UTC) Received: from pdx1-sub0-mail-a233.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 48864761754 for ; Wed, 7 Jun 2023 21:04:03 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1686171843; a=rsa-sha256; cv=none; b=UIanYHls8B1e2zdz4kLP9AVb1exxMHHIcOo4U/FsoOlOcK/NzPsib03OO4n1ZplkSsunGt 8Uf3S8QqFGSejPSUiX+AslpsXTRSnna7GDyjXl1GT+zSouTX1IcQWHVDaPqZJZvSfpvUyh vmguttadfmRqtZ47qFAv+aIIU1IyWKetaoorWL9qPO30pEa7j6AAn7501c/DOwqQrPyT+G rcShq7ik5nDOXmVqlOs/RFG6M0rkeaQoA0sl4Tgvbf/5sakqOqbbPO9v3cue5VJLp+rROC f14ZXWJL55fjPOF4oMuYZOOhhG2bDPhHUMW9jv7UuegxvwI5dJPHRPa1zBmW5Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1686171843; 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:dkim-signature; bh=VgEDVaD96PpiQVvSuuPolddYls8rHIqf3JryVYxh20Q=; b=n+Y6tEvJcehfkF8g02ESIPiF2PdFTM6xWdo17HhQ5XPhi0JIlHx2hAcPjnm/s4oG6M43a+ U8f6F4O8Tq+NhTQEkR4WPCtgmQ+o2IIuUXt635j5it2USnfnHcR53/1acb9ybq/Wu145kY p9ipe+rdt4kzB/5d892SVAwXPxtsfFm8C2I7Bow7B5oMg0mEBT+FXJcfBSmO4KqjmtAW8o hQPDMvK1QlpafaKi0R43qdBWQSs3MYiIUycP10935BsDwStmKFxF0iRTb4OgYnskns16nX OpCAz6hXDCWAuLg7XmSp0SA1V1HR3aksa8l9xgDge/y5Yxu/WHmf+a3VXZAslA== ARC-Authentication-Results: i=1; rspamd-6f5cfd578c-p4nmq; auth=pass smtp.auth=dreamhost smtp.mailfrom=kjlx@templeofstupid.com X-Sender-Id: dreamhost|x-authsender|kjlx@templeofstupid.com X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|kjlx@templeofstupid.com X-MailChannels-Auth-Id: dreamhost X-Shelf-Shelf: 2063dcb93d50f266_1686171843564_4165597997 X-MC-Loop-Signature: 1686171843564:2785457679 X-MC-Ingress-Time: 1686171843564 Received: from pdx1-sub0-mail-a233.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.103.24.122 (trex/6.8.1); Wed, 07 Jun 2023 21:04:03 +0000 Received: from kmjvbox (c-73-93-64-36.hsd1.ca.comcast.net [73.93.64.36]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: kjlx@templeofstupid.com) by pdx1-sub0-mail-a233.dreamhost.com (Postfix) with ESMTPSA id 4Qc0FZ3DYQzmL for ; Wed, 7 Jun 2023 14:04:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=templeofstupid.com; s=dreamhost; t=1686171842; bh=VgEDVaD96PpiQVvSuuPolddYls8rHIqf3JryVYxh20Q=; h=Date:From:To:Cc:Subject:Content-Type:Content-Transfer-Encoding; b=hu3WDKEe0rcjosyx/zlIvFagh5mtaQx+6schtl4mekrXSgPd7bOSiVY8mAh8QOBBf Nm0foHzaCpmM55Q/5IlJM8lEc8qkSkfHHxXjZTGvtIgVd5W3ui+YnPY2vYxXKyE+is I0qPQf7eBPKlSSw/xU32dSqIGhvdqG0CptDTp8TA= Received: from johansen (uid 1000) (envelope-from kjlx@templeofstupid.com) id e005f by kmjvbox (DragonFly Mail Agent v0.12); Wed, 07 Jun 2023 14:04:01 -0700 Date: Wed, 7 Jun 2023 14:04:01 -0700 From: Krister Johansen To: Alexei Starovoitov Cc: bpf , Alexei Starovoitov , Daniel Borkmann , John Fastabend , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , KP Singh , Stanislav Fomichev , Hao Luo , Jiri Olsa , "David S. Miller" , Jakub Kicinski , Jesper Dangaard Brouer , Nathan Chancellor , Nick Desaulniers , Tom Rix , LKML , Network Development , clang-built-linux , stable Subject: Re: [PATCH bpf] bpf: search_bpf_extables should search subprogram extables Message-ID: <20230607210401.GB2023@templeofstupid.com> References: <20230605164955.GA1977@templeofstupid.com> <20230606004139.GE1977@templeofstupid.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 05, 2023 at 06:31:57PM -0700, Alexei Starovoitov wrote: > On Mon, Jun 5, 2023 at 5:46 PM Krister Johansen wrote: > > With your comments in mind, I took > > another look at the ksym fields in the aux structs. I have this in the > > main program: > > > > ksym = { > > start = 18446744072638420852, > > end = 18446744072638423040, > > name = <...> > > lnode = { > > next = 0xffff88d9c1065168, > > prev = 0xffff88da91609168 > > }, > > tnode = { > > node = {{ > > __rb_parent_color = 18446613068361611640, > > rb_right = 0xffff88da91609178, > > rb_left = 0xffff88d9f0c5a578 > > }, { > > __rb_parent_color = 18446613068361611664, > > rb_right = 0xffff88da91609190, > > rb_left = 0xffff88d9f0c5a590 > > }} > > }, > > prog = true > > }, > > > > and this in the func[0] subprogram: > > > > ksym = { > > start = 18446744072638420852, > > end = 18446744072638423040, > > name = <...> > > lnode = { > > next = 0xffff88da91609168, > > prev = 0xffffffff981f8990 > > }, > > tnode = { > > node = {{ > > __rb_parent_color = 18446613068361606520, > > rb_right = 0x0, > > rb_left = 0x0 > > }, { > > __rb_parent_color = 18446613068361606544, > > rb_right = 0x0, > > rb_left = 0x0 > > }} > > }, > > prog = true > > }, > > > > That sure looks like func[0] is a leaf in the rbtree and the main > > program is an intermediate node with leaves. If that's the case, then > > bpf_prog_ksym_find may have found the main program instead of the > > subprogram. In that case, do you think it's better to skip the main > > program's call to bpf_prog_ksym_set_addr() if it has subprograms instead > > of searching for subprograms if the main program is found? > > I see. > Looks like we're doing double bpf_prog_kallsyms_add(). > First in in jit_subprogs(): > for (i = 0; i < env->subprog_cnt; i++) { > bpf_prog_lock_ro(func[i]); > bpf_prog_kallsyms_add(func[i]); > } > and then again: > bpf_prog_kallsyms_add(prog); > in bpf_prog_load(). > > because func[0] is the main prog. > > We are also doing double bpf_prog_lock_ro() for main prog, > but that's not causing harm. > > The fix is probably just this: > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 1e38584d497c..89266dac9c12 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -17633,7 +17633,7 @@ static int jit_subprogs(struct bpf_verifier_env *env) > /* finally lock prog and jit images for all functions and > * populate kallsysm > */ > - for (i = 0; i < env->subprog_cnt; i++) { > + for (i = 1; i < env->subprog_cnt; i++) { > bpf_prog_lock_ro(func[i]); > bpf_prog_kallsyms_add(func[i]); > } This will cause the oops to always occur, because func[0] has a extable entry when jit_subporgs() completes, but prog->aux doesn't. jit_subprogs also sets prog->bpf_func which prevents the other copy of the main program from getting jit'd, and consequently getting an extable assigned. There are probably a few options to fix: 1. skip the bpf_prog_kallsyms_add in bpf_prog_load if the program being loaded has subprograms 2. check extables when searching to see if they're NULL and if the subprogram has one instead 3. copy the main program's extable back to prog->aux I'll send out a v2 here shortly that includes the selftest you requested. It takes approach #3, which is also a 1-line change. -K