2021-03-18 06:39:12

by Jianlin Lv

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

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 <[email protected]>
---
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;
--
2.25.1

2021-03-18 15:59:35

by Daniel Borkmann

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

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 <[email protected]>
> ---
> 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

2021-03-19 02:06:52

by Jianlin Lv

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

On Thu, Mar 18, 2021 at 11:58 PM Daniel Borkmann <[email protected]> wrote:
>
> 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 <[email protected]>
> > ---
> > 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

I have some different opinions. I think the addition of the mask just helps
developers understand that the currently processed instruction covers all
possible values of size;

In addition, from my experience in reading the verfier.c file,
to fully understand this part of the code, it needs to understand the
meaning of each instruction.
It is really hard to say that this is an easy task, at least for me.
Haha ^_^

Regards,
Jianlin