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