On Thu, Jul 4, 2024 at 8:17 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On 04/07/2024 01:15, Andrii Nakryiko wrote:
> > BPF skeleton was designed from day one to be extensible. Generated BPF
> > skeleton code specifies actual sizes of map/prog/variable skeletons for
> > that reason and libbpf is supposed to work with newer/older versions
> > correctly.
> >
> > Unfortunately, it was missed that we implicitly embed hard-coded most
> > up-to-date (according to libbpf's version of libbpf.h header used to
> > compile BPF skeleton header) sizes of those strucs, which can differ
> > from the actual sizes at runtime when libbpf is used as a shared
> > library.
> >
> > We have a few places were we just index array of maps/progs/vars, which
> > implicitly uses these potentially invalid sizes of structs.
> >
> > This patch aims to fix this problem going forward. Once this lands,
> > we'll backport these changes in Github repo to create patched releases
> > for older libbpfs.
> >
> > Fixes: d66562fba1ce ("libbpf: Add BPF object skeleton support")
> > Fixes: 430025e5dca5 ("libbpf: Add subskeleton scaffolding")
>
> Great catch! I suppose it also sort of
>
> Fixes: 08ac454e258 ("libbpf: Auto-attach struct_ops BPF maps in BPF
> skeleton")
>
> ...since that introduced the new bpf_map_skeleton field. Not a big deal
> since it's new and not a stable backport candidate.
>
yeah, I put the original changes that introduced this
bug/inflexibility in the first place. Either way Github releases and
backporting will be done separate from the kernel repo.
> > Co-developed-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> I'm guessing that you found this when auto-attach silently didn't happen?
nope, we actually got crashes due to memory corruption in our internal
production testing
>
> Nit: would it be worth dropping a debug logging message here
>
>
> /* Skeleton is created with earlier version of bpftool
> * which does not support auto-attachment
> */
> - if (s->map_skel_sz < sizeof(struct bpf_map_skeleton))
> + if (s->map_skel_sz < sizeof(struct bpf_map_skeleton)) {
> + pr_debug("libbpf version mismatch, cannot auto-attach\n");
ok, sure. But as Eduard pointed out, it's not really a bug or anything
like that, it's an expected backwards compat mechanism. I can add
debug-level (or probably info-level?) message, though.
> return 0;
> + }
>
> ...as it's a hard issue to spot?
>
> For the series:
>
> Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
