From: Giuliano Procida <gprocida@google.com>
To: libabigail@sourceware.org
Cc: dodji@seketeli.org, kernel-team@android.com, maennich@google.com,
sidnayyar@google.com, vvvvvv@google.com
Subject: Re: [PATCH] Narrow Linux symbol CRCs to 32 bits
Date: Thu, 27 Oct 2022 16:53:40 +0100 [thread overview]
Message-ID: <CAGvU0Hkco6bwbRhzKMJPDodEmZOCTtUUOWhMnp7r2WOrV-OZ7w@mail.gmail.com> (raw)
In-Reply-To: <20221027140928.1480353-1-gprocida@google.com>
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.
>
> * include/abg-ir.h (elf_symbol::elf_symbol): Change CRC type
> from optional<uint64_t> to optional<uint32_t>.
> (elf_symbol::create): Likewise.
> (elf_symbol::get_crc): Likewise.
> (elf_symbol::set_crc): Likewise.
> * src/abg-ir.cc (elf_symbol::priv) Change CRC type from
> optional<uint64_t> to optional<uint32_t>.
> (elf_symbol::priv::priv): Likewise.
> (elf_symbol::elf_symbol): Likewise.
> (elf_symbol::create): Likewise.
> (elf_symbol::get_crc): Likewise.
> (elf_symbol::set_crc): Likewise.
> * src/abg-reporter-priv.cc (maybe_report_diff_for_symbol):
> Change CRC type from optional<uint64_t> to
> optional<uint32_t>.
> * src/abg-symtab-reader.cc (symtab::load_): Change crc_values
> value type from uint64_t to uint32_t.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
> include/abg-ir.h | 8 ++++----
> src/abg-ir.cc | 12 ++++++------
> src/abg-reporter-priv.cc | 4 ++--
> src/abg-symtab-reader.cc | 2 +-
> 4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/include/abg-ir.h b/include/abg-ir.h
> index 4892f0e2..546603f4 100644
> --- a/include/abg-ir.h
> +++ b/include/abg-ir.h
> @@ -940,7 +940,7 @@ private:
> const version& ve,
> visibility vi,
> bool is_in_ksymtab = false,
> - const abg_compat::optional<uint64_t>& crc = {},
> + const abg_compat::optional<uint32_t>& crc = {},
> const abg_compat::optional<std::string>& ns = {},
> bool is_suppressed = false);
>
> @@ -966,7 +966,7 @@ public:
> const version& ve,
> visibility vi,
> bool is_in_ksymtab = false,
> - const abg_compat::optional<uint64_t>& crc = {},
> + const abg_compat::optional<uint32_t>& crc = {},
> const abg_compat::optional<std::string>& ns = {},
> bool is_suppressed = false);
>
> @@ -1039,11 +1039,11 @@ public:
> void
> set_is_in_ksymtab(bool is_in_ksymtab);
>
> - const abg_compat::optional<uint64_t>&
> + const abg_compat::optional<uint32_t>&
> get_crc() const;
>
> void
> - set_crc(const abg_compat::optional<uint64_t>& crc);
> + set_crc(const abg_compat::optional<uint32_t>& crc);
>
> const abg_compat::optional<std::string>&
> get_namespace() const;
> diff --git a/src/abg-ir.cc b/src/abg-ir.cc
> index 5193934a..a043a532 100644
> --- a/src/abg-ir.cc
> +++ b/src/abg-ir.cc
> @@ -1788,7 +1788,7 @@ struct elf_symbol::priv
> // STT_COMMON definition of that name that has the largest size.
> bool is_common_;
> bool is_in_ksymtab_;
> - abg_compat::optional<uint64_t> crc_;
> + abg_compat::optional<uint32_t> crc_;
> abg_compat::optional<std::string> namespace_;
> bool is_suppressed_;
> elf_symbol_wptr main_symbol_;
> @@ -1822,7 +1822,7 @@ struct elf_symbol::priv
> const elf_symbol::version& ve,
> elf_symbol::visibility vi,
> bool is_in_ksymtab,
> - const abg_compat::optional<uint64_t>& crc,
> + const abg_compat::optional<uint32_t>& crc,
> const abg_compat::optional<std::string>& ns,
> bool is_suppressed)
> : env_(e),
> @@ -1896,7 +1896,7 @@ elf_symbol::elf_symbol(const environment* e,
> const version& ve,
> visibility vi,
> bool is_in_ksymtab,
> - const abg_compat::optional<uint64_t>& crc,
> + const abg_compat::optional<uint32_t>& crc,
> const abg_compat::optional<std::string>& ns,
> bool is_suppressed)
> : priv_(new priv(e,
> @@ -1971,7 +1971,7 @@ elf_symbol::create(const environment* e,
> const version& ve,
> visibility vi,
> bool is_in_ksymtab,
> - const abg_compat::optional<uint64_t>& crc,
> + const abg_compat::optional<uint32_t>& crc,
> const abg_compat::optional<std::string>& ns,
> bool is_suppressed)
> {
> @@ -2208,7 +2208,7 @@ elf_symbol::set_is_in_ksymtab(bool is_in_ksymtab)
> /// Getter of the 'crc' property.
> ///
> /// @return the CRC (modversions) value for Linux Kernel symbols, if any
> -const abg_compat::optional<uint64_t>&
> +const abg_compat::optional<uint32_t>&
> elf_symbol::get_crc() const
> {return priv_->crc_;}
>
> @@ -2216,7 +2216,7 @@ elf_symbol::get_crc() const
> ///
> /// @param crc the new CRC (modversions) value for Linux Kernel symbols
> void
> -elf_symbol::set_crc(const abg_compat::optional<uint64_t>& crc)
> +elf_symbol::set_crc(const abg_compat::optional<uint32_t>& crc)
> {priv_->crc_ = crc;}
>
> /// Getter of the 'namespace' property.
> diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
> index 2ecf8016..70839085 100644
> --- a/src/abg-reporter-priv.cc
> +++ b/src/abg-reporter-priv.cc
> @@ -1149,8 +1149,8 @@ maybe_report_diff_for_symbol(const elf_symbol_sptr& symbol1,
> << "\n";
> }
>
> - const abg_compat::optional<uint64_t>& crc1 = symbol1->get_crc();
> - const abg_compat::optional<uint64_t>& crc2 = symbol2->get_crc();
> + const abg_compat::optional<uint32_t>& crc1 = symbol1->get_crc();
> + const abg_compat::optional<uint32_t>& crc2 = symbol2->get_crc();
> if (crc1 != crc2)
> {
> const std::string none = "(none)";
> diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
> index 606c96a2..8a566f8d 100644
> --- a/src/abg-symtab-reader.cc
> +++ b/src/abg-symtab-reader.cc
> @@ -265,7 +265,7 @@ symtab::load_(Elf* elf_handle,
>
> const bool is_kernel = elf_helpers::is_linux_kernel(elf_handle);
> std::unordered_set<std::string> exported_kernel_symbols;
> - std::unordered_map<std::string, uint64_t> crc_values;
> + std::unordered_map<std::string, uint32_t> crc_values;
> std::unordered_map<std::string, std::string> namespaces;
>
> for (size_t i = 0; i < number_syms; ++i)
> --
> 2.38.0.135.g90850a2211-goog
>
next prev parent reply other threads:[~2022-10-27 15:54 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-27 14:09 Giuliano Procida
2022-10-27 15:53 ` Giuliano Procida [this message]
2022-10-28 15:13 ` Dodji Seketeli
2022-10-28 16:08 ` Giuliano Procida
2022-11-17 10:13 ` Dodji Seketeli
2022-11-11 13:21 ` [PATCH v2] " Giuliano Procida
2022-11-11 15:44 ` Giuliano Procida
2022-11-17 10:10 ` Dodji Seketeli
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=CAGvU0Hkco6bwbRhzKMJPDodEmZOCTtUUOWhMnp7r2WOrV-OZ7w@mail.gmail.com \
--to=gprocida@google.com \
--cc=dodji@seketeli.org \
--cc=kernel-team@android.com \
--cc=libabigail@sourceware.org \
--cc=maennich@google.com \
--cc=sidnayyar@google.com \
--cc=vvvvvv@google.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 read-only IMAP folder(s) and NNTP newsgroup(s).