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.
>
> >
> > > - } 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.
> Thanks,
> David
