Date: Wed, 14 Feb 2024 14:47:45 +0100
From: Radek Krejci <radek.krejci@...cle.com>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [External] : Re: srcversion hash does not cover all the module's
 source/dependency files

Dne 14. 02. 24 v 14:36 Masahiro Yamada napsal(a):
> On Wed, Feb 14, 2024 at 6:43 PM Radek Krejci <radek.krejci@...cle.com> wrote:
>> Hi,
>> I've found a bug in modpost - when it gets the list of source files to
>> generate srcversion hash, it skips all the source/dependency files
>> except for the first one.
>>
>> There is patch [1] in v5.8rc1 replacing get_next_line() by get_line() in
>> parse_source_file() function. Besides other things, the difference
>> between those 2 functions is that get_next_line() trims the leading
>> spaces of the line being returned. The issue is, that the source (deps_
>> located at the same directory) file names in the list, being processed
>> in parse_source_file(), are indented. So, when the code gets to
>> "Terminate line at first space, to get rid of final ' \'", it
>> effectively hides the source file name from further processing, since
>> the first space is at the beginning of the line.
>>
>> I checked this behavior with modpost from v5.4 and v5.14 (and confirmed
>> with the current master in git). In my case, my kernel module had just 2
>> source files - mymodule.c and mymodule.h (both located at the same
>> directory). With modpost from v5.4, the code change in any of the files
>> was reflected in srcversion hash. But with modpost from v5.14 (and
>> master) there is no hash change when the code change appears in the
>> header file, which is listed at the end of the deps_ list. I believe
>> this is quite simple to reproduce with any module, but if needed, I can
>> prepare some example code to reproduce the issue.
>
> Thanks. You are right.
>
>
>> I noticed this [2] email thread in the list. It mentions a similar
>> issue. However, since it happened a half year before the change [1] was
>> introduced and because I was unable to find any further details,
>> including the promised patch, I believe that these 2 things are unrelated.
>
> Correct. They are different things.
>
> Parsing the headers located in the same directory seems
> to be a design.
>
>
>
>> The enclosed patch worked for me, but there might be some other
>> consequences that I've missed, so feel free to modify it on your own or
>> let me know.
>>
>> Is there anything else I can do to help fixing this issue?
>
> I think the attached patch is correct.
> I will pick it up later.
>
>
>
> One question is, is this feature still used?
>
> I assume the answer is yes because you noticed this bug.
> (but you are the first/only person who noticed it
> in the past 3 years)
>

I was also surprised that no one noticed so far. Maybe my use case is 
somehow specific - I wasn't able to use the module's version (not 
maintained by the module authors), so the srcversion was the way how to 
check that the loaded module is the one I expect. And surprisingly, with 
the same source files, the expected hash changed between kernels v5.4 
and v5.14.

Thanks,
Radek

>
>
>
>
> Thanks.
>
>
>
>
>> Regards,
>> Radek Krejci
>>
>>
>> [1] -
>> https://urldefense.com/v3/__https://lore.kernel.org/linux-kbuild/20200601055731.3006266-26-masahiroy@kernel.org/__;!!ACWV5N9M2RV99hQ!LR5KEN-WVLtWxWk3vtaqPD9DwmpPPIwMFle61qi8b83V1SGdNbFydDDQ_DGJ_bUmjM6Z6NgkH4NuxliZewmIkA$
>> [2] -
>> https://urldefense.com/v3/__https://lore.kernel.org/linux-kbuild/CAN19L9G-mFN-MTmw0FS3ZX4d1MjeoL2U*s-Fk7Qw9UYWn5Q1YA@mail.gmail.com/__;Kw!!ACWV5N9M2RV99hQ!LR5KEN-WVLtWxWk3vtaqPD9DwmpPPIwMFle61qi8b83V1SGdNbFydDDQ_DGJ_bUmjM6Z6NgkH4Nuxlga4dF0SA$
>
>
> --
> Best Regards
> Masahiro Yamada

Powered by blists - more mailing lists