Re: [PATCH bpf-next v2] bpf: Simplify expression for identify bpf mem type

[Daniel Borkmann]

On 3/18/21 7:36 AM, Jianlin Lv wrote:

Added BPF_LD_ST_SIZE_MASK macro as mask of size modifier that help to
reduce the evaluation of expressions in if statements,
and remove BPF_SIZE_MASK in netronome driver.

Signed-off-by: Jianlin Lv <jianlin...@arm.com>
---
v2: Move the bpf_LD_ST_SIZE_MASK macro definition to include/linux/bpf.h
---
  drivers/net/ethernet/netronome/nfp/bpf/main.h |  8 +++-----
  include/linux/bpf.h                           |  1 +
  kernel/bpf/verifier.c                         | 12 ++++--------
  3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h 
b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index d0e17eebddd9..e90981e69763 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -346,8 +346,6 @@ struct nfp_insn_meta {
        struct list_head l;
  };

-#define BPF_SIZE_MASK 0x18

-
  static inline u8 mbpf_class(const struct nfp_insn_meta *meta)
  {
        return BPF_CLASS(meta->insn.code);
@@ -375,7 +373,7 @@ static inline bool is_mbpf_alu(const struct nfp_insn_meta 
*meta)

static inline bool is_mbpf_load(const struct nfp_insn_meta *meta)

  {
-       return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_LDX | BPF_MEM);
+       return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_LDX | BPF_MEM);
  }

static inline bool is_mbpf_jmp32(const struct nfp_insn_meta *meta)

@@ -395,7 +393,7 @@ static inline bool is_mbpf_jmp(const struct nfp_insn_meta 
*meta)

static inline bool is_mbpf_store(const struct nfp_insn_meta *meta)

  {
-       return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_MEM);
+       return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_MEM);
  }

static inline bool is_mbpf_load_pkt(const struct nfp_insn_meta *meta)

@@ -430,7 +428,7 @@ static inline bool is_mbpf_classic_store_pkt(const struct 
nfp_insn_meta *meta)

static inline bool is_mbpf_atomic(const struct nfp_insn_meta *meta)

  {
-       return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_ATOMIC);
+       return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | 
BPF_ATOMIC);
  }

static inline bool is_mbpf_mul(const struct nfp_insn_meta *meta)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a25730eaa148..e85924719c65 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -995,6 +995,7 @@ struct bpf_array {
                                 BPF_F_RDONLY_PROG |    \
                                 BPF_F_WRONLY |         \
                                 BPF_F_WRONLY_PROG)
+#define BPF_LD_ST_SIZE_MASK    0x18    /* mask of size modifier */

#define BPF_MAP_CAN_READ BIT(0)

  #define BPF_MAP_CAN_WRITE     BIT(1)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f9096b049cd6..29fdfdb8abfa 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11384,15 +11384,11 @@ static int convert_ctx_accesses(struct 
bpf_verifier_env *env)
        for (i = 0; i < insn_cnt; i++, insn++) {
                bpf_convert_ctx_access_t convert_ctx_access;

- if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) ||

-                   insn->code == (BPF_LDX | BPF_MEM | BPF_H) ||
-                   insn->code == (BPF_LDX | BPF_MEM | BPF_W) ||
-                   insn->code == (BPF_LDX | BPF_MEM | BPF_DW))
+               /* opcode: BPF_MEM | <size> | BPF_LDX */
+               if ((insn->code & ~BPF_LD_ST_SIZE_MASK) == (BPF_LDX | BPF_MEM))
                        type = BPF_READ;
-               else if (insn->code == (BPF_STX | BPF_MEM | BPF_B) ||
-                        insn->code == (BPF_STX | BPF_MEM | BPF_H) ||
-                        insn->code == (BPF_STX | BPF_MEM | BPF_W) ||
-                        insn->code == (BPF_STX | BPF_MEM | BPF_DW))
+               /* opcode: BPF_MEM | <size> | BPF_STX */
+               else if ((insn->code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | 
BPF_MEM))
                        type = BPF_WRITE;
                else
                        continue;

To me this cleanup makes the code harder to read, in particular on verfier side,
I don't think it's worth it, especially given it's not in (highly) performance
critical code.

Thanks,
Daniel