Re: [PATCH] Narrow Linux symbol CRCs to 32 bits

[Giuliano Procida <gprocida@google.com>]

Hi Dodji.

On Fri, 28 Oct 2022 at 16:13, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello Giuliano,
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > Self-reply.
> >
> > On Thu, 27 Oct 2022 at 15:09, Giuliano Procida <gprocida@google.com> wrote:
> >>
> >> MODVERSIONS CRCs are 32-bit hashes of strings representing C type
> >> elements or typed symbols. The hash is calculated using a 32-bit CRC,
> >> hence the name. The kernel module loading code (implicitly) truncates
> >> any provided CRC value to 32 bits before comparing it with anything.
> >>
> >> When support was added to libabigail, values up to 64 bits wide were
> >> supported.
> >
> > True so far.
> >
> >> Recently, Linux kernel builds have now started generating
> >> ELF CRC symbols with 64-bit values (where the low 32 bits are the
> >> CRC). Together this has resulted in incorrect CRCs in ABIs.
> >
> > The actual problem is a change to how CRCs are emitted in kernel objects.
> > I think the change 7b4537199a4a8480b8c3ba37a2d44765ce76cd9b was
> > responsible.
> >
> >> This change resolves the problem by narrowing libabigail's concept of
> >> Linux CRC to 32 bits. No tests are affected.
> >
> > The representation change is fine. The problem with bad CRCs is still
> > there.
>
> The changes look good to me.  Do you want me to apply it right now, or
> do you prefer to send a subsequent patch that addresses the new way how
> CRCs are emitted in kernel object?

I'm happy for the change to go in. However, the commit message text
should be updated. I would make it:

--

MODVERSIONS CRCs are 32-bit hashes of strings representing C type
elements or typed symbols. The hashes are calculated using a 32-bit CRC,
hence the name. The kernel module loading code (implicitly) truncates
any provided CRC value to 32 bits before comparing it with anything.

When support was added to libabigail, values up to 64 bits wide were
supported. This change narrows libabigail's concept of Linux CRC to 32
bits. No tests are affected.

--

I'm not sure when we'll get around to updating the symtab reader. It'll
probably not be done by me. As I understand things, there are
something like 3 or 4 different encodings of CRCs depending on the
kernel version and architecture (and that's ignoring endianness issues).

Matthias has even suggested it might be better (more reliable) just reading
CRCs from Modules.symvers instead. However, that would require more
integration into build scripts etc.

> I am fine with either way.
>
> [...]
>
> Cheers,
>
> --
>                 Dodji
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>