Re: [PATCH v1] abg-dwarf-reader: detect kernel modules without exports as such

Re: [PATCH v1] abg-dwarf-reader: detect kernel modules without exports as such

[Mark Wielaard]

Hi,

On Wed, Jul 24, 2019 at 10:32:55PM +0100, Matthias Maennich via libabigail wrote:
> Kernel modules without exported symbols (no use of EXPORT_SYMBOL*()),
> will not have a __ksymtab_strings section. Libabigail will therefore
> assume they are usual ELF binaries. That leads to wrong results as
> now all ELF symbols are considered part of the ABI. That is obviously
> wrong. Instead consider binaries having a .modinfo section to be kernel
> binaries. We keep the __ksymtab_strings condition as vmlinux has no
> .modinfo section but a __ksymtab_strings if symbols are exported.
> [...] 
> 	* src/abg-dwarf-reader.cc(is_linux_kernel_binary): consider
> 	  binaries only having a .modinfo section to be kernel binaries
> [...]
>    bool
>    is_linux_kernel_binary() const
> -  {return find_section(elf_handle(), "__ksymtab_strings", SHT_PROGBITS);}
> +  {
> +    return find_section(elf_handle(), "__ksymtab_strings", SHT_PROGBITS)
> +	   || find_section(elf_handle(), ".modinfo", SHT_PROGBITS);
> +  }

I think the change itself is correct and better than what is there
now.

But this is interesting to me because we are discussing an
eu-elfclassify program addition for elfutils. That utility at the
moment classifies a --linux-kernel-module as:

/* Returns true if the file is a linux kernel module (is ET_REL and
   has the two magic sections .modinfo and .gnu.linkonce.this_module).  */
static bool
is_linux_kernel_module (void)
{
  return (elf_kind (elf) == ELF_K_ELF
          && elf_type == ET_REL
          && has_modinfo
          && has_gnu_linkonce_this_module);
}

Just wondering if you believe that check is too strict?

The reason to check for both "magic" section names was mainly because
".modinfo" seemed a little too generic.

One difference is that eu-elfclassify doesn't check the section type.
Maybe it should do that like here, to make sure that it is
SHT_PROGBITS.

Sorry for hijacking this patch review for something slightly
different. But I was happy to see something very similar to what I was
working on in a different, but somewhat related project, and was
wondering whether it was the correct way to classify something as a
kernel module.

Cheers,

Mark

Re: [PATCH v1] abg-dwarf-reader: detect kernel modules without exports as such

[Dodji Seketeli]

Hello Mark,

Mark Wielaard <mark@klomp.org> a écrit:

[...]

> But this is interesting to me because we are discussing an
> eu-elfclassify program addition for elfutils. That utility at the
> moment classifies a --linux-kernel-module as:
>
> /* Returns true if the file is a linux kernel module (is ET_REL and
>    has the two magic sections .modinfo and .gnu.linkonce.this_module).  */
> static bool
> is_linux_kernel_module (void)
> {
>   return (elf_kind (elf) == ELF_K_ELF
>           && elf_type == ET_REL
>           && has_modinfo
>           && has_gnu_linkonce_this_module);
> }
>
> Just wondering if you believe that check is too strict?

FWIW, this looks just fine to me.

I'd let others chime in in case I missed something important :-)

Cheers,

-- 
		Dodji

Re: [PATCH v1] abg-dwarf-reader: detect kernel modules without exports as such

[Matthias Maennich via libabigail]

Hi Mark!

On Wed, Jul 24, 2019 at 11:57:10PM +0200, Mark Wielaard wrote:
> Hi,
> 
> On Wed, Jul 24, 2019 at 10:32:55PM +0100, Matthias Maennich via libabigail wrote:
> > Kernel modules without exported symbols (no use of EXPORT_SYMBOL*()),
> > will not have a __ksymtab_strings section. Libabigail will therefore
> > assume they are usual ELF binaries. That leads to wrong results as
> > now all ELF symbols are considered part of the ABI. That is obviously
> > wrong. Instead consider binaries having a .modinfo section to be kernel
> > binaries. We keep the __ksymtab_strings condition as vmlinux has no
> > .modinfo section but a __ksymtab_strings if symbols are exported.
> > [...] 
> > 	* src/abg-dwarf-reader.cc(is_linux_kernel_binary): consider
> > 	  binaries only having a .modinfo section to be kernel binaries
> > [...]
> >    bool
> >    is_linux_kernel_binary() const
> > -  {return find_section(elf_handle(), "__ksymtab_strings", SHT_PROGBITS);}
> > +  {
> > +    return find_section(elf_handle(), "__ksymtab_strings", SHT_PROGBITS)
> > +	   || find_section(elf_handle(), ".modinfo", SHT_PROGBITS);
> > +  }
> 
> I think the change itself is correct and better than what is there
> now.
> 
> But this is interesting to me because we are discussing an
> eu-elfclassify program addition for elfutils. That utility at the
> moment classifies a --linux-kernel-module as:
> 
> /* Returns true if the file is a linux kernel module (is ET_REL and
>    has the two magic sections .modinfo and .gnu.linkonce.this_module).  */
> static bool
> is_linux_kernel_module (void)
> {
>   return (elf_kind (elf) == ELF_K_ELF
>           && elf_type == ET_REL
>           && has_modinfo
>           && has_gnu_linkonce_this_module);
> }
> 
> Just wondering if you believe that check is too strict?
> 
> The reason to check for both "magic" section names was mainly because
> ".modinfo" seemed a little too generic.
> 
> One difference is that eu-elfclassify doesn't check the section type.
> Maybe it should do that like here, to make sure that it is
> SHT_PROGBITS.
> 
To be honest, I am not entirely sure myself. I was told in IRC that the check
above is sufficient and should be good for kernels of at least 3.18+ (possibly
older kernels as well). I will add a patch on top to restrict abigail a bit
more on this.

> Sorry for hijacking this patch review for something slightly
> different. But I was happy to see something very similar to what I was
> working on in a different, but somewhat related project, and was
> wondering whether it was the correct way to classify something as a
> kernel module.
> 
> Cheers,
> 
> Mark

-- 
Cheers,
Matthias

[PATCH v1] abg-dwarf-reader: detect kernel modules without exports as such

[Matthias Maennich via libabigail]

Kernel modules without exported symbols (no use of EXPORT_SYMBOL*()),
will not have a __ksymtab_strings section. Libabigail will therefore
assume they are usual ELF binaries. That leads to wrong results as
now all ELF symbols are considered part of the ABI. That is obviously
wrong. Instead consider binaries having a .modinfo section to be kernel
binaries. We keep the __ksymtab_strings condition as vmlinux has no
.modinfo section but a __ksymtab_strings if symbols are exported.

One case is still open (and requires maybe some documentation): if a
kernel does not export symbols (no module support), none of the
conditions apply. But, who would be interested in the ABI of a kernel
that does not expose any?

	* src/abg-dwarf-reader.cc(is_linux_kernel_binary): consider
	  binaries only having a .modinfo section to be kernel binaries

Co-developed-by: Alessio Balsini <balsini@android.com>
Signed-off-by: Alessio Balsini <balsini@android.com>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-dwarf-reader.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index 47af7e3516d3..ab3f8e3b2525 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -8350,7 +8350,10 @@ public:
   ///
   bool
   is_linux_kernel_binary() const
-  {return find_section(elf_handle(), "__ksymtab_strings", SHT_PROGBITS);}
+  {
+    return find_section(elf_handle(), "__ksymtab_strings", SHT_PROGBITS)
+	   || find_section(elf_handle(), ".modinfo", SHT_PROGBITS);
+  }
 
   /// Getter of the "show_stats" flag.
   ///
-- 
2.22.0.657.g960e92d24f-goog

Re: [PATCH v1] abg-dwarf-reader: detect kernel modules without exports as such

[Dodji Seketeli]

Hello,

Matthias Maennich <maennich@google.com> a écrit:

[...]

>
> 	* src/abg-dwarf-reader.cc(is_linux_kernel_binary): consider
> 	  binaries only having a .modinfo section to be kernel binaries

This looks good to me and has been pushed to master.

Thanks a lot.

Cheers,

-- 
		Dodji