Message-ID: <CABCJKueGRBdFfGW-cvOvqxc-a85GpxtwPmLdE_1RiAkNLrEg+g@mail.gmail.com>
Date: Mon, 15 Jul 2024 20:39:59 +0000
From: Sami Tolvanen <samitolvanen@...gle.com>
To: Petr Pavlu <petr.pavlu@...e.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	Miguel Ojeda <ojeda@...nel.org>, Matthew Maurer <mmaurer@...gle.com>, 
	Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, 
	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 00/15] Implement MODVERSIONS for Rust

Hi Petr,

On Wed, Jul 10, 2024 at 7:30 AM Petr Pavlu <petr.pavlu@...e.com> wrote:
>
> On 6/17/24 19:58, Sami Tolvanen wrote:
> > Hi folks,
> >
> > This series implements CONFIG_MODVERSIONS for Rust, an important
> > feature for distributions like Android that want to ship Rust
> > kernel modules, and depend on modversions to help ensure module ABI
> > compatibility.
>
> Thanks for working on this. Below is some feedback with my (open)SUSE
> hat on, although it should be quite general.

Great, thanks for taking a look!

> > There have been earlier proposals [1][2] that would allow Rust
> > modules to coexist with modversions, but none that actually implement
> > symbol versioning. Unlike C, Rust source code doesn't have sufficient
> > information about the final ABI, as the compiler has considerable
> > freedom in adjusting structure layout for improved performance [3],
> > for example, which makes using a source code parser like genksyms
> > a non-starter. Based on Matt's suggestion and previous feedback
> > from maintainers, this series uses DWARF debugging information for
> > computing versions. DWARF is an established and relatively stable
> > format, which includes all the necessary ABI details, and adding a
> > CONFIG_DEBUG_INFO dependency for Rust symbol versioning seems like a
> > reasonable trade-off.
>
> Using the DWARF data makes sense to me. Distribution kernels are
> normally built with debuginfo because one has to be able to debug them
> later, unsurprisingly. Besides that, one now typically wants to use BPF
> together with built-in BTF data (CONFIG_DEBUG_INFO_BTF), which also
> requires building the kernel with debuginfo.
>
> I would however keep in mind that producing all DWARF data has some
> cost. From a quick test, an x86_64-defconfig build is ~30% slower for me
> with CONFIG_DEBUG_INFO=y. The current genksyms tool allows to have
> debuginfo disabled when backporting some patches and consequently have
> a quicker feedback whether modversions changed. This option would
> disappear with gendwarfksyms but I think it is acceptable.

Yes, this matches my benchmarks too. Presumably folks who care about
build times and are not interested in debugging information or Rust
support would prefer genksyms instead. I'm planning to turn this into
a Kconfig choice in the next version.

> > The first 12 patches of this series add a small tool for computing
> > symbol versions from DWARF, called gendwarfksyms. When passed a list
> > of exported symbols, the tool generates an expanded type string
> > for each symbol, and computes symbol CRCs similarly to genksyms.
> > gendwarfksyms is written in C and uses libdw to process DWARF, mainly
> > because of the existing support for C host tools that use elfutils
> > (e.g., objtool).
>
> In addition to calculating CRCs of exported symbols, genksyms has other
> features which I think are important.
>
> Firstly, the genksyms tool has a human-readable storage format for input
> data used in the calculation of symbol CRCs. Setting the make variable
> KBUILD_SYMTYPES enables dumping this data and storing it in *.symtypes
> files.
>
> When a developer later modifies the kernel and wants to check if some
> symbols have changed, they can take these files and feed them as
> *.symref back to genksyms. This allows the tool to provide an actual
> reason why some symbols have changed, instead of just printing that
> their CRCs are different.
>
> Is there any plan to add the same functionality to gendwarfksyms, or do
> you envison that people will use libabigail, Symbol-Type Graph, or
> another tool for making this type of comparison?

gendwarfksyms also uses human-readable input for the CRC calculations,
and it prints out the input strings with the --debug option. I plan to
hook this up to KBUILD_SYMTYPES in v2. It should be convenient enough
to simply compare the pretty-printed output with diff, so I'm not sure
if a built-in comparison option is needed. Any other DWARF analysis
tool can be used to spot the differences too, as you mentioned.

> Secondly, when distributions want to maintain stable kABI, they need to
> be able to deal with patch backports that add new members to structures.
> One common approach is to have placeholders in important structures
> which can be later replaced by the new members as needed. __GENKSYMS__
> ifdefs are then used at the C source level to hide these kABI-compatible
> changes from genksyms.
>
> Gendwarfksyms works on the resulting binary and so using such ifdefs
> wouldn't work. Instead, I suspect that what is required is a mechanism
> to tell the tool that a given change is ok, probably by allowing to
> specify some map from the original definition to the new one.
>
> Is there a plan to implement something like this, or how could it be
> addressed?

That's a great question. Here's what Android uses currently to
maintain a stable kABI, I assume you're doing something similar?

https://android.googlesource.com/kernel/common/+/refs/heads/android15-6.6/include/linux/android_kabi.h

If using unions here is acceptable to everyone, a simple solution
would be to use a known name prefix for the reserved members and teach
gendwarfksyms to only print out the original type for the replaced
ones. For example:

The initial placeholder:

    u8 __kabi_reserved_1[8];

After replacement:

    union {
            u64 new_member;
            struct {
                    u8 __kabi_reserved_1[8];
            };
    }

Here gendwarfksyms would see the __kabi_reserved prefix and only use
u8 [8] for the CRC calculation. Does this sound reasonable?

Greg, I know you've been dealing with this for a long time, any thoughts?

> > Another compatibility issue is fitting the extremely long mangled
> > Rust symbol names into struct modversion_info, which can only hold
> > 55-character names (on 64-bit systems). Previous proposals suggested
> > changing the structure to support longer names, but the conclusion was
> > that we cannot break userspace tools that parse the version table.
> >
> > The next two patches of the series implement support for hashed
> > symbol names in struct modversion_info, where names longer than 55
> > characters are hashed, and the hash is stored in the name field. To
> > avoid breaking userspace tools, the binary hash is prefixed with a
> > null-terminated string containing the name of the hash function. While
> > userspace tools can later be updated to potentially produce more
> > useful information about the long symbols, this allows them to
> > continue working in the meantime.
>
> I think this approach with hashed names is quite complex. I'd personally
> be also in favor of having a new section with variable-length strings to
> store the names of imported symbols. As yet another alternative, it
> should be also possible to refer directly into .symtab/.strtab to avoid
> duplicating the names, but I suspect it would be non-trivial to
> implement.

Thanks for the feedback. I think for the next version we'll look into
reviving the relevant bits of Matt's previous patch series, which
implemented a new section that supports variable-length names:

https://lore.kernel.org/lkml/CAGSQo03R+HYcX2JJDSm7LA8V370s_3hrbTBc2s51Pp9nxWTz5w@mail.gmail.com/

Sami

Powered by blists - more mailing lists