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

[Jessica Yu <jeyu@kernel.org>]

+++ John Stultz [30/03/20 23:25 -0700]:
>On Mon, Mar 30, 2020 at 10:49 PM John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, Mar 11, 2020 at 10:03 AM Jessica Yu <jeyu@kernel.org> 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@vger.kernel.org
>> > Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>> > Tested-by: Matthias Maennich <maennich@google.com>
>> > Reviewed-by: Matthias Maennich <maennich@google.com>
>> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> > Signed-off-by: Jessica Yu <jeyu@kernel.org>
>> > ---
>> > 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