On Tue, Feb 15, 2022 at 10:58 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> Yonghong Song <yhs@xxxxxx> writes:
>
> > On 2/15/22 11:38 AM, Shung-Hsi Yu wrote:
> >> On Fri, Feb 11, 2022 at 10:36:28PM -0800, Yonghong Song wrote:
> >>> On 2/11/22 9:40 PM, Shung-Hsi Yu wrote:
> >>>> On Thu, Feb 10, 2022 at 02:59:03PM -0800, Yonghong Song wrote:
> >>>>> On 2/10/22 2:34 PM, Alexei Starovoitov wrote:
> >>>>>> On Thu, Feb 10, 2022 at 10:17 AM Yonghong Song <yhs@xxxxxx> wrote:
> >>>>>>> On 2/10/22 2:01 AM, Michal Suchánek wrote:
> >>>>>>>> On Mon, Jan 31, 2022 at 09:36:44AM -0800, Yonghong Song wrote:
> >>>>>>>>> On 1/27/22 7:10 AM, Shung-Hsi Yu wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> We recently run into module load failure related to split BTF on openSUSE
> >>>>>>>>>> Tumbleweed[1], which I believe is something that may also happen on other
> >>>>>>>>>> rolling distros.
> >>>>>>>>>>
> >>>>>>>>>> The error looks like the follow (though failure is not limited to ipheth)
> >>>>>>>>>>
> >>>>>>>>>> BPF:[103111] STRUCT BPF:size=152 vlen=2 BPF: BPF:Invalid name BPF:
> >>>>>>>>>>
> >>>>>>>>>> failed to validate module [ipheth] BTF: -22
> >>>>>>>>>>
> >>>>>>>>>> The error comes down to trying to load BTF of *kernel modules from a
> >>>>>>>>>> different build* than the runtime kernel (but the source is the same), where
> >>>>>>>>>> the base BTF of the two build is different.
> >>>>>>>>>>
> >>>>>>>>>> While it may be too far stretched to call this a bug, solving this might
> >>>>>>>>>> make BTF adoption easier. I'd natively think that we could further split
> >>>>>>>>>> base BTF into two part to avoid this issue, where .BTF only contain exported
> >>>>>>>>>> types, and the other (still residing in vmlinux) holds the unexported types.
> >>>>>>>>>
> >>>>>>>>> What is the exported types? The types used by export symbols?
> >>>>>>>>> This for sure will increase btf handling complexity.
> >>>>>>>>
> >>>>>>>> And it will not actually help.
> >>>>>>>>
> >>>>>>>> We have modversion ABI which checks the checksum of the symbols that the
> >>>>>>>> module imports and fails the load if the checksum for these symbols does
> >>>>>>>> not match. It's not concerned with symbols not exported, it's not
> >>>>>>>> concerned with symbols not used by the module. This is something that is
> >>>>>>>> sustainable across kernel rebuilds with minor fixes/features and what
> >>>>>>>> distributions watch for.
> >>>>>>>>
> >>>>>>>> Now with BTF the situation is vastly different. There are at least three
> >>>>>>>> bugs:
> >>>>>>>>
> >>>>>>>> - The BTF check is global for all symbols, not for the symbols the
> >>>>>>>> module uses. This is not sustainable. Given the BTF is supposed to
> >>>>>>>> allow linking BPF programs that were built in completely different
> >>>>>>>> environment with the kernel it is completely within the scope of BTF
> >>>>>>>> to solve this problem, it's just neglected.
> >>>>>>>> - It is possible to load modules with no BTF but not modules with
> >>>>>>>> non-matching BTF. Surely the non-matching BTF could be discarded.
> >>>>>>>> - BTF is part of vermagic. This is completely pointless since modules
> >>>>>>>> without BTF can be loaded on BTF kernel. Surely it would not be too
> >>>>>>>> difficult to do the reverse as well. Given BTF must pass extra check
> >>>>>>>> to be used having it in vermagic is just useless moise.
> >>>>>>>>
> >>>>>>>>>> Does that sound like something reasonable to work on?
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> ## Root case (in case anyone is interested in a verbose version)
> >>>>>>>>>>
> >>>>>>>>>> On openSUSE Tumbleweed there can be several builds of the same source. Since
> >>>>>>>>>> the source is the same, the binaries are simply replaced when a package with
> >>>>>>>>>> a larger build number is installed during upgrade.
> >>>>>>>>>>
> >>>>>>>>>> In our case, a rebuild is triggered[2], and resulted in changes in base BTF.
> >>>>>>>>>> More precisely, the BTF_KIND_FUNC{,_PROTO} of i2c_smbus_check_pec(u8 cpec,
> >>>>>>>>>> struct i2c_msg *msg) and inet_lhash2_bucket_sk(struct inet_hashinfo *h,
> >>>>>>>>>> struct sock *sk) was added to the base BTF of 5.15.12-1.3. Those functions
> >>>>>>>>>> are previously missing in base BTF of 5.15.12-1.1.
> >>>>>>>>>
> >>>>>>>>> As stated in [2] below, I think we should understand why rebuild is
> >>>>>>>>> triggered. If the rebuild for vmlinux is triggered, why the modules cannot
> >>>>>>>>> be rebuild at the same time?
> >>>>>>>>
> >>>>>>>> They do get rebuilt. However, if you are running the kernel and install
> >>>>>>>> the update you get the new modules with the old kernel. If the install
> >>>>>>>> script fails to copy the kernel to your EFI partition based on the fact
> >>>>>>>> a kernel with the same filename is alreasy there you get the same.
> >>>>>>>>
> >>>>>>>> If you have 'stable' distribution adding new symbols is normal and it
> >>>>>>>> does not break module loading without BTF but it breaks BTF.
> >>>>>>>
> >>>>>>> Okay, I see. One possible solution is that if kernel module btf
> >>>>>>> does not match vmlinux btf, the kernel module btf will be ignored
> >>>>>>> with a dmesg warning but kernel module load will proceed as normal.
> >>>>>>> I think this might be also useful for bpf lskel kernel modules as
> >>>>>>> well which tries to be portable (with CO-RE) for different kernels.
> >>>>>>
> >>>>>> That sounds like #2 that Michal is proposing:
> >>>>>> "It is possible to load modules with no BTF but not modules with
> >>>>>> non-matching BTF. Surely the non-matching BTF could be discarded."
> >>>>
> >>>> Since we're talking about matching check, I'd like bring up another issue.
> >>>>
> >>>> AFAICT with current form of BTF, checking whether BTF on kernel module
> >>>> matches cannot be made entirely robust without a new version of btf_header
> >>>> that contain info about the base BTF.
> >>>
> >>> The base BTF is always the one associated with running kernel and typically
> >>> the BTF is under /sys/kernel/btf/vmlinux. Did I miss
> >>> anything here?
> >>>
> >>>> As effective as the checks are in this case, by detecting a type name being
> >>>> an empty string and thus conclude it's non-matching, with some (bad) luck a
> >>>> non-matching BTF could pass these checks a gets loaded.
> >>>
> >>> Could you be a little bit more specific about the 'bad luck' a
> >>> non-matching BTF could get loaded? An example will be great.
> >>
> >> Let me try take a jab at it. Say here's a hypothetical BTF for a kernel
> >> module which only type information for `struct something *`:
> >>
> >> [5] PTR '(anon)' type_id=4
> >>
> >> Which is built upon the follow base BTF:
> >>
> >> [1] INT 'unsigned char' size=1 bits_offset=0 nr_bits=8 encoding=(none)
> >> [2] PTR '(anon)' type_id=3
> >> [3] STRUCT 'list_head' size=16 vlen=2
> >> 'next' type_id=2 bits_offset=0
> >> 'prev' type_id=2 bits_offset=64
> >> [4] STRUCT 'something' size=2 vlen=2
> >> 'locked' type_id=1 bits_offset=0
> >> 'pending' type_id=1 bits_offset=8
> >>
> >> Due to the situation mentioned in the beginning of the thread, the *runtime*
> >> kernel have a different base BTF, in this case type IDs are offset by 1 due
> >> to an additional typedef entry:
> >>
> >> [1] TYPEDEF 'u8' type_id=1
> >> [2] INT 'unsigned char' size=1 bits_offset=0 nr_bits=8 encoding=(none)
> >> [3] PTR '(anon)' type_id=3
> >> [4] STRUCT 'list_head' size=16 vlen=2
> >> 'next' type_id=2 bits_offset=0
> >> 'prev' type_id=2 bits_offset=64
> >> [5] STRUCT 'something' size=2 vlen=2
> >> 'locked' type_id=1 bits_offset=0
> >> 'pending' type_id=1 bits_offset=8
> >>
> >> Then when loading the BTF on kernel module on the runtime, the kernel will
> >> mistakenly interprets "PTR '(anon)' type_id=4" as `struct list_head *`
> >> rather than `struct something *`.
> >>
> >> Does this should possible? (at least theoretically)
> >
> > Thanks for explanation. Yes, from BTF type resolution point of view,
> > yes it is possible.
>
> Could we add a marker or something to prevent this from happening?
> Something like putting the hash of the entire BTF structure into the
> header and referring to that from the "child"; or really any other way
> of detecting that the combined BTF you're constructing is going to be
> wrong?
>
Extending BTF format (including its header) is quite disrupting to the
entire ecosystem around BTF. Given split BTF is only used for kernel
modules, I think it's a better approach to add checksum to module's
ELF itself (as an extra BTF-related section, .BTF.base_checksum or
whatever) and check it during kernel module loading time.
As for having full BTF. You can do that, and it will work for generic
CO-RE approach, but it might not work for kfunc and other things that
expect that, say, struct task_struct has one specific ID that
corresponds to task_struct BTF ID in vmlinux BTF. If kernel module is
loaded against vmlinux BTF that has just slightly different definition
of task_struct (e.g., one field was added at the end), dedup algorithm
will detect those differences and will preserve module's definition of
task_struct as a separate type, which won't be recognized by kernel as
task_struct.
But again, given it's all module-specific, we can utilize custom
.BTF.* sections to record any such information without disrupting any
other user of BTF, including all the BPF applications out there.
> -Toke
>
