Message-ID: <87h89kkjnk.fsf@netronome.com>
Date:   Thu, 23 May 2019 21:20:15 +0100
From:   Jiong Wang <jiong.wang@...ronome.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Jiong Wang <jiong.wang@...ronome.com>,
        Daniel Borkmann <daniel@...earbox.net>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, oss-drivers@...ronome.com,
        davem@...emloft.net, paul.burton@...s.com, udknight@...il.com,
        zlim.lnx@...il.com, illusionist.neo@...il.com,
        naveen.n.rao@...ux.ibm.com, sandipan@...ux.ibm.com,
        schwidefsky@...ibm.com, heiko.carstens@...ibm.com,
        jakub.kicinski@...ronome.com
Subject: Re: [PATCH v7 bpf-next 01/16] bpf: verifier: mark verified-insn with sub-register zext flag


Alexei Starovoitov writes:

<snip>

> well, it made me realize that we're probably doing it wrong,
> since after calling check_reg_arg() we need to re-parse insn encoding.
> How about we change check_reg_arg()'s enum reg_arg_type instead?

This is exactly what I had implemented in my initial internal version.

> The caller has much more context and no need to parse insn opcode again.

And I had exactly the same thoughts, is_reg64 is duplicating what has been
done.

The code evolved into the current shape mostly because I agreed if we
re-centre all checks inside check_reg_arg, then we don't need to touch
quite a few call sites of check_reg_arg, the change/patch looks simpler,
and I was thinking is_reg64 is a quick check, so the overhead is not big.

> Something like:
> enum reg_arg_type {
>         SRC_OP64,        
>         DST_OP64,       
>         DST_OP_NO_MARK, // probably no need to split this one ?
>         SRC_OP32,      
>         DST_OP32,      
> };
>

Yeah, no need to split DST_OP_NO_MARK, my split was

enum reg_arg_type {
   SRC_OP,
+  SRC32_OP,
   DST_OP,
+  DST32_OP,
   DST_OP_NO_MARK 
}

No renaming on existing SRC/DST_OP, they mean 64-bit, the changes are
smaller, looks better?

But, we also need to know whether one patch-insn define sub-register, if it
is, we then conservatively mark it as needing zero extension. patch-insn
doesn't go through check_reg_arg analysis, so still need separate insn
parsing.

And because we also introduced hi32 rand which should only happen if
insn_aux.zext_dst is false. But when zext_dst is false, there are two
situations, one is this insn define a sub-register whose hi32 is not used
later, the other is this insn define a full 64-bit register. hi32 rand
should only happen on the prior situation. So is_reg64 also called to rule
out the latter. The use of is_64 could be removed by changing aux.zext_dst
from bool to enum, so we rename it to aux.reg_def, its value could be:

  REG_DEF_NONE (some insn doesn't define value, this is default)
  REG_DEF64
  REG_DEF32
  REG_DEF32_ZEXT

When calling check_reg_arg, and DST/DST_32 will initialize aux.reg_def into
REG_DEF64 and REG_DEF32 accordingly. Then, a later 64-bit use on sub-register
could promote REG_DEF32 into REG_DEF32_ZEXT.

In all, my propose changes are:
  1. split enum reg_arg_type, adding new SRC32_OP and DST32_OP, during insn
     walking, let call sites of check_reg_arg passing correct type
     directly, remove insn re-parsing inside check_reg_arg.
  2. keep "is_reg64", it will be used by parsing patched-insn.
  3. change aux.zext_dst to aux.reg_def, and change the type from bool to
     the enum listed above. When promoting one reg to REG_DEF32_ZEXT, also
     do sanity check, the promotion should only happen on REG_DEF32.

Does this looks good?

Regards,
Jiong

Powered by blists - more mailing lists