On Tue, Apr 19, 2022 at 1:18 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Wed, Apr 20, 2022 at 12:53:55AM IST, Joanne Koong wrote:
> > > [...]
> > > There is another issue I noticed while basing other work on this. You have
> > > declared bpf_dynptr in UAPI header as:
> > >
> > > struct bpf_dynptr {
> > > __u64 :64;
> > > __u64 :64;
> > > } __attribute__((aligned(8)));
> > >
> > > Sadly, in C standard, the compiler is under no obligation to initialize padding
> > > bits when the object is zero initialized (using = {}). It is worse, when
> > > unrelated struct fields are assigned the padding bits are assumed to attain
> > > unspecified values, but compilers are usually conservative in that case (C11
> > > 6.2.6.1 p6).
> > Thanks for noting this. By "padding bits", you are referring to the
> > unnamed fields, correct?
> >
> > From the commit message in 5eaed6eedbe9, I see:
> >
> > INTERNATIONAL STANDARD ©ISO/IEC ISO/IEC 9899:201x
> > Programming languages — C
> > http://www.open-std.org/Jtc1/sc22/wg14/www/docs/n1547.pdf
> > page 157:
> > Except where explicitly stated otherwise, for the purposes of
> > this subclause unnamed members of objects of structure and union
> > type do not participate in initialization. Unnamed members of
> > structure objects have indeterminate value even after initialization.
> >
> > so it seems like the best way to address that here is to just have the
> > fields be explicitly named, like something like
> >
> > struct bpf_dynptr {
> > __u64 anon1;
> > __u64 anon2;
> > } __attribute__((aligned(8)))
> >
> > Do you agree with this assessment?
> >
>
> Yes, this should work. Also, maybe 'variable not initialized error' shouldn't be
> 'verifier internal error', since it would quite common for user to hit it.
>
I looked into this some more and I don't think it's an issue that the
compiler doesn't initialize anonymous fields and/or initializes it
with indeterminate values. We set up the dynptr in
bpf_dynptr_from_mem() and bpf_dynptr_alloc() where we initialize its
contents with real values. It doesn't matter if prior to
bpf_dynptr_from_mem()/bpf_dynptr_alloc() it's filled with garbage
values because they'll be overridden.
The "verifier internal error: variable not initialized on stack in
mark_as_dynptr_data" error you were seeing is unrelated to this. It's
because of a mistake in mark_as_dynptr_data() where when we check that
the memory size of the data should be within the spi bounds, the 3rd
argument we pass to is_spi_bounds_valid() should be the number of
slots, not the memory size (the value should be mem_size /
BPF_REG_SIZE, not mem_size). Changing this fixes the error.
> --
> Kartikeya
