Re: [PATCH v2] modpost: move the namespace field in Module.symvers last

[Jessica Yu]

+++ John Stultz [30/03/20 23:25 -0700]:
> On Mon, Mar 30, 2020 at 10:49 PM John Stultz <john.stultz@xxxxxxxxxx> wrote:
> > On Wed, Mar 11, 2020 at 10:03 AM Jessica Yu <jeyu@xxxxxxxxxx> wrote:
> > >
> > > In order to preserve backwards compatability with kmod tools, we have to
> > > move the namespace field in Module.symvers last, as the depmod -e -E
> > > option looks at the first three fields in Module.symvers to check symbol
> > > versions (and it's expected they stay in the original order of crc,
> > > symbol, module).
> > >
> > > In addition, update an ancient comment above read_dump() in modpost that
> > > suggested that the export type field in Module.symvers was optional. I
> > > suspect that there were historical reasons behind that comment that are
> > > no longer accurate. We have been unconditionally printing the export
> > > type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.
> > >
> > > Fix up read_dump() to treat each field as non-optional. I suspect the
> > > original read_dump() code treated the export field as optional in order
> > > to support pre <= 2.6.18 Module.symvers (which did not have the export
> > > type field). Note that although symbol namespaces are optional, the
> > > field will not be omitted from Module.symvers if a symbol does not have
> > > a namespace. In this case, the field will simply be empty and the next
> > > delimiter or end of line will follow.
> > >
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
> > > Tested-by: Matthias Maennich <maennich@xxxxxxxxxx>
> > > Reviewed-by: Matthias Maennich <maennich@xxxxxxxxxx>
> > > Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > Signed-off-by: Jessica Yu <jeyu@xxxxxxxxxx>
> > > ---
> > > v2:
> > >
> > >   - Explain the changes to read_dump() and the comment (and provide
> > >     historical context) in the commit message. (Lucas De Marchi)
> > >
> > >  Documentation/kbuild/modules.rst |  4 ++--
> > >  scripts/export_report.pl         |  2 +-
> > >  scripts/mod/modpost.c            | 24 ++++++++++++------------
> > >  3 files changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
> > > index 69fa48ee93d6..e0b45a257f21 100644
> > > --- a/Documentation/kbuild/modules.rst
> > > +++ b/Documentation/kbuild/modules.rst
> > > @@ -470,9 +470,9 @@ build.
> > >
> > >         The syntax of the Module.symvers file is::
> > >
> > > -       <CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
> > > +       <CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
> > >
> > > -       0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
> > > +       0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
> > >
> > >         The fields are separated by tabs and values may be empty (e.g.
> > >         if no namespace is defined for an exported symbol).
> >
> > Despite the documentation here claiming the namespace field can be
> > empty, I'm seeing some trouble with this patch when building external
> > modules:
> >   FATAL: parse error in symbol dump file
> >
> > I've confirmed reverting it make things work again, but its not clear
> > to me quite yet why.
> >
> > The only difference I can find is that the Module.symvers in the
> > external module project doesn't seem to have a tab at the end of each
> > line (where as Module.symvers for the kernel - which doesn't seem to
> > have any namespace names - does).
> >
> > Is there something I need to tweak on the external Kbuild to get
> > Module.symvers to be generated properly (with the empty tab at the
> > end) for this new change?
> > Or does the parser need to be a bit more flexible?
>
> One extra clue on this: I noticed the external module Makefile had
> KBUILD_EXTRA_SYMBOLS="$(EXTRA_SYMBOLS)"  in the $(MAKE) string, where
> EXTRA_SYMBOLS pointed to some files that no longer exist.  I removed
> the KBUILD_EXTRA_SYMBOLS= argument, and magically, the generated
> Module.symvers now had tabs at the end of each line.
>
> I wonder if there some path in the KBUILD_EXTRA_SYMBOLS= handling that
> isn't generating the output in the same way?

I'm afraid we're going to need some specifics on reproducing this
issue. Could you provide a reproducer or steps on how to reproduce? I
have not been able to trigger this problem even with
KBUILD_EXTRA_SYMBOLS pointing to an invalid path (I also tested with
valid paths).

I tested with a skeleton external module that exports two functions,
one with a namespace and one without. I built this module against the
latest v5.6 kernel. The generated Module.symvers was correct - the
namespaced function had the namespace at the end and the other
function without a namespace had a tab at the end.

I also tested with two external modules, one with a symbol dependency
on the other, so KBUILD_EXTRA_SYMBOLS usage is required here. The
generated Module.symvers was also correct here.

The only advice I can offer at this time is that all external modules
must be built against the new kernel to generate a correctly formated
Module.symvers file. If you have any KBUILD_EXTRA_SYMBOLS pointing to
an outdated Module.symvers file for example, you will see the "FATAL:
parse error in symbol dump file" error.

Hope this helps,

Jessica