Date:   Wed, 2 Mar 2022 18:46:45 +0100
From:   Michal Suchánek <msuchanek@...e.de>
To:     Shung-Hsi Yu <shung-hsi.yu@...e.com>
Cc:     Yonghong Song <yhs@...com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Connor O'Brien <connoro@...gle.com>, 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 Wed, Feb 16, 2022 at 03:38:31AM +0800, 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@...com> 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)

At least not this way because you have different number of entries which
was the original issue.

So is this possible at least theoretically, and how likely will that
happen in practice?

What else other than the number of entries has to match?

Thanks

Michal

Powered by blists - more mailing lists