Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 16 Jun 2021 08:03:28 +0800
From:   Jisheng Zhang <jszhang3@mail.ustc.edu.cn>
To:     Alex Ghiti <alex@ghiti.fr>
Cc:     Andreas Schwab <schwab@linux-m68k.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Luke Nelson <luke.r.nels@gmail.com>,
        Xi Wang <xi.wang@gmail.com>, linux-riscv@lists.infradead.org,
        linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com,
        netdev@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH] riscv: Ensure BPF_JIT_REGION_START aligned with PMD
 size
Message-ID: <20210616080328.6548e762@xhacker>
In-Reply-To: <ab536c78-ba1c-c65c-325a-8f9fba6e9d46@ghiti.fr>
References: <20210330022144.150edc6e@xhacker>
        <20210330022521.2a904a8c@xhacker>
        <87o8ccqypw.fsf@igel.home>
        <20210612002334.6af72545@xhacker>
        <87bl8cqrpv.fsf@igel.home>
        <20210614010546.7a0d5584@xhacker>
        <87im2hsfvm.fsf@igel.home>
        <20210615004928.2d27d2ac@xhacker>
        <ab536c78-ba1c-c65c-325a-8f9fba6e9d46@ghiti.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, 15 Jun 2021 20:54:19 +0200
Alex Ghiti <alex@ghiti.fr> wrote:

> Hi Jisheng,

Hi Alex,

>=20
> Le 14/06/2021 =C3=A0 18:49, Jisheng Zhang a =C3=A9crit=C2=A0:
> > From: Jisheng Zhang <jszhang@kernel.org>
> >=20
> > Andreas reported commit fc8504765ec5 ("riscv: bpf: Avoid breaking W^X")
> > breaks booting with one kind of config file, I reproduced a kernel panic
> > with the config:
> >=20
> > [    0.138553] Unable to handle kernel paging request at virtual addres=
s ffffffff81201220
> > [    0.139159] Oops [#1]
> > [    0.139303] Modules linked in:
> > [    0.139601] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.13.0-rc5-def=
ault+ #1
> > [    0.139934] Hardware name: riscv-virtio,qemu (DT)
> > [    0.140193] epc : __memset+0xc4/0xfc
> > [    0.140416]  ra : skb_flow_dissector_init+0x1e/0x82
> > [    0.140609] epc : ffffffff8029806c ra : ffffffff8033be78 sp : ffffff=
e001647da0
> > [    0.140878]  gp : ffffffff81134b08 tp : ffffffe001654380 t0 : ffffff=
ff81201158
> > [    0.141156]  t1 : 0000000000000002 t2 : 0000000000000154 s0 : ffffff=
e001647dd0
> > [    0.141424]  s1 : ffffffff80a43250 a0 : ffffffff81201220 a1 : 000000=
0000000000
> > [    0.141654]  a2 : 000000000000003c a3 : ffffffff81201258 a4 : 000000=
0000000064
> > [    0.141893]  a5 : ffffffff8029806c a6 : 0000000000000040 a7 : ffffff=
ffffffffff
> > [    0.142126]  s2 : ffffffff81201220 s3 : 0000000000000009 s4 : ffffff=
ff81135088
> > [    0.142353]  s5 : ffffffff81135038 s6 : ffffffff8080ce80 s7 : ffffff=
ff80800438
> > [    0.142584]  s8 : ffffffff80bc6578 s9 : 0000000000000008 s10: ffffff=
ff806000ac
> > [    0.142810]  s11: 0000000000000000 t3 : fffffffffffffffc t4 : 000000=
0000000000
> > [    0.143042]  t5 : 0000000000000155 t6 : 00000000000003ff
> > [    0.143220] status: 0000000000000120 badaddr: ffffffff81201220 cause=
: 000000000000000f
> > [    0.143560] [<ffffffff8029806c>] __memset+0xc4/0xfc
> > [    0.143859] [<ffffffff8061e984>] init_default_flow_dissectors+0x22/0=
x60
> > [    0.144092] [<ffffffff800010fc>] do_one_initcall+0x3e/0x168
> > [    0.144278] [<ffffffff80600df0>] kernel_init_freeable+0x1c8/0x224
> > [    0.144479] [<ffffffff804868a8>] kernel_init+0x12/0x110
> > [    0.144658] [<ffffffff800022de>] ret_from_exception+0x0/0xc
> > [    0.145124] ---[ end trace f1e9643daa46d591 ]---
> >=20
> > After some investigation, I think I found the root cause: commit
> > 2bfc6cd81bd ("move kernel mapping outside of linear mapping") moves
> > BPF JIT region after the kernel:
> >=20
> > The &_end is unlikely aligned with PMD size, so the front bpf jit
> > region sits with part of kernel .data section in one PMD size mapping.
> > But kernel is mapped in PMD SIZE, when bpf_jit_binary_lock_ro() is
> > called to make the first bpf jit prog ROX, we will make part of kernel
> > .data section RO too, so when we write to, for example memset the
> > .data section, MMU will trigger a store page fault. =20
>=20
> Good catch, we make sure no physical allocation happens between _end and=
=20
> the next PMD aligned address, but I missed this one.
>=20
> >=20
> > To fix the issue, we need to ensure the BPF JIT region is PMD size
> > aligned. This patch acchieve this goal by restoring the BPF JIT region
> > to original position, I.E the 128MB before kernel .text section. =20
>=20
> But I disagree with your solution: I made sure modules and BPF programs=20
> get their own virtual regions to avoid worst case scenario where one=20
> could allocate all the space and leave nothing to the other (we are=20
> limited to +- 2GB offset). Why don't just align BPF_JIT_REGION_START to=20
> the next PMD aligned address?

Originally, I planed to fix the issue by aligning BPF_JIT_REGION_START, but
IIRC, BPF experts are adding (or have added) "Calling kernel functions from=
 BPF"
feature, there's a risk that BPF JIT region is beyond the 2GB of module reg=
ion:

------
module
------
kernel
------
BPF_JIT

So I made this patch finally. In this patch, we let BPF JIT region sit
between module and kernel.

To address "make sure modules and BPF programs get their own virtual region=
s",
what about something as below (applied against this patch)?

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgta=
ble.h
index 380cd3a7e548..da1158f10b09 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -31,7 +31,7 @@
 #define BPF_JIT_REGION_SIZE	(SZ_128M)
 #ifdef CONFIG_64BIT
 #define BPF_JIT_REGION_START	(BPF_JIT_REGION_END - BPF_JIT_REGION_SIZE)
-#define BPF_JIT_REGION_END	(MODULES_END)
+#define BPF_JIT_REGION_END	(PFN_ALIGN((unsigned long)&_start))
 #else
 #define BPF_JIT_REGION_START	(PAGE_OFFSET - BPF_JIT_REGION_SIZE)
 #define BPF_JIT_REGION_END	(VMALLOC_END)
@@ -40,7 +40,7 @@
 /* Modules always live before the kernel */
 #ifdef CONFIG_64BIT
 #define MODULES_VADDR	(PFN_ALIGN((unsigned long)&_end) - SZ_2G)
-#define MODULES_END	(PFN_ALIGN((unsigned long)&_start))
+#define MODULES_END	(BPF_JIT_REGION_END)
 #endif
=20


>=20
> Again, good catch, thanks,
>=20
> Alex
>=20
> >=20
> > Reported-by: Andreas Schwab <schwab@linux-m68k.org>
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> >   arch/riscv/include/asm/pgtable.h | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >=20
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/=
pgtable.h
> > index 9469f464e71a..380cd3a7e548 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -30,9 +30,8 @@
> >  =20
> >   #define BPF_JIT_REGION_SIZE	(SZ_128M)
> >   #ifdef CONFIG_64BIT
> > -/* KASLR should leave at least 128MB for BPF after the kernel */
> > -#define BPF_JIT_REGION_START	PFN_ALIGN((unsigned long)&_end)
> > -#define BPF_JIT_REGION_END	(BPF_JIT_REGION_START + BPF_JIT_REGION_SIZE)
> > +#define BPF_JIT_REGION_START	(BPF_JIT_REGION_END - BPF_JIT_REGION_SIZE)
> > +#define BPF_JIT_REGION_END	(MODULES_END)
> >   #else
> >   #define BPF_JIT_REGION_START	(PAGE_OFFSET - BPF_JIT_REGION_SIZE)
> >   #define BPF_JIT_REGION_END	(VMALLOC_END)
> >  =20