On Fri, May 6, 2022 at 3:46 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, May 6, 2022 at 1:32 PM David Vernet <void@xxxxxxxxxxxxx> wrote:
> >
> > On Fri, May 06, 2022 at 12:09:37PM -0700, Joanne Koong wrote:
> > > I think the bpf philosophy leans more towards conflating related-ish
> > > patches into the same patchset. I think this patch could be its own
> > > stand-alone patchset, but it's also related to the dynptr patchset in that
> > > dynptrs need it to properly describe its initialization helper functions.
> > > I'm happy to submit this as its own patchset though if that is preferred :)
> >
> > Totally up to you, if that's the BPF convention then that's fine with me.
>
> You meant
>
> - [ARG_PTR_TO_UNINIT_MEM] = &mem_types,
>
> parts as stand-alone patch? That would be invalid on its own without
> adding MEM_UNINT, so would potentially break bisection. So no, it
> shouldn't be a stand-alone patch. Each patch has to be logically
> separate but not causing any regressions in behavior, compilation,
> selftest, etc. So, for example, while we normally put selftests into
> separate tests, if kernel change breaks selftests, selftests have to
> be fixed in the same patch to avoid having any point where bisection
> can detect the breakage.
>
Ah okay, I thought we were talking about having all of the first patch
be its standalone patch. sorry for the confusion.
>
> >
> > >
> > > > - } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE ||
> > > > > - base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) {
> > > > > + } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) {
> > > > > if (type_may_be_null(arg_type) && register_is_null(reg))
> > > > > return 0;
> > > > >
> > > > > @@ -5811,7 +5801,7 @@ static int check_func_arg(struct bpf_verifier_env
> > > > *env, u32 arg,
> > > > > verbose(env, "invalid map_ptr to access
> > > > map->value\n");
> > > > > return -EACCES;
> > > > > }
> > > > > - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE);
> > > > > + meta->raw_mode = arg_type & MEM_UNINIT;
> > > >
> > > > Given that we're stashing in a bool here, should this be:
> > > >
> > > > meta->raw_mode = (arg_type & MEM_UNINIT) != 0;
> > > >
> > > I think just arg_type & MEM_UNINIT is okay because it implicitly converts
> > > from 1 -> true, 0 -> false. This is the convention that's used elsewhere in
> > > the linux codebase as well
> >
> > Yeah I think functionally it will work just fine as is. I saw that a few
> > other places in verifier.c use operators that explicitly make the result 0
> > or 1, e.g.:
> >
> > 14699
> > 14700 env->strict_alignment = !!(attr->prog_flags & BPF_F_STRICT_ALIGNMENT);
> >
> > But the compiler will indeed implicitly convert any nonzero value to 1 if
> > it's stored in a bool, so it's not necessary for correctness. It looks like
> > the kernel style guide also implies that using the extra operators isn't
> > necessary, so I think we can leave it as you have it now:
> > https://www.kernel.org/doc/html/latest/process/coding-style.html#using-bool
>
> Yeah, the above example is rather unusual, I'd say. We do
> !!(bool_expr) only when we want to assign that to integer (not bool)
> variable/field as 0 or 1. Otherwise it's a well-defined compiler
> conversion rule for any non-zero value to be true during bool
> conversion.
>
> >
> > > > What do you think about this as a possibly more concise way to express that
> > > > the curr and next args differ?
> > > >
> > > > return (base_type(arg_curr) == ARG_PTR_TO_MEM) !=
> > > > arg_type_is_mem_size(arg_next);
> > > >
> > > I was trying to decide between this and the more verbose expression above
> > > and ultimately went with the more verbose expression because it seemed more
> > > readable to me. But I don't feel strongly :) I'm cool with either one
> >
> > I don't feel strongly either, if you think your way is more readable then
> > don't feel obligated to change it.
> >
>
> Heh, this also caught my eye. It's subjective, but inequality is
> shorter and more readable (even in terms of the logic it expresses).
> But it's fine either way with me.
Since both of you think the inequality way is more readable, I will
change it to inequality for v4 then :)
>
> > Thanks,
> > David
