| Message ID | 08693d17-f2a1-4b5e-a136-81138ca3a58d@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | srcversion hash does not cover all the module's source/dependency files | expand |
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
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
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