Date:   Fri, 11 Feb 2022 22:36:28 -0800
From:   Yonghong Song <yhs@...com>
To:     Shung-Hsi Yu <shung-hsi.yu@...e.com>
CC:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Connor O'Brien <connoro@...gle.com>,
        Michal Suchánek <msuchanek@...e.de>,
        bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>
Subject: Re: BTF compatibility issue across builds



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@...com> wrote:
>>>> On 2/10/22 2:01 AM, Michal Suchánek wrote:
>>>>> Hello,
>>>>>
>>>>> 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.


> 
>>> That's probably the simplest way forward.
>>>
>>> The patch
>>> https://patchwork.kernel.org/project/netdevbpf/patch/20220209052141.140063-1-connoro@google.com/
>>> shouldn't be necessary too.
>>
>> Right the patch tried to address this issue and if we allow
>> non-matching BTF is ignored and then treaking DEBUG_INFO_BTF_MODULES
>> is not necessary.
> 
> Not being able to load kernel module with non-matching BTF and the absence
> of robust matching check are the two reasons that lead us to the same path
> of disabling DEBUG_INFO_BTF_MODULES a while back.
> 
> Ignoring non-matching BTF will solve the former, but not the latter, so I'd
> hope that the above patch get's taken (though I'm obviously biased).
> 

Powered by blists - more mailing lists