[PATCH] linux-yocto_4.14.bb: fix for deterministic srcversion

[PATCH] linux-yocto_4.14.bb: fix for deterministic srcversion

[Juro Bystricky]

"srcversion" field inserted into module modinfo section contains a
sum of the source files which made it. However, this field can
be incorrect. Building the same module can end up having inconsistent
srcversion field eventhough the sources remain the same.

This basically negates the whole purpose of the field srcversion,
and breaks build reproducibility as well.

The problem is fairly easy reproduceable by comparing "srcversion" of
kernel modules built in a workplace of a short directory name with
"srcversion" of the same modules built in a workplace of a fairly long
directory name.

The reason for incorrect srcversion is that some source files can be
simply silently skipped from the checksum calculation due to limited
buffer space for line parsing.

[YOCTO #12544]

Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
---
 .../modpost-srcversion-sometimes-incorrect.patch   | 48 ++++++++++++++++++++++
 meta/recipes-kernel/linux/linux-yocto_4.14.bb      |  4 +-
 2 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch

diff --git a/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch b/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
new file mode 100644
index 0000000..1680293
--- /dev/null
+++ b/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
@@ -0,0 +1,48 @@
+"srcversion" field inserted into module modinfo section contains a
+sum of the source files which made it. However, this field can
+be incorrect. Building the same module can end up having inconsistent
+srcversion field eventhough the sources remain the same.
+This can be reproduced by building modules in a deeply nested directory,
+but other factors contribute as well.
+
+The reason for incorrect srcversion is that some source files can be
+simply silently skipped from the checksum calculation due to limited
+buffer space for line parsing.
+
+This patch addresses two issues:
+
+1. Allocates a larger line buffer (32k vs 4k).
+2. Issues a warning if a line length exceeds the line buffer.
+
+Upstream-Status: Submitted [https://patchwork.kernel.org/patch/10318141/]
+Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
+
+diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
+index 9917f92..955f26e 100644
+--- a/scripts/mod/modpost.c
++++ b/scripts/mod/modpost.c
+@@ -385,9 +385,10 @@ void *grab_file(const char *filename, unsigned long *size)
+   * spaces in the beginning of the line is trimmed away.
+   * Return a pointer to a static buffer.
+   **/
++#define MODPOST_MAX_LINE 32768
+ char *get_next_line(unsigned long *pos, void *file, unsigned long size)
+ {
+-	static char line[4096];
++	static char line[MODPOST_MAX_LINE];
+ 	int skip = 1;
+ 	size_t len = 0;
+ 	signed char *p = (signed char *)file + *pos;
+@@ -402,8 +403,11 @@ char *get_next_line(unsigned long *pos, void *file, unsigned long size)
+ 		if (*p != '\n' && (*pos < size)) {
+ 			len++;
+ 			*s++ = *p++;
+-			if (len > 4095)
++			if (len > (sizeof(line)-1)) {
++				warn(" %s: line exceeds buffer size %zu bytes\n"
++				     , __func__, sizeof(line));
+ 				break; /* Too long, stop */
++			}
+ 		} else {
+ 			/* End of string */
+ 			*s = '\0';
diff --git a/meta/recipes-kernel/linux/linux-yocto_4.14.bb b/meta/recipes-kernel/linux/linux-yocto_4.14.bb
index ba5e356..6c0a88f 100644
--- a/meta/recipes-kernel/linux/linux-yocto_4.14.bb
+++ b/meta/recipes-kernel/linux/linux-yocto_4.14.bb
@@ -22,7 +22,9 @@ SRCREV_machine ?= "edc90f45a716ffe8e16cebaaf3b5db070af0280a"
 SRCREV_meta ?= "5f6c3e32365bffb1993c0c62abf2c5bb8916a57f"
 
 SRC_URI = "git://git.yoctoproject.org/linux-yocto.git;name=machine;branch=${KBRANCH}; \
-           git://git.yoctoproject.org/yocto-kernel-cache;type=kmeta;name=meta;branch=yocto-4.14;destsuffix=${KMETA}"
+           git://git.yoctoproject.org/yocto-kernel-cache;type=kmeta;name=meta;branch=yocto-4.14;destsuffix=${KMETA} \
+           file://modpost-srcversion-sometimes-incorrect.patch \
+           "
 
 LINUX_VERSION ?= "4.14.24"
 
-- 
2.7.4

Re: [PATCH] linux-yocto_4.14.bb: fix for deterministic srcversion

[Bruce Ashfield]

On Fri, Mar 30, 2018 at 4:51 PM, Juro Bystricky
<juro.bystricky@intel.com> wrote:
> "srcversion" field inserted into module modinfo section contains a
> sum of the source files which made it. However, this field can
> be incorrect. Building the same module can end up having inconsistent
> srcversion field eventhough the sources remain the same.
>
> This basically negates the whole purpose of the field srcversion,
> and breaks build reproducibility as well.
>
> The problem is fairly easy reproduceable by comparing "srcversion" of
> kernel modules built in a workplace of a short directory name with
> "srcversion" of the same modules built in a workplace of a fairly long
> directory name.
>
> The reason for incorrect srcversion is that some source files can be
> simply silently skipped from the checksum calculation due to limited
> buffer space for line parsing.
>
> [YOCTO #12544]
>
> Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
> ---
>  .../modpost-srcversion-sometimes-incorrect.patch   | 48 ++++++++++++++++++++++
>  meta/recipes-kernel/linux/linux-yocto_4.14.bb      |  4 +-
>  2 files changed, 51 insertions(+), 1 deletion(-)
>  create mode 100644 meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
>
> diff --git a/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch b/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
> new file mode 100644
> index 0000000..1680293
> --- /dev/null
> +++ b/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
> @@ -0,0 +1,48 @@
> +"srcversion" field inserted into module modinfo section contains a
> +sum of the source files which made it. However, this field can
> +be incorrect. Building the same module can end up having inconsistent
> +srcversion field eventhough the sources remain the same.
> +This can be reproduced by building modules in a deeply nested directory,
> +but other factors contribute as well.
> +
> +The reason for incorrect srcversion is that some source files can be
> +simply silently skipped from the checksum calculation due to limited
> +buffer space for line parsing.
> +
> +This patch addresses two issues:
> +
> +1. Allocates a larger line buffer (32k vs 4k).
> +2. Issues a warning if a line length exceeds the line buffer.
> +
> +Upstream-Status: Submitted [https://patchwork.kernel.org/patch/10318141/]
> +Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
> +
> +diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> +index 9917f92..955f26e 100644
> +--- a/scripts/mod/modpost.c
> ++++ b/scripts/mod/modpost.c
> +@@ -385,9 +385,10 @@ void *grab_file(const char *filename, unsigned long *size)
> +   * spaces in the beginning of the line is trimmed away.
> +   * Return a pointer to a static buffer.
> +   **/
> ++#define MODPOST_MAX_LINE 32768
> + char *get_next_line(unsigned long *pos, void *file, unsigned long size)
> + {
> +-      static char line[4096];
> ++      static char line[MODPOST_MAX_LINE];
> +       int skip = 1;
> +       size_t len = 0;
> +       signed char *p = (signed char *)file + *pos;
> +@@ -402,8 +403,11 @@ char *get_next_line(unsigned long *pos, void *file, unsigned long size)
> +               if (*p != '\n' && (*pos < size)) {
> +                       len++;
> +                       *s++ = *p++;
> +-                      if (len > 4095)
> ++                      if (len > (sizeof(line)-1)) {
> ++                              warn(" %s: line exceeds buffer size %zu bytes\n"
> ++                                   , __func__, sizeof(line));
> +                               break; /* Too long, stop */
> ++                      }
> +               } else {
> +                       /* End of string */
> +                       *s = '\0';
> diff --git a/meta/recipes-kernel/linux/linux-yocto_4.14.bb b/meta/recipes-kernel/linux/linux-yocto_4.14.bb
> index ba5e356..6c0a88f 100644
> --- a/meta/recipes-kernel/linux/linux-yocto_4.14.bb
> +++ b/meta/recipes-kernel/linux/linux-yocto_4.14.bb
> @@ -22,7 +22,9 @@ SRCREV_machine ?= "edc90f45a716ffe8e16cebaaf3b5db070af0280a"
>  SRCREV_meta ?= "5f6c3e32365bffb1993c0c62abf2c5bb8916a57f"
>
>  SRC_URI = "git://git.yoctoproject.org/linux-yocto.git;name=machine;branch=${KBRANCH}; \
> -           git://git.yoctoproject.org/yocto-kernel-cache;type=kmeta;name=meta;branch=yocto-4.14;destsuffix=${KMETA}"
> +           git://git.yoctoproject.org/yocto-kernel-cache;type=kmeta;name=meta;branch=yocto-4.14;destsuffix=${KMETA} \
> +           file://modpost-srcversion-sometimes-incorrect.patch \

I can carry this directly in the impacted linux-yocto trees, but not
like this in
the SRC_URIs.

I'll queue it and test over the weekend and will send SRCREV updates
early next week.

Have you tried it on more than 4.14 ? I'll test it across all the versions I
support, but hearing how it has already been tested is helpful.

Bruce

> +           "
>
>  LINUX_VERSION ?= "4.14.24"
>
> --
> 2.7.4
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"

✗ patchtest: failure for linux-yocto_4.14.bb: fix for deterministic srcversion

[Patchwork]

== Series Details ==

Series: linux-yocto_4.14.bb: fix for deterministic srcversion
Revision: 1
URL   : https://patchwork.openembedded.org/series/11626/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Issue             Series does not apply on top of target branch [test_series_merge_on_head] 
  Suggested fix    Rebase your series on top of targeted branch
  Targeted branch  master (currently at d6c13a5ff4)



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Guidelines:     https://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe

Re: [PATCH] linux-yocto_4.14.bb: fix for deterministic srcversion

[Bystricky, Juro]

I have also tested it with git linux, about v16-rc1 (581e400ff935d34d95811258586128bf11baef15 to be exact) .
I agree there should be a better way than SRC_URI. The patched code is, I believe, pretty ancient so the problem
has been around for a long time.

Re: [PATCH] linux-yocto_4.14.bb: fix for deterministic srcversion

[Khem Raj]

On Fri, Mar 30, 2018 at 1:51 PM, Juro Bystricky
<juro.bystricky@intel.com> wrote:
> "srcversion" field inserted into module modinfo section contains a
> sum of the source files which made it. However, this field can
> be incorrect. Building the same module can end up having inconsistent
> srcversion field eventhough the sources remain the same.
>
> This basically negates the whole purpose of the field srcversion,
> and breaks build reproducibility as well.
>
> The problem is fairly easy reproduceable by comparing "srcversion" of
> kernel modules built in a workplace of a short directory name with
> "srcversion" of the same modules built in a workplace of a fairly long
> directory name.
>
> The reason for incorrect srcversion is that some source files can be
> simply silently skipped from the checksum calculation due to limited
> buffer space for line parsing.
>
> [YOCTO #12544]
>
> Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
> ---
>  .../modpost-srcversion-sometimes-incorrect.patch   | 48 ++++++++++++++++++++++
>  meta/recipes-kernel/linux/linux-yocto_4.14.bb      |  4 +-
>  2 files changed, 51 insertions(+), 1 deletion(-)
>  create mode 100644 meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
>
> diff --git a/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch b/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
> new file mode 100644
> index 0000000..1680293
> --- /dev/null
> +++ b/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
> @@ -0,0 +1,48 @@
> +"srcversion" field inserted into module modinfo section contains a
> +sum of the source files which made it. However, this field can
> +be incorrect. Building the same module can end up having inconsistent
> +srcversion field eventhough the sources remain the same.
> +This can be reproduced by building modules in a deeply nested directory,
> +but other factors contribute as well.
> +
> +The reason for incorrect srcversion is that some source files can be
> +simply silently skipped from the checksum calculation due to limited
> +buffer space for line parsing.
> +
> +This patch addresses two issues:
> +
> +1. Allocates a larger line buffer (32k vs 4k).
> +2. Issues a warning if a line length exceeds the line buffer.
> +
> +Upstream-Status: Submitted [https://patchwork.kernel.org/patch/10318141/]
> +Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
> +
> +diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> +index 9917f92..955f26e 100644
> +--- a/scripts/mod/modpost.c
> ++++ b/scripts/mod/modpost.c
> +@@ -385,9 +385,10 @@ void *grab_file(const char *filename, unsigned long *size)
> +   * spaces in the beginning of the line is trimmed away.
> +   * Return a pointer to a static buffer.
> +   **/
> ++#define MODPOST_MAX_LINE 32768
> + char *get_next_line(unsigned long *pos, void *file, unsigned long size)
> + {
> +-      static char line[4096];
> ++      static char line[MODPOST_MAX_LINE];
> +       int skip = 1;
> +       size_t len = 0;
> +       signed char *p = (signed char *)file + *pos;
> +@@ -402,8 +403,11 @@ char *get_next_line(unsigned long *pos, void *file, unsigned long size)
> +               if (*p != '\n' && (*pos < size)) {
> +                       len++;
> +                       *s++ = *p++;
> +-                      if (len > 4095)
> ++                      if (len > (sizeof(line)-1)) {
> ++                              warn(" %s: line exceeds buffer size %zu bytes\n"
> ++                                   , __func__, sizeof(line));
> +                               break; /* Too long, stop */
> ++                      }

this should be upstreamed into kernel as well.

Re: [PATCH] linux-yocto_4.14.bb: fix for deterministic srcversion

[Bruce Ashfield]

On Fri, Mar 30, 2018 at 7:16 PM, Khem Raj <raj.khem@gmail.com> wrote:
> On Fri, Mar 30, 2018 at 1:51 PM, Juro Bystricky
> <juro.bystricky@intel.com> wrote:
>> "srcversion" field inserted into module modinfo section contains a
>> sum of the source files which made it. However, this field can
>> be incorrect. Building the same module can end up having inconsistent
>> srcversion field eventhough the sources remain the same.
>>
>> This basically negates the whole purpose of the field srcversion,
>> and breaks build reproducibility as well.
>>
>> The problem is fairly easy reproduceable by comparing "srcversion" of
>> kernel modules built in a workplace of a short directory name with
>> "srcversion" of the same modules built in a workplace of a fairly long
>> directory name.
>>
>> The reason for incorrect srcversion is that some source files can be
>> simply silently skipped from the checksum calculation due to limited
>> buffer space for line parsing.
>>
>> [YOCTO #12544]
>>
>> Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
>> ---
>>  .../modpost-srcversion-sometimes-incorrect.patch   | 48 ++++++++++++++++++++++
>>  meta/recipes-kernel/linux/linux-yocto_4.14.bb      |  4 +-
>>  2 files changed, 51 insertions(+), 1 deletion(-)
>>  create mode 100644 meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
>>
>> diff --git a/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch b/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
>> new file mode 100644
>> index 0000000..1680293
>> --- /dev/null
>> +++ b/meta/recipes-kernel/linux/files/modpost-srcversion-sometimes-incorrect.patch
>> @@ -0,0 +1,48 @@
>> +"srcversion" field inserted into module modinfo section contains a
>> +sum of the source files which made it. However, this field can
>> +be incorrect. Building the same module can end up having inconsistent
>> +srcversion field eventhough the sources remain the same.
>> +This can be reproduced by building modules in a deeply nested directory,
>> +but other factors contribute as well.
>> +
>> +The reason for incorrect srcversion is that some source files can be
>> +simply silently skipped from the checksum calculation due to limited
>> +buffer space for line parsing.
>> +
>> +This patch addresses two issues:
>> +
>> +1. Allocates a larger line buffer (32k vs 4k).
>> +2. Issues a warning if a line length exceeds the line buffer.
>> +
>> +Upstream-Status: Submitted [https://patchwork.kernel.org/patch/10318141/]
>> +Signed-off-by: Juro Bystricky <juro.bystricky@intel.com>
>> +
>> +diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>> +index 9917f92..955f26e 100644
>> +--- a/scripts/mod/modpost.c
>> ++++ b/scripts/mod/modpost.c
>> +@@ -385,9 +385,10 @@ void *grab_file(const char *filename, unsigned long *size)
>> +   * spaces in the beginning of the line is trimmed away.
>> +   * Return a pointer to a static buffer.
>> +   **/
>> ++#define MODPOST_MAX_LINE 32768
>> + char *get_next_line(unsigned long *pos, void *file, unsigned long size)
>> + {
>> +-      static char line[4096];
>> ++      static char line[MODPOST_MAX_LINE];
>> +       int skip = 1;
>> +       size_t len = 0;
>> +       signed char *p = (signed char *)file + *pos;
>> +@@ -402,8 +403,11 @@ char *get_next_line(unsigned long *pos, void *file, unsigned long size)
>> +               if (*p != '\n' && (*pos < size)) {
>> +                       len++;
>> +                       *s++ = *p++;
>> +-                      if (len > 4095)
>> ++                      if (len > (sizeof(line)-1)) {
>> ++                              warn(" %s: line exceeds buffer size %zu bytes\n"
>> ++                                   , __func__, sizeof(line));
>> +                               break; /* Too long, stop */
>> ++                      }
>
> this should be upstreamed into kernel as well.

Yup. Juro had this in the actual patch:

Upstream-Status: Submitted [https://patchwork.kernel.org/patch/10318141/]

So we can watch its progress as well.

Bruce

> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"