public inbox for libabigail@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Narrow Linux symbol CRCs to 32 bits
@ 2022-10-27 14:09 Giuliano Procida
  2022-10-27 15:53 ` Giuliano Procida
  2022-11-11 13:21 ` [PATCH v2] " Giuliano Procida
  0 siblings, 2 replies; 8+ messages in thread
From: Giuliano Procida @ 2022-10-27 14:09 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich, sidnayyar, vvvvvv

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. 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.

This change resolves the problem by narrowing libabigail's concept of
Linux CRC to 32 bits. No tests are affected.

	* 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Narrow Linux symbol CRCs to 32 bits
  2022-10-27 14:09 [PATCH] Narrow Linux symbol CRCs to 32 bits Giuliano Procida
@ 2022-10-27 15:53 ` Giuliano Procida
  2022-10-28 15:13   ` Dodji Seketeli
  2022-11-11 13:21 ` [PATCH v2] " Giuliano Procida
  1 sibling, 1 reply; 8+ messages in thread
From: Giuliano Procida @ 2022-10-27 15:53 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, maennich, sidnayyar, vvvvvv

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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Narrow Linux symbol CRCs to 32 bits
  2022-10-27 15:53 ` Giuliano Procida
@ 2022-10-28 15:13   ` Dodji Seketeli
  2022-10-28 16:08     ` Giuliano Procida
  0 siblings, 1 reply; 8+ messages in thread
From: Dodji Seketeli @ 2022-10-28 15:13 UTC (permalink / raw)
  To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich, sidnayyar, vvvvvv

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 am fine with either way. 

[...]

Cheers,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Narrow Linux symbol CRCs to 32 bits
  2022-10-28 15:13   ` Dodji Seketeli
@ 2022-10-28 16:08     ` Giuliano Procida
  2022-11-17 10:13       ` Dodji Seketeli
  0 siblings, 1 reply; 8+ messages in thread
From: Giuliano Procida @ 2022-10-28 16:08 UTC (permalink / raw)
  To: Dodji Seketeli; +Cc: libabigail, kernel-team, maennich, sidnayyar, vvvvvv

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.
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] Narrow Linux symbol CRCs to 32 bits
  2022-10-27 14:09 [PATCH] Narrow Linux symbol CRCs to 32 bits Giuliano Procida
  2022-10-27 15:53 ` Giuliano Procida
@ 2022-11-11 13:21 ` Giuliano Procida
  2022-11-11 15:44   ` Giuliano Procida
  2022-11-17 10:10   ` Dodji Seketeli
  1 sibling, 2 replies; 8+ messages in thread
From: Giuliano Procida @ 2022-11-11 13:21 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich, sidnayyar, vvvvvv

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.

	* 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.1.493.g58b659f92b-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] Narrow Linux symbol CRCs to 32 bits
  2022-11-11 13:21 ` [PATCH v2] " Giuliano Procida
@ 2022-11-11 15:44   ` Giuliano Procida
  2022-11-17 10:10   ` Dodji Seketeli
  1 sibling, 0 replies; 8+ messages in thread
From: Giuliano Procida @ 2022-11-11 15:44 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, maennich, sidnayyar, vvvvvv

I forgot to mention.

A colleague is working on a fix that will continue from this commit.
Should be ready for review next week.

Giuliano.

On Fri, 11 Nov 2022 at 13:22, Giuliano Procida <gprocida@google.com> wrote:
>
> 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.
>
>         * 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.1.493.g58b659f92b-goog
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] Narrow Linux symbol CRCs to 32 bits
  2022-11-11 13:21 ` [PATCH v2] " Giuliano Procida
  2022-11-11 15:44   ` Giuliano Procida
@ 2022-11-17 10:10   ` Dodji Seketeli
  1 sibling, 0 replies; 8+ messages in thread
From: Dodji Seketeli @ 2022-11-17 10:10 UTC (permalink / raw)
  To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich, sidnayyar, vvvvvv

Hello Giuliano,

Giuliano Procida <gprocida@google.com> a écrit:

> 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.
>
> 	* 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.

Applied to master, thanks!

[...]

Cheers,


-- 
		Dodji

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Narrow Linux symbol CRCs to 32 bits
  2022-10-28 16:08     ` Giuliano Procida
@ 2022-11-17 10:13       ` Dodji Seketeli
  0 siblings, 0 replies; 8+ messages in thread
From: Dodji Seketeli @ 2022-11-17 10:13 UTC (permalink / raw)
  To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich, sidnayyar, vvvvvv

Hello,

[...]

On Fri, 28 Oct 2022 at 16:13, Dodji Seketeli <dodji@seketeli.org> wrote:

>> 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?

Giuliano Procida <gprocida@google.com> a écrit:

> 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.

This felt through the cracks on my end, sorry about that.

I have just applied the v2 of the patch that you've sent 5 days ago.

Thank you for that.

[...]

Cheers,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-11-17 10:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-27 14:09 [PATCH] Narrow Linux symbol CRCs to 32 bits Giuliano Procida
2022-10-27 15:53 ` Giuliano Procida
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

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).