srcversion hash does not cover all the module's source/dependency files

srcversion hash does not cover all the module's source/dependency files

[Radek Krejci]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

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.

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.

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?

Regards,
Radek Krejci


[1] - 
https://lore.kernel.org/linux-kbuild/20200601055731.3006266-26-masahiroy@kernel.org/
[2] - 
https://lore.kernel.org/linux-kbuild/CAN19L9G-mFN-MTmw0FS3ZX4d1MjeoL2U+s-Fk7Qw9UYWn5Q1YA@mail.gmail.com/ 

[-- Attachment #2: 0001-modpost-trim-leading-spaces-when-processing-source-f.patch --]
[-- Type: text/x-patch, Size: 1123 bytes --]

From 8e14b8fdb71385ab53af48fc56d9cd9e323e1265 Mon Sep 17 00:00:00 2001
From: Radek Krejci <radek.krejci@oracle.com>
Date: Wed, 14 Feb 2024 10:14:07 +0100
Subject: [PATCH] modpost: trim leading spaces when processing source files
 list

get_line() does not trim the leading spaces, but the
parse_source_files() expects to get lines with source files paths where
the first space occurs after the file path.

Signed-off-by: Radek Krejci <radek.krejci@oracle.com>
---
 scripts/mod/sumversion.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/mod/sumversion.c b/scripts/mod/sumversion.c
index 31066bfdba04..dc4878502276 100644
--- a/scripts/mod/sumversion.c
+++ b/scripts/mod/sumversion.c
@@ -326,7 +326,12 @@ static int parse_source_files(const char *objfile, struct md4_ctx *md)
 
 	/* Sum all files in the same dir or subdirs. */
 	while ((line = get_line(&pos))) {
-		char* p = line;
+		char* p;
+
+		/* trim the leading spaces away */
+		while (isspace(*line))
+			line++;
+		p = line;
 
 		if (strncmp(line, "source_", sizeof("source_")-1) == 0) {
 			p = strrchr(line, ' ');
-- 
2.43.0

Re: srcversion hash does not cover all the module's source/dependency files

[Masahiro Yamada]

On Wed, Feb 14, 2024 at 6:43 PM Radek Krejci <radek.krejci@oracle.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)






Thanks.




> Regards,
> Radek Krejci
>
>
> [1] -
> https://lore.kernel.org/linux-kbuild/20200601055731.3006266-26-masahiroy@kernel.org/
> [2] -
> https://lore.kernel.org/linux-kbuild/CAN19L9G-mFN-MTmw0FS3ZX4d1MjeoL2U+s-Fk7Qw9UYWn5Q1YA@mail.gmail.com/



--
Best Regards
Masahiro Yamada

Re: [External] : Re: srcversion hash does not cover all the module's source/dependency files

[Radek Krejci]

Dne 14. 02. 24 v 14:36 Masahiro Yamada napsal(a):
> On Wed, Feb 14, 2024 at 6:43 PM Radek Krejci <radek.krejci@oracle.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