[PATCH] arm64: net: bpf: don't BUG() on large shifts

[PATCH] arm64: net: bpf: don't BUG() on large shifts

[Rabin Vincent]

Attempting to generate UBFM/SBFM instructions with shifts that can't be
encoded in the immediate fields of the opcodes leads to a trigger of a
BUG() in the instruction generation code.  As the ARMv8 ARM says: "The
shift amounts must be in the range 0 to one less than the register width
of the instruction, inclusive."  Make the JIT reject unencodable shifts
instead of crashing.

 ------------[ cut here ]------------
 kernel BUG at arch/arm64/kernel/insn.c:766!
 Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
 CPU: 0 PID: 669 Comm: insmod Not tainted 4.4.0-rc8+ #4
 PC is at aarch64_insn_gen_bitfield+0xcc/0xd4
 LR is at build_body+0x1000/0x2914
 ..
 Call trace:
 [<ffffffc00008c65c>] aarch64_insn_gen_bitfield+0xcc/0xd4
 [<ffffffc000096bfc>] build_body+0x1000/0x2914
 [<ffffffc000098590>] bpf_int_jit_compile+0x7c/0x1b4
 [<ffffffc000130d10>] bpf_prog_select_runtime+0x20/0xcc
 [<ffffffc0004afbac>] bpf_prepare_filter+0x3d8/0x3e8
 [<ffffffc0004afc30>] bpf_prog_create+0x74/0xa4
 [<ffffffbffc3de1d4>] test_bpf_init+0x1d4/0x748 [test_bpf]
 [<ffffffc00008293c>] do_one_initcall+0x90/0x1a8
 [<ffffffc000140c4c>] do_init_module+0x60/0x1c8
 [<ffffffc00011bdcc>] load_module+0x1554/0x1c98
 [<ffffffc00011c62c>] SyS_init_module+0x11c/0x140
 [<ffffffc000085cb0>] el0_svc_naked+0x24/0x28

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm64/net/bpf_jit_comp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index b162ad70effc..3f4f089a85c0 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -255,6 +255,7 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
 	const s32 imm = insn->imm;
 	const int i = insn - ctx->prog->insnsi;
 	const bool is64 = BPF_CLASS(code) == BPF_ALU64;
+	const int bits = is64 ? 64 : 32;
 	u8 jmp_cond;
 	s32 jmp_offset;
 
@@ -444,14 +445,20 @@ emit_bswap_uxt:
 		break;
 	case BPF_ALU | BPF_LSH | BPF_K:
 	case BPF_ALU64 | BPF_LSH | BPF_K:
+		if (imm < 0 || imm >= bits)
+			return -EINVAL;
 		emit(A64_LSL(is64, dst, dst, imm), ctx);
 		break;
 	case BPF_ALU | BPF_RSH | BPF_K:
 	case BPF_ALU64 | BPF_RSH | BPF_K:
+		if (imm < 0 || imm >= bits)
+			return -EINVAL;
 		emit(A64_LSR(is64, dst, dst, imm), ctx);
 		break;
 	case BPF_ALU | BPF_ARSH | BPF_K:
 	case BPF_ALU64 | BPF_ARSH | BPF_K:
+		if (imm < 0 || imm >= bits)
+			return -EINVAL;
 		emit(A64_ASR(is64, dst, dst, imm), ctx);
 		break;
 
-- 
2.6.4

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Alexei Starovoitov]

On Tue, Jan 05, 2016 at 06:39:03PM +0100, Rabin Vincent wrote:
> Attempting to generate UBFM/SBFM instructions with shifts that can't be
> encoded in the immediate fields of the opcodes leads to a trigger of a
> BUG() in the instruction generation code.  As the ARMv8 ARM says: "The
> shift amounts must be in the range 0 to one less than the register width
> of the instruction, inclusive."  Make the JIT reject unencodable shifts
> instead of crashing.
> 
>  ------------[ cut here ]------------
>  kernel BUG at arch/arm64/kernel/insn.c:766!
>  Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>  CPU: 0 PID: 669 Comm: insmod Not tainted 4.4.0-rc8+ #4
>  PC is at aarch64_insn_gen_bitfield+0xcc/0xd4
>  LR is at build_body+0x1000/0x2914
>  ..
>  Call trace:
>  [<ffffffc00008c65c>] aarch64_insn_gen_bitfield+0xcc/0xd4
>  [<ffffffc000096bfc>] build_body+0x1000/0x2914
>  [<ffffffc000098590>] bpf_int_jit_compile+0x7c/0x1b4
>  [<ffffffc000130d10>] bpf_prog_select_runtime+0x20/0xcc
>  [<ffffffc0004afbac>] bpf_prepare_filter+0x3d8/0x3e8
>  [<ffffffc0004afc30>] bpf_prog_create+0x74/0xa4
>  [<ffffffbffc3de1d4>] test_bpf_init+0x1d4/0x748 [test_bpf]
>  [<ffffffc00008293c>] do_one_initcall+0x90/0x1a8
>  [<ffffffc000140c4c>] do_init_module+0x60/0x1c8
>  [<ffffffc00011bdcc>] load_module+0x1554/0x1c98
>  [<ffffffc00011c62c>] SyS_init_module+0x11c/0x140
>  [<ffffffc000085cb0>] el0_svc_naked+0x24/0x28
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/arm64/net/bpf_jit_comp.c | 7 +++++++
>  1 file changed, 7 insertions(+)

this one is better to be addressed in verifier instead of eBPF JITs.
Please reject it in check_alu_op() instead.
Though this bug is arm64 only and doesn't affect x64, it's better
to reject such invalid programs, since shifts with large constants
can be only be created manually. llvm doesn't generate such things.

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Rabin Vincent]

On Tue, Jan 05, 2016 at 09:55:58AM -0800, Alexei Starovoitov wrote:
> this one is better to be addressed in verifier instead of eBPF JITs.
> Please reject it in check_alu_op() instead.

AFAICS the eBPF verifier is not called on the eBPF filters generated by
the BPF->eBPF conversion in net/core/filter.c, so performing this check
only in check_alu_op() will be insufficient.  So I think we'd need to
add this check to bpf_check_classic() too.  Or am I missing something?

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Alexei Starovoitov]

On Wed, Jan 06, 2016 at 09:31:27PM +0100, Rabin Vincent wrote:
> On Tue, Jan 05, 2016 at 09:55:58AM -0800, Alexei Starovoitov wrote:
> > this one is better to be addressed in verifier instead of eBPF JITs.
> > Please reject it in check_alu_op() instead.
> 
> AFAICS the eBPF verifier is not called on the eBPF filters generated by
> the BPF->eBPF conversion in net/core/filter.c, so performing this check
> only in check_alu_op() will be insufficient.  So I think we'd need to
> add this check to bpf_check_classic() too.  Or am I missing something?

correct. the check is needed in bpf_check_classic() too and I believe
it's ok to tighten it up in this case, since >32 shift is
invalid/undefined anyway. We can either accept it as nop or K&=31
or error. I think returning error is more user friendly long term, though
there is a small risk of rejecting previously loadable broken programs.

RE: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[David Laight]

From: Alexei Starovoitov
> Sent: 06 January 2016 22:13
> On Wed, Jan 06, 2016 at 09:31:27PM +0100, Rabin Vincent wrote:
> > On Tue, Jan 05, 2016 at 09:55:58AM -0800, Alexei Starovoitov wrote:
> > > this one is better to be addressed in verifier instead of eBPF JITs.
> > > Please reject it in check_alu_op() instead.
> >
> > AFAICS the eBPF verifier is not called on the eBPF filters generated by
> > the BPF->eBPF conversion in net/core/filter.c, so performing this check
> > only in check_alu_op() will be insufficient.  So I think we'd need to
> > add this check to bpf_check_classic() too.  Or am I missing something?
> 
> correct. the check is needed in bpf_check_classic() too and I believe
> it's ok to tighten it up in this case, since >32 shift is
> invalid/undefined anyway. We can either accept it as nop or K&=31
> or error. I think returning error is more user friendly long term, though
> there is a small risk of rejecting previously loadable broken programs.

Or replace with an assignment of zero?

	David

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Daniel Borkmann]

On 01/07/2016 12:07 PM, David Laight wrote:
> From: Alexei Starovoitov
>> Sent: 06 January 2016 22:13
>> On Wed, Jan 06, 2016 at 09:31:27PM +0100, Rabin Vincent wrote:
>>> On Tue, Jan 05, 2016 at 09:55:58AM -0800, Alexei Starovoitov wrote:
>>>> this one is better to be addressed in verifier instead of eBPF JITs.
>>>> Please reject it in check_alu_op() instead.
>>>
>>> AFAICS the eBPF verifier is not called on the eBPF filters generated by
>>> the BPF->eBPF conversion in net/core/filter.c, so performing this check
>>> only in check_alu_op() will be insufficient.  So I think we'd need to
>>> add this check to bpf_check_classic() too.  Or am I missing something?
>>
>> correct. the check is needed in bpf_check_classic() too and I believe
>> it's ok to tighten it up in this case, since >32 shift is
>> invalid/undefined anyway. We can either accept it as nop or K&=31
>> or error. I think returning error is more user friendly long term, though
>> there is a small risk of rejecting previously loadable broken programs.
>
> Or replace with an assignment of zero?

The question is what is less risky in terms of uabi. To reject such
filters with such K shift vals upfront in verifier, or to just allow
[0, reg_size - 1] values and handle outliers silently. I think both
might be possible, the latter just needs to be clearly specified in
the documentation somewhere. If we go for the latter, then probably
just rewriting that K value as masked one might seem better. Broken
programs might then still be loadable (and still be broken) ... afaik
in case of register (case of shifts with X) with large shift vals
ARM64 is doing 'modulo reg_size' implicitly.

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Rabin Vincent]

On Thu, Jan 07, 2016 at 01:48:48PM +0100, Daniel Borkmann wrote:
> The question is what is less risky in terms of uabi. To reject such
> filters with such K shift vals upfront in verifier, or to just allow
> [0, reg_size - 1] values and handle outliers silently. I think both
> might be possible, the latter just needs to be clearly specified in
> the documentation somewhere. If we go for the latter, then probably
> just rewriting that K value as masked one might seem better. Broken
> programs might then still be loadable (and still be broken) ... afaik
> in case of register (case of shifts with X) with large shift vals
> ARM64 is doing 'modulo reg_size' implicitly.

The case of what happens with such shifts with X is also already
architecture-specific, even when using the interpreters.  For example, the
following program returns 1 on ARM64 but 0 on ARM.

	BPF_STMT(BPF_LD | BPF_IMM, 1),
	BPF_STMT(BPF_LDX | BPF_IMM, 32),
	BPF_STMT(BPF_ALU | BPF_LSH | BPF_X, 0),
	BPF_STMT(BPF_RET | BPF_A, 0)

To start rejecting large K shifts in the verifier because they are
architecture-specific while continuing to allow the equally
architecture-specific large X shifts (because we can't verify them
statically) would be rather inconsistent.

If it is desired to enforce uniformity across architectures despite the
risk for subtly changing the behaviour of existing programs, then the
desired uniform semantics of these shifts should really be implemented
both for the K and X shifts, which would mean modifiying the interpreter
and the various arch JITs too.

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Daniel Borkmann]

On 01/08/2016 04:58 PM, Rabin Vincent wrote:
> On Thu, Jan 07, 2016 at 01:48:48PM +0100, Daniel Borkmann wrote:
>> The question is what is less risky in terms of uabi. To reject such
>> filters with such K shift vals upfront in verifier, or to just allow
>> [0, reg_size - 1] values and handle outliers silently. I think both
>> might be possible, the latter just needs to be clearly specified in
>> the documentation somewhere. If we go for the latter, then probably
>> just rewriting that K value as masked one might seem better. Broken
>> programs might then still be loadable (and still be broken) ... afaik
>> in case of register (case of shifts with X) with large shift vals
>> ARM64 is doing 'modulo reg_size' implicitly.
>
> The case of what happens with such shifts with X is also already
> architecture-specific, even when using the interpreters.  For example, the
> following program returns 1 on ARM64 but 0 on ARM.
>
> 	BPF_STMT(BPF_LD | BPF_IMM, 1),
> 	BPF_STMT(BPF_LDX | BPF_IMM, 32),
> 	BPF_STMT(BPF_ALU | BPF_LSH | BPF_X, 0),
> 	BPF_STMT(BPF_RET | BPF_A, 0)
>
> To start rejecting large K shifts in the verifier because they are
> architecture-specific while continuing to allow the equally
> architecture-specific large X shifts (because we can't verify them
> statically) would be rather inconsistent.

Hmm, yeah, agree with you that this would be inconsistent. Last time we
actually had this topic [1], I believe the consensus was that such BPF
programs are to be considered "undefined".

In that case, I think just masking the K value silently into its allowed
range for classic and eBPF might be better. It would eventually not be
uniform with regards to shifts where X is involved, but if we consider
it "undefined" behaviour, such uniformity is probably not needed.

> If it is desired to enforce uniformity across architectures despite the
> risk for subtly changing the behaviour of existing programs, then the
> desired uniform semantics of these shifts should really be implemented
> both for the K and X shifts, which would mean modifiying the interpreter
> and the various arch JITs too.

   [1] https://lkml.org/lkml/2015/12/4/148

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Alexei Starovoitov]

On Fri, Jan 08, 2016 at 05:44:42PM +0100, Daniel Borkmann wrote:
> On 01/08/2016 04:58 PM, Rabin Vincent wrote:
> >On Thu, Jan 07, 2016 at 01:48:48PM +0100, Daniel Borkmann wrote:
> >>The question is what is less risky in terms of uabi. To reject such
> >>filters with such K shift vals upfront in verifier, or to just allow
> >>[0, reg_size - 1] values and handle outliers silently. I think both
> >>might be possible, the latter just needs to be clearly specified in
> >>the documentation somewhere. If we go for the latter, then probably
> >>just rewriting that K value as masked one might seem better. Broken
> >>programs might then still be loadable (and still be broken) ... afaik
> >>in case of register (case of shifts with X) with large shift vals
> >>ARM64 is doing 'modulo reg_size' implicitly.
> >
> >The case of what happens with such shifts with X is also already
> >architecture-specific, even when using the interpreters.  For example, the
> >following program returns 1 on ARM64 but 0 on ARM.
> >
> >	BPF_STMT(BPF_LD | BPF_IMM, 1),
> >	BPF_STMT(BPF_LDX | BPF_IMM, 32),
> >	BPF_STMT(BPF_ALU | BPF_LSH | BPF_X, 0),
> >	BPF_STMT(BPF_RET | BPF_A, 0)
> >
> >To start rejecting large K shifts in the verifier because they are
> >architecture-specific while continuing to allow the equally
> >architecture-specific large X shifts (because we can't verify them
> >statically) would be rather inconsistent.
> 
> Hmm, yeah, agree with you that this would be inconsistent. Last time we
> actually had this topic [1], I believe the consensus was that such BPF
> programs are to be considered "undefined".
> 
> In that case, I think just masking the K value silently into its allowed
> range for classic and eBPF might be better. It would eventually not be
> uniform with regards to shifts where X is involved, but if we consider
> it "undefined" behaviour, such uniformity is probably not needed.

I don't think masking the K makes it any more 'consistent'.
As was shown arm and arm64 cpus are inconsistent between themselves
and it's not a job of sw to cover up such things.
imo rejecting broken programs at the load time for both cBPF and eBPF
is better for users, program authors and compiler writers in
the first place. If llvm (due to a bug) generates a shift with constant>32,
I'd prefer verifier to tell me about it instead of silently doing the mask
which likely will be much harder to discover and program going to stay
broken anyway.

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Will Deacon]

On Tue, Jan 05, 2016 at 06:39:03PM +0100, Rabin Vincent wrote:
> Attempting to generate UBFM/SBFM instructions with shifts that can't be
> encoded in the immediate fields of the opcodes leads to a trigger of a
> BUG() in the instruction generation code.  As the ARMv8 ARM says: "The
> shift amounts must be in the range 0 to one less than the register width
> of the instruction, inclusive."  Make the JIT reject unencodable shifts
> instead of crashing.

I moaned about those BUG_ONs when they were introduced:

  https://lkml.org/lkml/2014/7/17/438

The response then was that the verifier would catch these issues so
there was nothing to worry about. Has something changed so that is no
longer the case? Do we need to consider a different way of rejecting
invalid instructions at the encoding stage rather than bringing down the
kernel?

Will

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Alexei Starovoitov]

On Fri, Jan 08, 2016 at 03:44:23PM +0000, Will Deacon wrote:
> On Tue, Jan 05, 2016 at 06:39:03PM +0100, Rabin Vincent wrote:
> > Attempting to generate UBFM/SBFM instructions with shifts that can't be
> > encoded in the immediate fields of the opcodes leads to a trigger of a
> > BUG() in the instruction generation code.  As the ARMv8 ARM says: "The
> > shift amounts must be in the range 0 to one less than the register width
> > of the instruction, inclusive."  Make the JIT reject unencodable shifts
> > instead of crashing.
> 
> I moaned about those BUG_ONs when they were introduced:
> 
>   https://lkml.org/lkml/2014/7/17/438
> 
> The response then was that the verifier would catch these issues so
> there was nothing to worry about. Has something changed so that is no
> longer the case? Do we need to consider a different way of rejecting
> invalid instructions at the encoding stage rather than bringing down the
> kernel?

that discussion lead to replacement of all BUG_ONs in
arch/arm64/net/bpf_jit_comp.c with pr_err_once(), but looks like
arch/arm64/kernel/insn.c wasn't addressed.
The amount of BUG_ONs there is indeed overkill regardless of what
verifier and other JITs do. btw, x64 JIT doesn't have runtime BUG_ONs.

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Will Deacon]

On Fri, Jan 08, 2016 at 11:09:44AM -0800, Alexei Starovoitov wrote:
> On Fri, Jan 08, 2016 at 03:44:23PM +0000, Will Deacon wrote:
> > On Tue, Jan 05, 2016 at 06:39:03PM +0100, Rabin Vincent wrote:
> > > Attempting to generate UBFM/SBFM instructions with shifts that can't be
> > > encoded in the immediate fields of the opcodes leads to a trigger of a
> > > BUG() in the instruction generation code.  As the ARMv8 ARM says: "The
> > > shift amounts must be in the range 0 to one less than the register width
> > > of the instruction, inclusive."  Make the JIT reject unencodable shifts
> > > instead of crashing.
> > 
> > I moaned about those BUG_ONs when they were introduced:
> > 
> >   https://lkml.org/lkml/2014/7/17/438
> > 
> > The response then was that the verifier would catch these issues so
> > there was nothing to worry about. Has something changed so that is no
> > longer the case? Do we need to consider a different way of rejecting
> > invalid instructions at the encoding stage rather than bringing down the
> > kernel?
> 
> that discussion lead to replacement of all BUG_ONs in
> arch/arm64/net/bpf_jit_comp.c with pr_err_once(), but looks like
> arch/arm64/kernel/insn.c wasn't addressed.
> The amount of BUG_ONs there is indeed overkill regardless of what
> verifier and other JITs do. btw, x64 JIT doesn't have runtime BUG_ONs.

Maybe, but insn.c is also used by the alternatives patching code, so we
really need a way to communicate failure back to the BPF JIT when passed
an invalid instruction description.

Will

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Alexei Starovoitov]

On Tue, Jan 12, 2016 at 05:17:10PM +0000, Will Deacon wrote:
> On Fri, Jan 08, 2016 at 11:09:44AM -0800, Alexei Starovoitov wrote:
> > On Fri, Jan 08, 2016 at 03:44:23PM +0000, Will Deacon wrote:
> > > On Tue, Jan 05, 2016 at 06:39:03PM +0100, Rabin Vincent wrote:
> > > > Attempting to generate UBFM/SBFM instructions with shifts that can't be
> > > > encoded in the immediate fields of the opcodes leads to a trigger of a
> > > > BUG() in the instruction generation code.  As the ARMv8 ARM says: "The
> > > > shift amounts must be in the range 0 to one less than the register width
> > > > of the instruction, inclusive."  Make the JIT reject unencodable shifts
> > > > instead of crashing.
> > > 
> > > I moaned about those BUG_ONs when they were introduced:
> > > 
> > >   https://lkml.org/lkml/2014/7/17/438
> > > 
> > > The response then was that the verifier would catch these issues so
> > > there was nothing to worry about. Has something changed so that is no
> > > longer the case? Do we need to consider a different way of rejecting
> > > invalid instructions at the encoding stage rather than bringing down the
> > > kernel?
> > 
> > that discussion lead to replacement of all BUG_ONs in
> > arch/arm64/net/bpf_jit_comp.c with pr_err_once(), but looks like
> > arch/arm64/kernel/insn.c wasn't addressed.
> > The amount of BUG_ONs there is indeed overkill regardless of what
> > verifier and other JITs do. btw, x64 JIT doesn't have runtime BUG_ONs.
> 
> Maybe, but insn.c is also used by the alternatives patching code, so we
> really need a way to communicate failure back to the BPF JIT when passed
> an invalid instruction description.

agree. I think there are several options to achieve that after
all BUG_ONs are removed:
- change interface for all insn generating macros to check for
  AARCH64_BREAK_FAULT opcode as error.
  That will require all of emit*() functions in bpf_jit_comp.c to
  be changed to accept/return error.
  Overall that looks like massive change.
- ignore AARCH64_BREAK_FAULT during emit and add another pass after
  all code is generated. If such insn is found in a jited code,
  discard the jit.
  I think that's better option.

Zi, any comments?
 

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Z Lim]

On Tue, Jan 12, 2016 at 11:23 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Tue, Jan 12, 2016 at 05:17:10PM +0000, Will Deacon wrote:
>> On Fri, Jan 08, 2016 at 11:09:44AM -0800, Alexei Starovoitov wrote:
>> > On Fri, Jan 08, 2016 at 03:44:23PM +0000, Will Deacon wrote:
>> > > On Tue, Jan 05, 2016 at 06:39:03PM +0100, Rabin Vincent wrote:
>> > > > Attempting to generate UBFM/SBFM instructions with shifts that can't be
>> > > > encoded in the immediate fields of the opcodes leads to a trigger of a
>> > > > BUG() in the instruction generation code.  As the ARMv8 ARM says: "The
>> > > > shift amounts must be in the range 0 to one less than the register width
>> > > > of the instruction, inclusive."  Make the JIT reject unencodable shifts
>> > > > instead of crashing.
>> > >
>> > > I moaned about those BUG_ONs when they were introduced:
>> > >
>> > >   https://lkml.org/lkml/2014/7/17/438
>> > >
>> > > The response then was that the verifier would catch these issues so
>> > > there was nothing to worry about. Has something changed so that is no
>> > > longer the case? Do we need to consider a different way of rejecting
>> > > invalid instructions at the encoding stage rather than bringing down the
>> > > kernel?
>> >
>> > that discussion lead to replacement of all BUG_ONs in
>> > arch/arm64/net/bpf_jit_comp.c with pr_err_once(), but looks like
>> > arch/arm64/kernel/insn.c wasn't addressed.
>> > The amount of BUG_ONs there is indeed overkill regardless of what
>> > verifier and other JITs do. btw, x64 JIT doesn't have runtime BUG_ONs.
>>
>> Maybe, but insn.c is also used by the alternatives patching code, so we
>> really need a way to communicate failure back to the BPF JIT when passed
>> an invalid instruction description.
>
> agree. I think there are several options to achieve that after
> all BUG_ONs are removed:
> - change interface for all insn generating macros to check for
>   AARCH64_BREAK_FAULT opcode as error.
>   That will require all of emit*() functions in bpf_jit_comp.c to
>   be changed to accept/return error.
>   Overall that looks like massive change.
> - ignore AARCH64_BREAK_FAULT during emit and add another pass after
>   all code is generated. If such insn is found in a jited code,
>   discard the jit.
>   I think that's better option.
>
> Zi, any comments?
>

Alexei, agreed. Second approach is cleaner. Full disclosure: I did not
look at other callers beyond JIT.

Separately, sounds like there's now preference and consensus to
removing all BUGs and BUG_ONs in insn.c. Did a quick grep of insn.c
and noticed a legacy instance, followed by many introduced around the
same time as JIT, and new additions since.

Will, any thoughts on the following replacement scheme?

BUG_ON() for codegen ==> pr_err(); return AARCH64_BREAK_FAULT;
BUG() for decoding ==> leave as is.
remaining BUG_ON() ==> leave as is.

Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Will Deacon]

On Tue, Jan 12, 2016 at 08:45:43PM -0800, Z Lim wrote:
> On Tue, Jan 12, 2016 at 11:23 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Tue, Jan 12, 2016 at 05:17:10PM +0000, Will Deacon wrote:
> >> On Fri, Jan 08, 2016 at 11:09:44AM -0800, Alexei Starovoitov wrote:
> >> > On Fri, Jan 08, 2016 at 03:44:23PM +0000, Will Deacon wrote:
> >> > > On Tue, Jan 05, 2016 at 06:39:03PM +0100, Rabin Vincent wrote:
> >> > > > Attempting to generate UBFM/SBFM instructions with shifts that can't be
> >> > > > encoded in the immediate fields of the opcodes leads to a trigger of a
> >> > > > BUG() in the instruction generation code.  As the ARMv8 ARM says: "The
> >> > > > shift amounts must be in the range 0 to one less than the register width
> >> > > > of the instruction, inclusive."  Make the JIT reject unencodable shifts
> >> > > > instead of crashing.
> >> > >
> >> > > I moaned about those BUG_ONs when they were introduced:
> >> > >
> >> > >   https://lkml.org/lkml/2014/7/17/438
> >> > >
> >> > > The response then was that the verifier would catch these issues so
> >> > > there was nothing to worry about. Has something changed so that is no
> >> > > longer the case? Do we need to consider a different way of rejecting
> >> > > invalid instructions at the encoding stage rather than bringing down the
> >> > > kernel?
> >> >
> >> > that discussion lead to replacement of all BUG_ONs in
> >> > arch/arm64/net/bpf_jit_comp.c with pr_err_once(), but looks like
> >> > arch/arm64/kernel/insn.c wasn't addressed.
> >> > The amount of BUG_ONs there is indeed overkill regardless of what
> >> > verifier and other JITs do. btw, x64 JIT doesn't have runtime BUG_ONs.
> >>
> >> Maybe, but insn.c is also used by the alternatives patching code, so we
> >> really need a way to communicate failure back to the BPF JIT when passed
> >> an invalid instruction description.
> >
> > agree. I think there are several options to achieve that after
> > all BUG_ONs are removed:
> > - change interface for all insn generating macros to check for
> >   AARCH64_BREAK_FAULT opcode as error.
> >   That will require all of emit*() functions in bpf_jit_comp.c to
> >   be changed to accept/return error.
> >   Overall that looks like massive change.
> > - ignore AARCH64_BREAK_FAULT during emit and add another pass after
> >   all code is generated. If such insn is found in a jited code,
> >   discard the jit.
> >   I think that's better option.
> >
> > Zi, any comments?
> >
> 
> Alexei, agreed. Second approach is cleaner. Full disclosure: I did not
> look at other callers beyond JIT.
> 
> Separately, sounds like there's now preference and consensus to
> removing all BUGs and BUG_ONs in insn.c. Did a quick grep of insn.c
> and noticed a legacy instance, followed by many introduced around the
> same time as JIT, and new additions since.
> 
> Will, any thoughts on the following replacement scheme?
> 
> BUG_ON() for codegen ==> pr_err(); return AARCH64_BREAK_FAULT;
> BUG() for decoding ==> leave as is.
> remaining BUG_ON() ==> leave as is.

That sounds good to me, thanks.

Will