bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Yonghong Song <yhs@fb.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Connor O'Brien <connoro@google.com>
Cc: "Michal Suchánek" <msuchanek@suse.de>,
	"Shung-Hsi Yu" <shung-hsi.yu@suse.com>, bpf <bpf@vger.kernel.org>,
	"Network Development" <netdev@vger.kernel.org>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Alexei Starovoitov" <ast@kernel.org>
Subject: Re: BTF compatibility issue across builds
Date: Thu, 10 Feb 2022 14:59:03 -0800	[thread overview]
Message-ID: <992ae1d2-0b26-3417-9c6b-132c8fcca0ad@fb.com> (raw)
In-Reply-To: <CAADnVQJ+OVPnBz8z3vNu8gKXX42jCUqfuvhWAyCQDu8N_yqqwQ@mail.gmail.com>



On 2/10/22 2:34 PM, Alexei Starovoitov wrote:
> On Thu, Feb 10, 2022 at 10:17 AM Yonghong Song <yhs@fb.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."
> 
> 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.

  reply	other threads:[~2022-02-10 22:59 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-27 15:10 BTF compatibility issue across builds Shung-Hsi Yu
2022-01-31 17:36 ` Yonghong Song
2022-02-10 10:01   ` Michal Suchánek
2022-02-10 18:17     ` Yonghong Song
2022-02-10 22:34       ` Alexei Starovoitov
2022-02-10 22:59         ` Yonghong Song [this message]
2022-02-12  5:40           ` Shung-Hsi Yu
2022-02-12  6:36             ` Yonghong Song
2022-02-15 19:38               ` Shung-Hsi Yu
2022-02-15 17:47                 ` Yonghong Song
2022-02-15 18:57                   ` Toke Høiland-Jørgensen
2022-02-20  0:28                     ` Andrii Nakryiko
2022-02-16  8:48                   ` David Laight
2022-03-02 17:46                 ` Michal Suchánek
2022-03-03  4:27                   ` Shung-Hsi Yu
2022-02-11  6:01     ` Andrii Nakryiko
2022-02-11 17:20       ` Toke Høiland-Jørgensen
2022-02-11 22:20         ` Andrii Nakryiko
2022-02-11 23:58           ` Toke Høiland-Jørgensen
2022-02-12  7:37             ` Shung-Hsi Yu
2022-02-13 15:40               ` Toke Høiland-Jørgensen
2022-02-14 20:19                 ` Michal Suchánek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=992ae1d2-0b26-3417-9c6b-132c8fcca0ad@fb.com \
    --to=yhs@fb.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=connoro@google.com \
    --cc=daniel@iogearbox.net \
    --cc=msuchanek@suse.de \
    --cc=netdev@vger.kernel.org \
    --cc=shung-hsi.yu@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).