From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd29.google.com (mail-io1-xd29.google.com [IPv6:2607:f8b0:4864:20::d29]) by sourceware.org (Postfix) with ESMTPS id 70EA43851520 for ; Thu, 27 Oct 2022 15:54:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 70EA43851520 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=google.com Received: by mail-io1-xd29.google.com with SMTP id h188so1921885iof.1 for ; Thu, 27 Oct 2022 08:54:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=idm0DZDGHshXMAVsuke2FF4hUA9lm/7Cyaekr5OUA3E=; b=ICnosKwHY1EbzZl99n/Pw4/wtyKrq6c5GcFZokt6SpvHFp7OGnjbyTd2NULv1ZJ7kv BYY98zYD/TPsJkidf2AiI3e2hsMZJrhXWyC9Hzx8qCZ8+jAIxAN8Pqc0Vw1RKEndnUjq Vx1/CA7/mwf1YJd4j7o61QrgZhlR4fCOroXxUn9q2a3AMSKA7LzZxzCxKqhkzxdDrLIl gJLSGSwIT0JsDuKo0e+G5ZsVlSSDhOyaWYj4WCTGTO5JfZ7hnNJcsTFu/dES1JAHQEMd jM24DVrO/UDRYaT1rB+P76l/+XDEy5ptNwVl+TD2JWdWeRQH/xbmutDM9kKrRQX49Uv+ dj3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=idm0DZDGHshXMAVsuke2FF4hUA9lm/7Cyaekr5OUA3E=; b=zh5NcB6hM2CujgThR9fY6z5givUrRWL2Iatb2pzJyQlhLzP7DRYekuLQB10qiGby3v eQhuGaR1yPekBahcQLQL9KW7x5kF9J2xGjYzfx9rcwSKaAxUfojrNx0pCD2UicX2vjRV FmL+aJaBjzbJRfAPAXfcEwa4mQfq6sxpqEN5/DkgPGzrquP+SxYmofftO9uV5tbAwOs0 eW/nsIgEFaY9V2QGYvd/SrfomjUfqTnveNbkXuJUWZ9yLZFyBz0FH/8lzDMrI+BkfYJr nBLD3dNHKDdVlfJCb9BuRdBTboO4MPen1F1VToSpV4nbAUiz7bQ7QxMbh94fypWM1qTy o0ow== X-Gm-Message-State: ACrzQf2kGTM2/TJeFZaOZqmz9DEN3sQEF8FiQLQsJO0nfAnDLc2hut1V KU5d/DN7mdwPaZ4lgHPuj/MfbwdBl4BCrk0e+aITrSN/2VYd0Q== X-Google-Smtp-Source: AMsMyM6oHS9bHpiNMrBzw33IM+RJjAbk+DyTgKwKP1C5nK+b53kO2IDIjyeL8IU8Fw4HBz51R1pNtxuTKZaroGuST5Q= X-Received: by 2002:a05:6602:1494:b0:6c7:77b7:1533 with SMTP id a20-20020a056602149400b006c777b71533mr1055146iow.48.1666886057156; Thu, 27 Oct 2022 08:54:17 -0700 (PDT) MIME-Version: 1.0 References: <20221027140928.1480353-1-gprocida@google.com> In-Reply-To: <20221027140928.1480353-1-gprocida@google.com> From: Giuliano Procida Date: Thu, 27 Oct 2022 16:53:40 +0100 Message-ID: Subject: Re: [PATCH] Narrow Linux symbol CRCs to 32 bits To: libabigail@sourceware.org Cc: dodji@seketeli.org, kernel-team@android.com, maennich@google.com, sidnayyar@google.com, vvvvvv@google.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-27.2 required=5.0 tests=BAYES_00,DKIMWL_WL_MED,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,USER_IN_DEF_DKIM_WL,USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Self-reply. On Thu, 27 Oct 2022 at 15:09, Giuliano Procida 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 to optional. > (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 to optional. > (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 to > optional. > * src/abg-symtab-reader.cc (symtab::load_): Change crc_values > value type from uint64_t to uint32_t. > > Signed-off-by: Giuliano Procida > --- > 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& crc = {}, > + const abg_compat::optional& crc = {}, > const abg_compat::optional& ns = {}, > bool is_suppressed = false); > > @@ -966,7 +966,7 @@ public: > const version& ve, > visibility vi, > bool is_in_ksymtab = false, > - const abg_compat::optional& crc = {}, > + const abg_compat::optional& crc = {}, > const abg_compat::optional& ns = {}, > bool is_suppressed = false); > > @@ -1039,11 +1039,11 @@ public: > void > set_is_in_ksymtab(bool is_in_ksymtab); > > - const abg_compat::optional& > + const abg_compat::optional& > get_crc() const; > > void > - set_crc(const abg_compat::optional& crc); > + set_crc(const abg_compat::optional& crc); > > const abg_compat::optional& > 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 crc_; > + abg_compat::optional crc_; > abg_compat::optional 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& crc, > + const abg_compat::optional& crc, > const abg_compat::optional& 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& crc, > + const abg_compat::optional& crc, > const abg_compat::optional& 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& crc, > + const abg_compat::optional& crc, > const abg_compat::optional& 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& > +const abg_compat::optional& > 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& crc) > +elf_symbol::set_crc(const abg_compat::optional& 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& crc1 = symbol1->get_crc(); > - const abg_compat::optional& crc2 = symbol2->get_crc(); > + const abg_compat::optional& crc1 = symbol1->get_crc(); > + const abg_compat::optional& 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 exported_kernel_symbols; > - std::unordered_map crc_values; > + std::unordered_map crc_values; > std::unordered_map namespaces; > > for (size_t i = 0; i < number_syms; ++i) > -- > 2.38.0.135.g90850a2211-goog >