On Sun, Sep 29, 2024 at 11:30 PM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> wrote:
>
> Hi Andrii and Eduard,
>
> On Sun, Sep 22, 2024 at 02:41:08AM GMT, Shung-Hsi Yu wrote:
> > A topic that came up several times off-list at LPC was how to start
> > contributing to the BPF subsystem. One of the thing that would probably help
> > is to have a list of todos that are nice to have and can be implemented in a
> > relatively self-contained set of patches. Here's things that I've gathered.
> >
> > On the more concrete task sides (easy to hard):
> >
> > - Check return value of btf__align_of() in btf_dump_emit_struct_def()
>
> The above task is currently being worked on. But I though its better to
> ask you both for opinion before I lead anyone astray.
>
> My understanding is that btf__align_of() could return an error, thus all
> caller should check for its return value, for example:
>
> static void btf_dump_emit_struct_def(...)
> {
> ...
> align = btf__align_of(d->btf, id);
>
> for (i = 0; i < vlen; i++, m++) {
> const char *fname;
> int m_off, m_sz, m_align;
> ...
> m_align = packed ? 1 : btf__align_of(d->btf, m->type);
>
> in_bitfield = prev_bitfield && m_sz != 0;
>
> btf_dump_emit_bit_padding(d, off, m_off, m_align, in_bitfield, lvl + 1);
> btf_dump_printf(d, "\n%s", pfx(lvl + 1));
> ...
> }
>
> /* pad at the end, if necessary */
> if (is_struct)
> btf_dump_emit_bit_padding(d, off, t->size * 8, align, false, lvl + 1);
> ...
> }
>
> Should check whether align or m_align is 0 before moving forward.
>
> The reason I looked into this was because I ran into floating point
> exception a while back when trying to dump C-style header file out of a
> kernel module while mistakenly using the wrong base BTF, which crashed
> inside btf_dump_emit_bit_padding() at
>
> roundup(cur_off, next_align * 8)
>
> Because roundup() requires the second argument to be positive, yet
> next_align that came from a btf__align_of() call was 0. I believe this
> still may happen with basic BTF validation[1] added.
TBH, I'd rather improve sanity checking to prevent invalid INT/FLOAT
definitions that don't have size.
>
> So, questions:
> - Is checking return value of btf__align_of() in
> btf_dump_emit_struct_def() wanted? And if so:
The assumption is that you are dumping a well-formed BTF, so, barring
bugs, align_of() should always be valid and well-defined. So I'd leave
it as is, but, improve sanity checking, if that's the concern.
> - What's the preferable action to take when it returns an error?
> According to the comment for btf_dump_emit_type() it seems like the
> best thing to do is simply return in btf_dump_emit_struct_def()?
> - Should we add a pr_* to report such error?
> - Any place that skipping the return value check of btf__align_of() is
> fine?
All those btf_dump_emit_*() functions are void-returning, and so they
basically expect valid BTF. In the case of invalid BTF, that's a)
user's problem ;) but also b) those functions generally will just
either produce a bit of garbage output or exit early, resulting in
incomplete/clobbered vmlinux.h output, but other than that should be
fine.
If you provide invalid base BTF, way more than btf__align_of() would
cause problems.
>
> 1: https://lore.kernel.org/bpf/20230825202152.1813394-1-andrii@xxxxxxxxxx/
>
> (One more question below)
>
> ...
> > - Replace open-coded & PTR_MAYBE_NULL checks with type_may_be_null()
> > - Implement tnum_scast(), and use that to simply var_off induction in
> > coerce_reg_to_size_sx()
> > - Better error message when BTF generation failed, or at least fail earlier
> > - Refactor to use list_head to create a linked-list of bpf_verifier_state
> > instead of using bpf_verifier_state_list
> >
> > On the more general side of things:
> >
> > - Improve the documentation
> > - add the missing pieces (e.g. document all BPF_PROG_TYPE_*)
> > - update the out-date part (admittedly quite hard)
> > - Improve the BPF selftests coverage
> > - add test for fixes that have been merged but does not come with a
> > corresponding test case to prevent regression
> >
> > I want to keep the list from being too verbose, so I won't go into too
> > much detail in this email. But feel free to reply to this thread and
> > ask. You might want to use https://github.com/sjp38/hackermail to reply
> > if you're not familiar with mailing lists.
> > (I know mailing list don't have the best UX, is a scary place, and also
> > not the best issue tracker, we'll see how this works out and change if
> > needed)
> >
> > Also If anyone has other things they want to add to the list that will
> > be great.
>
> Is there any libbpf task(s) that you think that might be good for
> first-time contributors? I see there are issues tracker for the
> libbpf/libbpf project on GitHub[2], but wonder if there's specific ones
> that are suggested for first time contributor to tackle.
>
> 2: https://github.com/libbpf/libbpf/issues
I'd appreciate any help with handling these issues and helping users
with their problems and/or fixing OSS Fuzz issues (or at least
triaging them).
That would be a good start.
>
> ...
> > Resources
> >
> > - Introduction to BPF selftests
> > https://lore.kernel.org/bpf/62b54401510477eebdb6e1272ba4308ee121c215.camel@xxxxxxxxx/
