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.
So, questions:
- Is checking return value of btf__align_of() in
btf_dump_emit_struct_def() wanted? And if so:
- 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?
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
...
> Resources
>
> - Introduction to BPF selftests
> https://lore.kernel.org/bpf/62b54401510477eebdb6e1272ba4308ee121c215.camel@xxxxxxxxx/
