Re: [PATCH bpf-next 1/1] arm64: bpf: Add BPF exception tables - Jean-Philippe Brucker

From: Jean-Philippe Brucker <jean-philippe@linaro.org>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Song Liu <song@kernel.org>,
	linux-arm-kernel@lists.infradead.org, bpf <bpf@vger.kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	zlim.lnx@gmail.com, Martin KaFai Lau <kafai@fb.com>,
	Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
	Andrii Nakryiko <andriin@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>
Subject: Re: [PATCH bpf-next 1/1] arm64: bpf: Add BPF exception tables
Date: Thu, 30 Jul 2020 10:28:53 +0200	[thread overview]
Message-ID: <20200730082853.GA1529030@myrica> (raw)
In-Reply-To: <4791872a-9f7e-1c1c-392c-8b68a13091e3@iogearbox.net>

On Wed, Jul 29, 2020 at 11:29:43PM +0200, Daniel Borkmann wrote:
> On 7/29/20 7:28 PM, Song Liu wrote:
> > On Tue, Jul 28, 2020 at 8:37 AM Jean-Philippe Brucker
> > <jean-philippe@linaro.org> wrote:
> > > 
> > > When a tracing BPF program attempts to read memory without using the
> > > bpf_probe_read() helper, the verifier marks the load instruction with
> > > the BPF_PROBE_MEM flag. Since the arm64 JIT does not currently recognize
> > > this flag it falls back to the interpreter.
> > > 
> > > Add support for BPF_PROBE_MEM, by appending an exception table to the
> > > BPF program. If the load instruction causes a data abort, the fixup
> > > infrastructure finds the exception table and fixes up the fault, by
> > > clearing the destination register and jumping over the faulting
> > > instruction.
> > > 
> > > To keep the compact exception table entry format, inspect the pc in
> > > fixup_exception(). A more generic solution would add a "handler" field
> > > to the table entry, like on x86 and s390.
> > > 
> > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > 
> > This patch looks good to me.
> > 
> > Acked-by: Song Liu <songliubraving@fb.com>
> 
> +1, applied, thanks a lot!
> 
> > It is possible to add a selftest for this? I thought about this a
> > little bit, but
> > didn't get a good idea.
> 
> Why not adding a test_verifier.c test case which calls into bpf_get_current_task()
> to fetch pointer to current and then read out some field via BPF_PROBE_MEM which
> should then succeed on x86/s390x/arm64 but be skipped on the other archs? Jean-Philippe,
> could you look into following up with such test case(s)?

Sure I'll take a look. Ilya also added a selftests to trigger exceptions
in https://lore.kernel.org/bpf/20200715233301.933201-5-iii@linux.ibm.com/
It's useful but I think it relies on the verifier not mandating NULL
checks for next-level pointers (they are ptr_ instead of ptr_or_null_),
which might change in the future. So I'm wondering if we can deliberately
access an invalid pointer with the help of bpf_test_run, and check that
the result is zero. 

Thanks,
Jean