Received: by 2002:a05:6358:3188:b0:123:57c1:9b43 with SMTP id q8csp9949859rwd; Wed, 21 Jun 2023 14:09:58 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ59wDZSYkKmmk35p7Du3bJ9P3MW9mXO6GiQIwjmO264FMOVCVmO3lNWLBny9uhDCGAPayZJ X-Received: by 2002:a17:90a:94cc:b0:247:ab52:d5d8 with SMTP id j12-20020a17090a94cc00b00247ab52d5d8mr6559135pjw.26.1687381798071; Wed, 21 Jun 2023 14:09:58 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1687381798; cv=none; d=google.com; s=arc-20160816; b=dyHljkZG/etdFoKQcsci50csyM/L5nPf355RyEjY8kgFsUtjtSVPIu0gJGMCS8FuiL cXXH1Lp/NKltKQKpMQXzU1+wo1mUjR38bG5wiR/tGIBpy4Gp2mGeZziVPNvOmka0pWkD 20kqt4qRgAiKfqYwcEmzWMTcp+BZuAodSs0MTs1b8eTiA6dH+qk+MRPLHI57gZAlgy8v Huk8eXYqzdwhVERNxhy3Ov8YG6nhzidCFsHtKBMqFGXzkSXqP0pYVfaEFCFwdsoMgimU HBqmp23Bkc+WoZ5KbQJSugWYqq99Hlw0+bHNjekSmz0xzprbZart5pk4X14ud1ywaSoY C/4g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version :dkim-signature; bh=NkYjPecj7jkh0eSl5nFN/psGSW4/CTbJ5xdbDB+Wop0=; b=JfbtTzsjF+CL89K9/zp4t6SN+8M4IPXCBe5XVhgGrgjRF88hobC/tDKKshmTJjXwNF 3AMDB3pHXfK0c59JJjjpn32za/poNenGNmiAMOOIymSQMJaQRkPmn4zEHCN695uRjLJf E70DDKoxMnR2HlKuR/pJm95t0VTZSt4Bty7utlh/2JWaCQ5hQAYEeAl6JVVfUyiQ8s+1 q4W/GhWQ7Y2jo+eXeSUSkKugTjDICYP87oCTI2NvN3eDpmi2oJOHOKf0Om3VrHju+EEY ox7bjKFE6dwjzy/bz+2gSgXR2SNVPL4N40Kvsmq9WiaK1wM0QrnoXyd4HMiiqacDfCbv pZ1A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=j8KrwTzp; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id m24-20020a17090b069800b0025027e0ad3dsi4914259pjz.81.2023.06.21.14.09.45; Wed, 21 Jun 2023 14:09:58 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=j8KrwTzp; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230206AbjFUU54 (ORCPT + 99 others); Wed, 21 Jun 2023 16:57:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37940 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230025AbjFUU5x (ORCPT ); Wed, 21 Jun 2023 16:57:53 -0400 Received: from mail-lf1-x134.google.com (mail-lf1-x134.google.com [IPv6:2a00:1450:4864:20::134]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B62681BD4; Wed, 21 Jun 2023 13:57:33 -0700 (PDT) Received: by mail-lf1-x134.google.com with SMTP id 2adb3069b0e04-4f8792d2e86so4076393e87.1; Wed, 21 Jun 2023 13:57:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687381052; x=1689973052; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=NkYjPecj7jkh0eSl5nFN/psGSW4/CTbJ5xdbDB+Wop0=; b=j8KrwTzp6jE/6uuK++JqQ9NzN6+z0609C5pLtnysgbCrHSZPdaBhTKnonmJ9utZCvX D06WB8IdHrMy2mmBmIm4MQkOq9UU/qcEGRj2dKDiMwGKHb5nDyc2z+kPZd6uqWagqBdu Lfwy7l7iX3xLeO5uSOLaSnDqqxWOKx2xY/kHXRJpfzTKUfXqpw41lwBcrvIdLI6cz24V rbkkB+lqSBRHJpNpauQwuRBo/7UqE2H3Iinx+cbnaWSM2A0f5eetSd/Pjv90kr6HVLYa 0PVxvuBT4y9Rluh6zz6Pw9dGXgI0iTtRgQXJ9+pCLiKCkHCQj1j3NhD7DzBTP+6zXsuX IqPw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687381052; x=1689973052; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=NkYjPecj7jkh0eSl5nFN/psGSW4/CTbJ5xdbDB+Wop0=; b=WRc4DnjzIYNqcm3S4QGOIoEH7z+wsMXRUIXzJgScmuY40307hi1SZoNGVj4jLZ1MTj 7srGzoii/wsrz4pP6h0ibxqCIzZSKTnh/7XWST81XF7PznbDOxp4T79zE43T5z32F2dw Y4braSvzd2u4npV+fMyTaOjcbbMbaahMBTck8UsJjZLX4HdkgXm2giJpQQghAa+u/o3p v09MSPhDZKxV1l+j9acnCw/JuaVKdJrhnFrJAy/YBzeK1MopEbxJgFFFG1hKEdb1j+PU Gb4XyXmwfGxJawSJJIXc8yB5ad5T+xCt78/n4gnBQXet1+gM9/hu2dnp/dtTQpoE45ed Rkow== X-Gm-Message-State: AC+VfDyPhMkEvnrLdhXobKDd1Sl8GGciraR2hd+pUQVBcnDvCBSfUCnv bfNBuVQ1J/ne11m+bg/tUKTpt9VNHDi+xLQaliC3mhu+EvyJsIoR8Ws= X-Received: by 2002:a05:6512:3985:b0:4f9:615f:14dd with SMTP id j5-20020a056512398500b004f9615f14ddmr477123lfu.11.1687381051562; Wed, 21 Jun 2023 13:57:31 -0700 (PDT) MIME-Version: 1.0 References: <20230619100121.27534-1-puranjay12@gmail.com> <20230619100121.27534-4-puranjay12@gmail.com> In-Reply-To: From: Puranjay Mohan Date: Wed, 21 Jun 2023 22:57:20 +0200 Message-ID: Subject: Re: [PATCH bpf-next v3 3/3] bpf, arm64: use bpf_jit_binary_pack_alloc To: Mark Rutland Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev, song@kernel.org, catalin.marinas@arm.com, bpf@vger.kernel.org, kpsingh@kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,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: X-Mailing-List: linux-kernel@vger.kernel.org Hi Mark, On Wed, Jun 21, 2023 at 5:31=E2=80=AFPM Mark Rutland = wrote: > > On Mon, Jun 19, 2023 at 10:01:21AM +0000, Puranjay Mohan wrote: > > Use bpf_jit_binary_pack_alloc for memory management of JIT binaries in > > ARM64 BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and R= X > > buffers. The JIT writes the program into the RW buffer. When the JIT is > > done, the program is copied to the final RX buffer > > with bpf_jit_binary_pack_finalize. > > > > Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for ARM64 > > JIT as these functions are required by bpf_jit_binary_pack allocator. > > > > Signed-off-by: Puranjay Mohan > > From a quick look, I don't beleive the I-cache maintenance is quite right= -- > explanation below. > > > @@ -1562,34 +1610,39 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf= _prog *prog) > > > > /* 3. Extra pass to validate JITed code. */ > > if (validate_ctx(&ctx)) { > > - bpf_jit_binary_free(header); > > prog =3D orig_prog; > > - goto out_off; > > + goto out_free_hdr; > > } > > > > /* And we're done. */ > > if (bpf_jit_enable > 1) > > bpf_jit_dump(prog->len, prog_size, 2, ctx.image); > > > > - bpf_flush_icache(header, ctx.image + ctx.idx); > > + bpf_flush_icache(ro_header, ctx.ro_image + ctx.idx); > > I think this is too early; we haven't copied the instructions into the > ro_header yet, so that still contains stale instructions. > > IIUC at the whole point of this is to pack multiple programs into shared = ROX > pages, and so there can be an executable mapping of the RO page at this p= oint, > and the CPU can fetch stale instructions throught that. > > Note that *regardless* of whether there is an executeable mapping at this= point > (and even if no executable mapping exists until after the copy), we at le= ast > need a data cache clean to the PoU *after* the copy (so fetches don't get= a > stale value from the PoU), and the I-cache maintenance has to happeon the= VA > the instrutions will be executed from (or VIPT I-caches can still contain= stale > instructions). Thanks for catching this, It is a big miss from my side. I was able to reproduce the boot issue in the other thread on my raspberry pi. I think it is connected to the wrong I-cache handling done by me. As you rightly pointed out: We need to do bpf_flush_icache() after copying the instructions to the ro_header or the CPU can run incorrect instructions. When I move the call to bpf_flush_icache() after bpf_jit_binary_pack_finalize() (this does the copy to ro_header), the boot issue is fixed. Would this change be enough to make this work or I would need to do more with the data cache as well to catch other edge cases? Thanks, Puranjay > > Thanks, > Mark. > > > > > if (!prog->is_func || extra_pass) { > > if (extra_pass && ctx.idx !=3D jit_data->ctx.idx) { > > pr_err_once("multi-func JIT bug %d !=3D %d\n", > > ctx.idx, jit_data->ctx.idx); > > - bpf_jit_binary_free(header); > > prog->bpf_func =3D NULL; > > prog->jited =3D 0; > > prog->jited_len =3D 0; > > + goto out_free_hdr; > > + } > > + if (WARN_ON(bpf_jit_binary_pack_finalize(prog, ro_header, > > + header))) { > > + /* ro_header has been freed */ > > + ro_header =3D NULL; > > + prog =3D orig_prog; > > goto out_off; > > } > > - bpf_jit_binary_lock_ro(header); > > } else { > > jit_data->ctx =3D ctx; > > - jit_data->image =3D image_ptr; > > + jit_data->ro_image =3D ro_image_ptr; > > jit_data->header =3D header; > > + jit_data->ro_header =3D ro_header; > > } > > - prog->bpf_func =3D (void *)ctx.image; > > + prog->bpf_func =3D (void *)ctx.ro_image; > > prog->jited =3D 1; > > prog->jited_len =3D prog_size; > > > > @@ -1610,6 +1663,14 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_= prog *prog) > > bpf_jit_prog_release_other(prog, prog =3D=3D orig_prog ? > > tmp : orig_prog); > > return prog; > > + > > +out_free_hdr: > > + if (header) { > > + bpf_arch_text_copy(&ro_header->size, &header->size, > > + sizeof(header->size)); > > + bpf_jit_binary_pack_free(ro_header, header); > > + } > > + goto out_off; > > } > > > > bool bpf_jit_supports_kfunc_call(void) > > @@ -1617,6 +1678,13 @@ bool bpf_jit_supports_kfunc_call(void) > > return true; > > } > > > > +void *bpf_arch_text_copy(void *dst, void *src, size_t len) > > +{ > > + if (!aarch64_insn_copy(dst, src, len)) > > + return ERR_PTR(-EINVAL); > > + return dst; > > +} > > + > > u64 bpf_jit_alloc_exec_limit(void) > > { > > return VMALLOC_END - VMALLOC_START; > > @@ -2221,3 +2289,27 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_p= oke_type poke_type, > > > > return ret; > > } > > + > > +void bpf_jit_free(struct bpf_prog *prog) > > +{ > > + if (prog->jited) { > > + struct arm64_jit_data *jit_data =3D prog->aux->jit_data; > > + struct bpf_binary_header *hdr; > > + > > + /* > > + * If we fail the final pass of JIT (from jit_subprogs), > > + * the program may not be finalized yet. Call finalize he= re > > + * before freeing it. > > + */ > > + if (jit_data) { > > + bpf_jit_binary_pack_finalize(prog, jit_data->ro_h= eader, > > + jit_data->header); > > + kfree(jit_data); > > + } > > + hdr =3D bpf_jit_binary_pack_hdr(prog); > > + bpf_jit_binary_pack_free(hdr, NULL); > > + WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog)); > > + } > > + > > + bpf_prog_unlock_free(prog); > > +} > > -- > > 2.40.1 > >