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

[Jianlin Lv <iecedge@gmail.com>]

On Thu, Mar 18, 2021 at 11:58 PM Daniel Borkmann <daniel@iogearbox.net> 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 <Jianlin.Lv@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

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