Message-ID: <87ef5795b5.fsf@netronome.com>
Date:   Thu, 09 May 2019 13:32:30 +0100
From:   Jiong Wang <jiong.wang@...ronome.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Jiong Wang <jiong.wang@...ronome.com>, daniel@...earbox.net,
        bpf@...r.kernel.org, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH v6 bpf-next 01/17] bpf: verifier: offer more accurate helper function arg and return type


Alexei Starovoitov writes:

> On Wed, May 08, 2019 at 03:45:12PM +0100, Jiong Wang wrote:
>> 
>> I might be misunderstanding your points, please just shout if I am wrong.
>> 
>> Suppose the following BPF code:
>> 
>>   unsigned helper(unsigned long long, unsigned long long);
>>   unsigned long long test(unsigned *a, unsigned int c)
>>   {
>>     unsigned int b = *a;
>>     c += 10;
>>     return helper(b, c);
>>   }
>> 
>> We get the following instruction sequence by latest llvm
>> (-O2 -mattr=+alu32 -mcpu=v3)
>> 
>>   test:
>>     1: w1 = *(u32 *)(r1 + 0)
>>     2: w2 += 10
>>     3: call helper
>>     4: exit
>> 
>> Argument Types
>> ===
>> Now instruction 1 and 2 are sub-register defines, and instruction 3, the
>> call, use them implicitly.
>> 
>> Without the introduction of the new ARG_CONST_SIZE32 and
>> ARG_CONST_SIZE32_OR_ZERO, we don't know what should be done with w1 and
>> w2, zero-extend them should be fine for all cases, but could resulting in a
>> few unnecessary zero-extension inserted.
>
> I don't think we're on the same page.
> The argument type is _const_.
> In the example above they are not _const_.

Right, have read check_func_arg + check_helper_mem_access again.

Looks like ARG_CONST_SIZE* are designed for describing memory access size
for things like bounds checking. It must be a constant for stack access,
otherwise prog will be rejected, but it looks to me variables are allowed
for pkt/map access.

But pkt/map has extra range info. So, indeed, ARG_CONST_SIZE32* are
unnecessary, the width could be figured out through the range.

Will just drop this patch in next version.

And sorry for repeating it again, I am still concerned on the issue
described at https://www.spinics.net/lists/netdev/msg568678.html.

To be simple, zext insertion is based on eBPF ISA and assumes all
sub-register defines from alu32 or narrow loads need it if the underlying
hardware arches don't do it. However, some arches support hardware zext
partially. For example, PowerPC, SPARC etc are 64-bit arches, while they
don't do hardware zext on alu32, they do it for narrow loads. And RISCV is
even more special, some alu32 has hardware zext, some don't.

At the moment we have single backend hook "bpf_jit_hardware_zext", once a
backend enable it, verifier just insert zero extension for all identified
alu32 and narrow loads.

Given verifier analysis info is not pushed down to JIT back-ends, verifier
needs more back-end info pushed up from back-ends. Do you think make sense
to introduce another hook "bpf_jit_hardware_zext_narrow_load" to at least
prevent unnecessary zext inserted for narrowed loads for arches like
PowerPC, SPARC?

The hooks to control verifier zext insertion then becomes two:

  bpf_jit_hardware_zext_alu32
  bpf_jit_hardware_zext_narrow_load

>> And that why I introduce these new argument types, without them, there
>> could be more than 10% extra zext inserted on benchmarks like bpf_lxc.
>
> 10% extra ? so be it.
> We're talking past each other here.
> I agree with your optimization goal, but I think you're missing
> the safety concerns I'm trying to explain.
>> But for helper functions, they are done by native code which may not follow
>> this convention. For example, on arm32, calling helper functions are just
>> jump to and execute native code. And if the helper returns u32, it just set
>> r0, no clearing of r1 which is the high 32-bit in the register pair
>> modeling eBPF R0.
>
> it's arm32 bug then. All helpers _must_ return 64-bit back to bpf prog
> and _must_ accept 64-bit from bpf prog.

Powered by blists - more mailing lists