On Tue, Nov 9, 2021 at 8:41 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, Nov 9, 2021 at 8:36 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Tue, Nov 9, 2021 at 8:34 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov
> > > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote:
> > > > > > This is a pure cleanup patchset that tries to use flag to mark whether
> > > > > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing
> > > > > > so allows us to embed the properties of arguments in the struct, which
> > > > > > is a more scalable solution than introducing a new enum. This patchset
> > > > > > performs this transformation only on arg_type. If it looks good,
> > > > > > follow-up patches will do the same on reg_type and ret_type.
> > > > > >
> > > > > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type'
> > > > > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs.
> > > > >
> > > > > Nice. Thank you for working on it!
> > > >
> > > > No problem. :)
> > > >
> > > > >
> > > > > The enum->struct conversion works for bpf_arg_type, but applying
> > > > > the same technique to bpf_reg_type could be problematic.
> > > > > Since it's part of bpf_reg_state which in turn is multiplied by a large factor.
> > > > > Growing enum from 4 bytes to 8 byte struct will consume quite
> > > > > a lot of extra memory.
> > > > >
> > > > > > 19 files changed, 932 insertions(+), 780 deletions(-)
> > > > >
> > > > > Just bpf_arg_type refactoring adds a lot of churn which could make
> > > > > backports of future fixes not automatic anymore.
> > > > > Similar converstion for bpf_reg_type and bpf_return_type will
> > > > > be even more churn.
> > > >
> > > > Acknowledged.
> > > >
> > > > > Have you considered using upper bits to represent flags?
> > > >
> > > > Yes, I thought about that. Some of my thoughts are:
> > > >
> > > > - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough?
> > > > - What if we run out of flag bits in future?
> > > > - We could fold btf_id in the structure in this patchset. And new
> > > > fields could be easily added if needed.
> > > >
> > > > So with these questions, I didn't pursue that approach in the first
> > > > place. But I admit that it does look better by writing
> > > >
> > > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL,
> > > >
> > > > Instead of
> > > >
> > > > + .arg3 = {
> > > > + .type = ARG_PTR_TO_MAP_VALUE,
> > > > + .flag = ARG_FLAG_MAYBE_NULL,
> > > > + },
> > > >
> > > > Let's see if there is any further comment. I can go take a look and
> > > > prepare for that approach in the next revision.
> > > >
> > >
> > > +1 on staying within a single enum and using upper bits
> > >
> > > >
> > > >
> > > > >
> > > > > Instead of diff:
> > > > > - .arg1_type = ARG_CONST_MAP_PTR,
> > > > > - .arg2_type = ARG_PTR_TO_FUNC,
> > > > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL,
> > > > > - .arg4_type = ARG_ANYTHING,
> > > > > + .arg1 = { .type = ARG_CONST_MAP_PTR },
> > > > > + .arg2 = { .type = ARG_PTR_TO_FUNC },
> > > > > + .arg3 = { .type = ARG_PTR_TO_STACK_OR_NULL },
> > > > > + .arg4 = { .type = ARG_ANYTHING },
> > > > >
> > > > > can we make it look like:
> > > > > .arg1_type = ARG_CONST_MAP_PTR,
> > > > > .arg2_type = ARG_PTR_TO_FUNC,
> > > > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL,
> > > > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL,
> > > > > .arg4_type = ARG_ANYTHING,
> > > > >
> > > > > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type)
> > > > > would share the same flag bit: MAYBE_NULL.
> > >
> > > I support using the same bit value, but should we use the exact same
> > > enum name for three different enums? Like MAYBE_NULL, which enum is it
> > > defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to
> > > REG_MAYBE_NULL be more explicit about what they apply to?
> >
> > argh, I meant to write "RET_MAYBE_NULL and ARG_MAYBE_NULL, in addition
> > to REG_MAYBE_NULL".
>
> Why differentiate? What difference do you see?
> The flag looks the same to me in return, reg and arg.
We have three different enums which will be combined with some
constants defined outside of some of those enums. Just have a gut
feeling that this will bite us at some point. Nothing more.
>
> > >
> > > BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not
> > > the same thing, are they? Is the plan to use two different bits for
> > > them or pretend that CONST_OR_ZERO "may be null"?
>
> What's the difference ?
> I think single MAYBE_NULL (or call it MAYBE_ZERO) can apply correctly
> to both ARG_CONST_SIZE, ARG_CONST_ALLOC_SIZE, and [ARG_]PTR_TO_MEM.
Again, just gut feeling that this will go wrong.
But also one specific example from kernel/bpf/verifier.c:
if (register_is_null(reg) && arg_type_may_be_null(arg_type))
goto skip_type_check;
Currently arg_type_may_be_null(arg_type) returns false for
ARG_CONST_SIZE_OR_ZERO. If we are not careful and blindly check the
MAYBE_NULL flag (which the current patch set seems to be doing), we'll
start returning true for it and some other _OR_ZERO arg types. It
might be benign in this particular case, I haven't traced if
ARG_CONST_SIZE_OR_ZERO can be passed in that particular code path, but
it was hardly intended this way, no?
