Thread (3 messages) flat view 3 messages, 3 authors, 2021-03-19

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

From: Daniel Borkmann <daniel@iogearbox.net>
Date: 2021-03-18 15:58:49
Also in: bpf, linux-api, lkml

On 3/18/21 7:36 AM, Jianlin Lv wrote:
quoted hunk ↗ jump to hunk
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 <redacted>
---
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help