On Tue, Mar 01, 2022 at 07:04:40PM -0300, Rafael David Tinoco wrote:
>
> >> The bad-commit mentioned in "the Fixes tag":
> >> Fixes: a23740ec43ba ("bpf: Track contents of read-only maps as scalars")
> >> Which as you say, could well have been fixing another issue.
> >> In fact, yes it was:
> >> https://lore.kernel.org/stable/20210821203108.215937-2-rafaeldtinoco@xxxxxxxxx/
> >> Daniel, what do you suggest please?
> >
> > Hm, okay, so a23740ec43ba ("bpf: Track contents of read-only maps as scalars") was
> > backported to 5.4.144 given Rafael needed it to fix a failing regression test [0].
> >
> > Normally, I would have said that we should just revert a23740ec43ba given it was
> > not a 'fix' in the first place, but then we are getting into a situation where it
> > would break Rafael's now functioning test case again on 5.4.144+ released kernels.
> >
>
> IIRC, Without this patch, eBPF programs with extern variables, either from ksyms
> or kconfig relocations, done by libbpf, used as branch conditions, won't work in
> <= 5.4.144.
>
> Something like:
>
> extern u32 CONFIG_ARCH_HAS_SYSCALL_WRAPPER __kconfig;
> ...
> if (CONFIG_ARCH_HAS_SYSCALL_WRAPPER) {
> valid BTF type declared/used
> } else {
> <dead code>: invalid BTF type declared/used
> }
> ...
>
> The dead code is always evaluated and object load does not pass the verifier.
>
> The workaround to mitigate this is to always rely in type/field existence checks
> for the branch conditions, instead of relying in kconfig/ksyms relocations.
>
> We've been doing this to support same CO-RE BPF obj in kernels < 5.4 so I guess
> we could continue doing this for 5.4 as well (allowing you to drop this "fix").
>
> Sorry for the burden (about having to introduce another fix, needed because of
> that patch). I hope nobody else is relying on it and, if they are, there is a
> mitigation described above.
>
> So, feel free to drop it if it's easier for 5.4 maintenance, I'll mitigate
> code on our side.
Thanks for the info.
Lee, can you make up a revert patch for 5.4 with the above information
in it so that I can queue it up?
thanks,
greg k-h
