Fix build ID parsing logic in stable trees

Fix build ID parsing logic in stable trees

[Jiri Olsa]

hi,
sending fix for buildid parsing that affects only stable trees
after merging upstream fix [1].

Upstream then factored out the whole buildid parsing code, so it
does not have the problem.

I'm sending the fix for affected longterm trees 5.15 6.1 6.6 6.11.

thanks,
jirka


[1] 905415ff3ffb ("lib/buildid: harden build ID parsing logic")

[PATCH stable 5.15] lib/buildid: Fix build ID parsing logic

[Jiri Olsa]

The parse_build_id_buf does not account Elf32_Nhdr header size
when getting the build id data pointer and returns wrong build
id data as result.

This is problem only stable trees that merged 8fa2b6817a95 fix,
the upstream build id code was refactored and returns proper
build id.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Fixes: 8fa2b6817a95 ("lib/buildid: harden build ID parsing logic")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 lib/buildid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/buildid.c b/lib/buildid.c
index e41fb0ee405f..cc5da016b235 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -40,7 +40,7 @@ static int parse_build_id_buf(unsigned char *build_id,
 		    name_sz == note_name_sz &&
 		    memcmp(nhdr + 1, note_name, note_name_sz) == 0 &&
 		    desc_sz > 0 && desc_sz <= BUILD_ID_SIZE_MAX) {
-			data = note_start + note_off + ALIGN(note_name_sz, 4);
+			data = note_start + note_off + sizeof(Elf32_Nhdr) + ALIGN(note_name_sz, 4);
 			memcpy(build_id, data, desc_sz);
 			memset(build_id + desc_sz, 0, BUILD_ID_SIZE_MAX - desc_sz);
 			if (size)
-- 
2.47.0

[PATCH stable 6.1 ] lib/buildid: Fix build ID parsing logic

[Jiri Olsa]

The parse_build_id_buf does not account Elf32_Nhdr header size
when getting the build id data pointer and returns wrong build
id data as result.

This is problem only for stable trees that merged 84887f4c1c3a
fix, the upstream build id code was refactored and returns proper
build id.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Fixes: 84887f4c1c3a ("lib/buildid: harden build ID parsing logic")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 lib/buildid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/buildid.c b/lib/buildid.c
index e41fb0ee405f..cc5da016b235 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -40,7 +40,7 @@ static int parse_build_id_buf(unsigned char *build_id,
 		    name_sz == note_name_sz &&
 		    memcmp(nhdr + 1, note_name, note_name_sz) == 0 &&
 		    desc_sz > 0 && desc_sz <= BUILD_ID_SIZE_MAX) {
-			data = note_start + note_off + ALIGN(note_name_sz, 4);
+			data = note_start + note_off + sizeof(Elf32_Nhdr) + ALIGN(note_name_sz, 4);
 			memcpy(build_id, data, desc_sz);
 			memset(build_id + desc_sz, 0, BUILD_ID_SIZE_MAX - desc_sz);
 			if (size)
-- 
2.47.0

[PATCH stable 6.6 ] lib/buildid: Fix build ID parsing logic

[Jiri Olsa]

The parse_build_id_buf does not account Elf32_Nhdr header size
when getting the build id data pointer and returns wrong build
id data as result.

This is problem only for stable trees that merged c83a80d8b84f
fix, the upstream build id code was refactored and returns proper
build id.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Fixes: c83a80d8b84f ("lib/buildid: harden build ID parsing logic")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 lib/buildid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/buildid.c b/lib/buildid.c
index d3bc3d0528d5..9fc46366597e 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -40,7 +40,7 @@ static int parse_build_id_buf(unsigned char *build_id,
 		    name_sz == note_name_sz &&
 		    memcmp(nhdr + 1, note_name, note_name_sz) == 0 &&
 		    desc_sz > 0 && desc_sz <= BUILD_ID_SIZE_MAX) {
-			data = note_start + note_off + ALIGN(note_name_sz, 4);
+			data = note_start + note_off + sizeof(Elf32_Nhdr) + ALIGN(note_name_sz, 4);
 			memcpy(build_id, data, desc_sz);
 			memset(build_id + desc_sz, 0, BUILD_ID_SIZE_MAX - desc_sz);
 			if (size)
-- 
2.47.0

[PATCH stable 6.11] lib/buildid: Fix build ID parsing logic

[Jiri Olsa]

The parse_build_id_buf does not account Elf32_Nhdr header size
when getting the build id data pointer and returns wrong build
id data as result.

This is problem only for stable trees that merged 768d731b8a0d
fix, the upstream build id code was refactored and returns proper
build id.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Fixes: 768d731b8a0d ("lib/buildid: harden build ID parsing logic")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 lib/buildid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/buildid.c b/lib/buildid.c
index 26007cc99a38..aee749f2647d 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -40,7 +40,7 @@ static int parse_build_id_buf(unsigned char *build_id,
 		    name_sz == note_name_sz &&
 		    memcmp(nhdr + 1, note_name, note_name_sz) == 0 &&
 		    desc_sz > 0 && desc_sz <= BUILD_ID_SIZE_MAX) {
-			data = note_start + note_off + ALIGN(note_name_sz, 4);
+			data = note_start + note_off + sizeof(Elf32_Nhdr) + ALIGN(note_name_sz, 4);
 			memcpy(build_id, data, desc_sz);
 			memset(build_id + desc_sz, 0, BUILD_ID_SIZE_MAX - desc_sz);
 			if (size)
-- 
2.47.0

Re: Fix build ID parsing logic in stable trees

[Greg KH]

On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> hi,
> sending fix for buildid parsing that affects only stable trees
> after merging upstream fix [1].
> 
> Upstream then factored out the whole buildid parsing code, so it
> does not have the problem.

Why not just take those patches instead?

thanks,

greg k-h

Re: Fix build ID parsing logic in stable trees

[Jiri Olsa]

On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > hi,
> > sending fix for buildid parsing that affects only stable trees
> > after merging upstream fix [1].
> > 
> > Upstream then factored out the whole buildid parsing code, so it
> > does not have the problem.
> 
> Why not just take those patches instead?

I guess we could, but I thought it's too big for stable

we'd need following 2 changes to fix the issue:
  de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
  60c845b4896b lib/buildid: take into account e_phoff when fetching program headers

and there's also few other follow ups:
  5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
  cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
  ad41251c290d lib/buildid: implement sleepable build_id_parse() API
  45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
  4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search

which I guess are not strictly needed

jirka

Re: Fix build ID parsing logic in stable trees

[Greg KH]

On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > hi,
> > > sending fix for buildid parsing that affects only stable trees
> > > after merging upstream fix [1].
> > > 
> > > Upstream then factored out the whole buildid parsing code, so it
> > > does not have the problem.
> > 
> > Why not just take those patches instead?
> 
> I guess we could, but I thought it's too big for stable
> 
> we'd need following 2 changes to fix the issue:
>   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
>   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> 
> and there's also few other follow ups:
>   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
>   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
>   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
>   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
>   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> 
> which I guess are not strictly needed

Can you verify what exact ones are needed here?  We'll be glad to take
them if you can verify that they work properly.

thanks,

greg k-h

Re: Fix build ID parsing logic in stable trees

[Jiri Olsa]

On Wed, Nov 06, 2024 at 07:12:05AM +0100, Greg KH wrote:
> On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> > On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > > hi,
> > > > sending fix for buildid parsing that affects only stable trees
> > > > after merging upstream fix [1].
> > > > 
> > > > Upstream then factored out the whole buildid parsing code, so it
> > > > does not have the problem.
> > > 
> > > Why not just take those patches instead?
> > 
> > I guess we could, but I thought it's too big for stable
> > 
> > we'd need following 2 changes to fix the issue:
> >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > 
> > and there's also few other follow ups:
> >   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
> >   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
> >   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
> >   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> >   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> > 
> > which I guess are not strictly needed
> 
> Can you verify what exact ones are needed here?  We'll be glad to take
> them if you can verify that they work properly.

ok, will check

jirka

Re: Fix build ID parsing logic in stable trees

[Omar Sandoval]

On Wed, Nov 06, 2024 at 12:57:34PM +0100, Jiri Olsa wrote:
> On Wed, Nov 06, 2024 at 07:12:05AM +0100, Greg KH wrote:
> > On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> > > On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > > > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > > > hi,
> > > > > sending fix for buildid parsing that affects only stable trees
> > > > > after merging upstream fix [1].
> > > > > 
> > > > > Upstream then factored out the whole buildid parsing code, so it
> > > > > does not have the problem.
> > > > 
> > > > Why not just take those patches instead?
> > > 
> > > I guess we could, but I thought it's too big for stable
> > > 
> > > we'd need following 2 changes to fix the issue:
> > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > 
> > > and there's also few other follow ups:
> > >   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
> > >   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
> > >   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
> > >   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> > >   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> > > 
> > > which I guess are not strictly needed
> > 
> > Can you verify what exact ones are needed here?  We'll be glad to take
> > them if you can verify that they work properly.
> 
> ok, will check

Hello,

I noticed that the BUILD-ID field in vmcoreinfo is broken on
stable/longterm kernels and found this thread. Can we please get this
fixed soon?

I tried cherry-picking the patches mentioned above ("lib/buildid: add
single folio-based file reader abstraction" and "lib/buildid: take into
account e_phoff when fetching program headers"), but they don't apply
cleanly before 6.11, and they'd need to be reworked for 5.15, which was
before folios were introduced. Jiri's minimal fix works for me and seems
like a much safer option.

Tested-by: Omar Sandoval <osandov@fb.com>

Thanks,
Omar

Re: Fix build ID parsing logic in stable trees

[Jiri Olsa]

On Thu, Nov 07, 2024 at 12:04:05PM -0800, Omar Sandoval wrote:
> On Wed, Nov 06, 2024 at 12:57:34PM +0100, Jiri Olsa wrote:
> > On Wed, Nov 06, 2024 at 07:12:05AM +0100, Greg KH wrote:
> > > On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> > > > On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > > > > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > > > > hi,
> > > > > > sending fix for buildid parsing that affects only stable trees
> > > > > > after merging upstream fix [1].
> > > > > > 
> > > > > > Upstream then factored out the whole buildid parsing code, so it
> > > > > > does not have the problem.
> > > > > 
> > > > > Why not just take those patches instead?
> > > > 
> > > > I guess we could, but I thought it's too big for stable
> > > > 
> > > > we'd need following 2 changes to fix the issue:
> > > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > > 
> > > > and there's also few other follow ups:
> > > >   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
> > > >   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
> > > >   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
> > > >   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> > > >   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> > > > 
> > > > which I guess are not strictly needed
> > > 
> > > Can you verify what exact ones are needed here?  We'll be glad to take
> > > them if you can verify that they work properly.
> > 
> > ok, will check
> 
> Hello,
> 
> I noticed that the BUILD-ID field in vmcoreinfo is broken on
> stable/longterm kernels and found this thread. Can we please get this
> fixed soon?
> 
> I tried cherry-picking the patches mentioned above ("lib/buildid: add
> single folio-based file reader abstraction" and "lib/buildid: take into
> account e_phoff when fetching program headers"), but they don't apply
> cleanly before 6.11, and they'd need to be reworked for 5.15, which was
> before folios were introduced. Jiri's minimal fix works for me and seems
> like a much safer option.

hi,
thanks for testing

I think for 6.11 we could go with backport of:
  de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
  60c845b4896b lib/buildid: take into account e_phoff when fetching program headers

and with the small fix for the rest

but I still need to figure out why also 60c845b4896b is needed
to fix the issue on 6.11.. hopefully today

jirka

> 
> Tested-by: Omar Sandoval <osandov@fb.com>
> 
> Thanks,
> Omar

Re: Fix build ID parsing logic in stable trees

[Andrii Nakryiko]

On Thu, Nov 7, 2024 at 12:04 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> On Wed, Nov 06, 2024 at 12:57:34PM +0100, Jiri Olsa wrote:
> > On Wed, Nov 06, 2024 at 07:12:05AM +0100, Greg KH wrote:
> > > On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> > > > On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > > > > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > > > > hi,
> > > > > > sending fix for buildid parsing that affects only stable trees
> > > > > > after merging upstream fix [1].
> > > > > >
> > > > > > Upstream then factored out the whole buildid parsing code, so it
> > > > > > does not have the problem.
> > > > >
> > > > > Why not just take those patches instead?
> > > >
> > > > I guess we could, but I thought it's too big for stable
> > > >
> > > > we'd need following 2 changes to fix the issue:
> > > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > >
> > > > and there's also few other follow ups:
> > > >   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
> > > >   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
> > > >   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
> > > >   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> > > >   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> > > >
> > > > which I guess are not strictly needed
> > >
> > > Can you verify what exact ones are needed here?  We'll be glad to take
> > > them if you can verify that they work properly.
> >
> > ok, will check
>
> Hello,
>
> I noticed that the BUILD-ID field in vmcoreinfo is broken on
> stable/longterm kernels and found this thread. Can we please get this
> fixed soon?
>
> I tried cherry-picking the patches mentioned above ("lib/buildid: add
> single folio-based file reader abstraction" and "lib/buildid: take into
> account e_phoff when fetching program headers"), but they don't apply
> cleanly before 6.11, and they'd need to be reworked for 5.15, which was
> before folios were introduced. Jiri's minimal fix works for me and seems
> like a much safer option.

I second that. Custom fix is minimal and keeps the rest of build ID
logic the same without involving all the folio conversions. I'd just
apply that.

>
> Tested-by: Omar Sandoval <osandov@fb.com>
>
> Thanks,
> Omar

Re: Fix build ID parsing logic in stable trees

[Jiri Olsa]

On Fri, Nov 08, 2024 at 09:35:34AM +0100, Jiri Olsa wrote:
> On Thu, Nov 07, 2024 at 12:04:05PM -0800, Omar Sandoval wrote:
> > On Wed, Nov 06, 2024 at 12:57:34PM +0100, Jiri Olsa wrote:
> > > On Wed, Nov 06, 2024 at 07:12:05AM +0100, Greg KH wrote:
> > > > On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> > > > > On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > > > > > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > > > > > hi,
> > > > > > > sending fix for buildid parsing that affects only stable trees
> > > > > > > after merging upstream fix [1].
> > > > > > > 
> > > > > > > Upstream then factored out the whole buildid parsing code, so it
> > > > > > > does not have the problem.
> > > > > > 
> > > > > > Why not just take those patches instead?
> > > > > 
> > > > > I guess we could, but I thought it's too big for stable
> > > > > 
> > > > > we'd need following 2 changes to fix the issue:
> > > > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > > > 
> > > > > and there's also few other follow ups:
> > > > >   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
> > > > >   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
> > > > >   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
> > > > >   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> > > > >   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> > > > > 
> > > > > which I guess are not strictly needed
> > > > 
> > > > Can you verify what exact ones are needed here?  We'll be glad to take
> > > > them if you can verify that they work properly.
> > > 
> > > ok, will check
> > 
> > Hello,
> > 
> > I noticed that the BUILD-ID field in vmcoreinfo is broken on
> > stable/longterm kernels and found this thread. Can we please get this
> > fixed soon?
> > 
> > I tried cherry-picking the patches mentioned above ("lib/buildid: add
> > single folio-based file reader abstraction" and "lib/buildid: take into
> > account e_phoff when fetching program headers"), but they don't apply
> > cleanly before 6.11, and they'd need to be reworked for 5.15, which was
> > before folios were introduced. Jiri's minimal fix works for me and seems
> > like a much safer option.
> 
> hi,
> thanks for testing
> 
> I think for 6.11 we could go with backport of:
>   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
>   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> 
> and with the small fix for the rest
> 
> but I still need to figure out why also 60c845b4896b is needed
> to fix the issue on 6.11.. hopefully today

ok, so the fix the issue in 6.11 with upstream backports we'd need both:

  1) de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
  2) 60c845b4896b lib/buildid: take into account e_phoff when fetching program headers

2) is needed because 1) seems to omit ehdr->e_phoff addition (patch below)
which is added back in 2)

IMO 6.11 is close to upstream and by taking above upstream fixes it will be
easier to backport other possible fixes in the future, for other trees I'd
take the original one line fix I posted

jirka


---
diff --git a/lib/buildid.c b/lib/buildid.c
index bfe00b66b1e8..19d9a0f6ce99 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -234,7 +234,7 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
 		return -EINVAL;
 
 	for (i = 0; i < phnum; ++i) {
-		phdr = freader_fetch(r, i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
+		phdr = freader_fetch(r, sizeof(Elf32_Ehdr) + i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
 		if (!phdr)
 			return r->err;
 
@@ -272,7 +272,7 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
 		return -EINVAL;
 
 	for (i = 0; i < phnum; ++i) {
-		phdr = freader_fetch(r, i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
+		phdr = freader_fetch(r, sizeof(Elf64_Ehdr) + i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
 		if (!phdr)
 			return r->err;
 

Re: Fix build ID parsing logic in stable trees

[Andrii Nakryiko]

On Fri, Nov 8, 2024 at 3:26 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Nov 08, 2024 at 09:35:34AM +0100, Jiri Olsa wrote:
> > On Thu, Nov 07, 2024 at 12:04:05PM -0800, Omar Sandoval wrote:
> > > On Wed, Nov 06, 2024 at 12:57:34PM +0100, Jiri Olsa wrote:
> > > > On Wed, Nov 06, 2024 at 07:12:05AM +0100, Greg KH wrote:
> > > > > On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> > > > > > On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > > > > > > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > > > > > > hi,
> > > > > > > > sending fix for buildid parsing that affects only stable trees
> > > > > > > > after merging upstream fix [1].
> > > > > > > >
> > > > > > > > Upstream then factored out the whole buildid parsing code, so it
> > > > > > > > does not have the problem.
> > > > > > >
> > > > > > > Why not just take those patches instead?
> > > > > >
> > > > > > I guess we could, but I thought it's too big for stable
> > > > > >
> > > > > > we'd need following 2 changes to fix the issue:
> > > > > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > > > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > > > >
> > > > > > and there's also few other follow ups:
> > > > > >   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
> > > > > >   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
> > > > > >   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
> > > > > >   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> > > > > >   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> > > > > >
> > > > > > which I guess are not strictly needed
> > > > >
> > > > > Can you verify what exact ones are needed here?  We'll be glad to take
> > > > > them if you can verify that they work properly.
> > > >
> > > > ok, will check
> > >
> > > Hello,
> > >
> > > I noticed that the BUILD-ID field in vmcoreinfo is broken on
> > > stable/longterm kernels and found this thread. Can we please get this
> > > fixed soon?
> > >
> > > I tried cherry-picking the patches mentioned above ("lib/buildid: add
> > > single folio-based file reader abstraction" and "lib/buildid: take into
> > > account e_phoff when fetching program headers"), but they don't apply
> > > cleanly before 6.11, and they'd need to be reworked for 5.15, which was
> > > before folios were introduced. Jiri's minimal fix works for me and seems
> > > like a much safer option.
> >
> > hi,
> > thanks for testing
> >
> > I think for 6.11 we could go with backport of:
> >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> >
> > and with the small fix for the rest
> >
> > but I still need to figure out why also 60c845b4896b is needed
> > to fix the issue on 6.11.. hopefully today
>
> ok, so the fix the issue in 6.11 with upstream backports we'd need both:
>
>   1) de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
>   2) 60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
>
> 2) is needed because 1) seems to omit ehdr->e_phoff addition (patch below)
> which is added back in 2)
>
> IMO 6.11 is close to upstream and by taking above upstream fixes it will be
> easier to backport other possible fixes in the future, for other trees I'd
> take the original one line fix I posted

I still maintain that very minimal is the way to go instead of risking
bringing new potential regressions by partially backporting folio
rework patchset.

Jiri, there is no point in risking this, best to fix this quickly and
minimally. If we ever need to backport further fixes, *then* we can
think about folio-based implementation backport.

>
> jirka
>
>
> ---
> diff --git a/lib/buildid.c b/lib/buildid.c
> index bfe00b66b1e8..19d9a0f6ce99 100644
> --- a/lib/buildid.c
> +++ b/lib/buildid.c
> @@ -234,7 +234,7 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
>                 return -EINVAL;
>
>         for (i = 0; i < phnum; ++i) {
> -               phdr = freader_fetch(r, i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
> +               phdr = freader_fetch(r, sizeof(Elf32_Ehdr) + i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
>                 if (!phdr)
>                         return r->err;
>
> @@ -272,7 +272,7 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
>                 return -EINVAL;
>
>         for (i = 0; i < phnum; ++i) {
> -               phdr = freader_fetch(r, i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
> +               phdr = freader_fetch(r, sizeof(Elf64_Ehdr) + i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
>                 if (!phdr)
>                         return r->err;
>

Re: Fix build ID parsing logic in stable trees

[Jiri Olsa]

On Wed, Nov 13, 2024 at 12:07:39PM -0800, Andrii Nakryiko wrote:
> On Fri, Nov 8, 2024 at 3:26 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Fri, Nov 08, 2024 at 09:35:34AM +0100, Jiri Olsa wrote:
> > > On Thu, Nov 07, 2024 at 12:04:05PM -0800, Omar Sandoval wrote:
> > > > On Wed, Nov 06, 2024 at 12:57:34PM +0100, Jiri Olsa wrote:
> > > > > On Wed, Nov 06, 2024 at 07:12:05AM +0100, Greg KH wrote:
> > > > > > On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> > > > > > > On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > > > > > > > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > > > > > > > hi,
> > > > > > > > > sending fix for buildid parsing that affects only stable trees
> > > > > > > > > after merging upstream fix [1].
> > > > > > > > >
> > > > > > > > > Upstream then factored out the whole buildid parsing code, so it
> > > > > > > > > does not have the problem.
> > > > > > > >
> > > > > > > > Why not just take those patches instead?
> > > > > > >
> > > > > > > I guess we could, but I thought it's too big for stable
> > > > > > >
> > > > > > > we'd need following 2 changes to fix the issue:
> > > > > > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > > > > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > > > > >
> > > > > > > and there's also few other follow ups:
> > > > > > >   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
> > > > > > >   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
> > > > > > >   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
> > > > > > >   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> > > > > > >   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> > > > > > >
> > > > > > > which I guess are not strictly needed
> > > > > >
> > > > > > Can you verify what exact ones are needed here?  We'll be glad to take
> > > > > > them if you can verify that they work properly.
> > > > >
> > > > > ok, will check
> > > >
> > > > Hello,
> > > >
> > > > I noticed that the BUILD-ID field in vmcoreinfo is broken on
> > > > stable/longterm kernels and found this thread. Can we please get this
> > > > fixed soon?
> > > >
> > > > I tried cherry-picking the patches mentioned above ("lib/buildid: add
> > > > single folio-based file reader abstraction" and "lib/buildid: take into
> > > > account e_phoff when fetching program headers"), but they don't apply
> > > > cleanly before 6.11, and they'd need to be reworked for 5.15, which was
> > > > before folios were introduced. Jiri's minimal fix works for me and seems
> > > > like a much safer option.
> > >
> > > hi,
> > > thanks for testing
> > >
> > > I think for 6.11 we could go with backport of:
> > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > >
> > > and with the small fix for the rest
> > >
> > > but I still need to figure out why also 60c845b4896b is needed
> > > to fix the issue on 6.11.. hopefully today
> >
> > ok, so the fix the issue in 6.11 with upstream backports we'd need both:
> >
> >   1) de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> >   2) 60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> >
> > 2) is needed because 1) seems to omit ehdr->e_phoff addition (patch below)
> > which is added back in 2)
> >
> > IMO 6.11 is close to upstream and by taking above upstream fixes it will be
> > easier to backport other possible fixes in the future, for other trees I'd
> > take the original one line fix I posted
> 
> I still maintain that very minimal is the way to go instead of risking
> bringing new potential regressions by partially backporting folio
> rework patchset.
> 
> Jiri, there is no point in risking this, best to fix this quickly and
> minimally. If we ever need to backport further fixes, *then* we can
> think about folio-based implementation backport.

ok, make sense, the original plan works for me as well

jirka

> 
> >
> > jirka
> >
> >
> > ---
> > diff --git a/lib/buildid.c b/lib/buildid.c
> > index bfe00b66b1e8..19d9a0f6ce99 100644
> > --- a/lib/buildid.c
> > +++ b/lib/buildid.c
> > @@ -234,7 +234,7 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
> >                 return -EINVAL;
> >
> >         for (i = 0; i < phnum; ++i) {
> > -               phdr = freader_fetch(r, i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
> > +               phdr = freader_fetch(r, sizeof(Elf32_Ehdr) + i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
> >                 if (!phdr)
> >                         return r->err;
> >
> > @@ -272,7 +272,7 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
> >                 return -EINVAL;
> >
> >         for (i = 0; i < phnum; ++i) {
> > -               phdr = freader_fetch(r, i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
> > +               phdr = freader_fetch(r, sizeof(Elf64_Ehdr) + i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
> >                 if (!phdr)
> >                         return r->err;
> >

Re: Fix build ID parsing logic in stable trees

[Omar Sandoval]

On Wed, Nov 13, 2024 at 10:12:39PM +0100, Jiri Olsa wrote:
> On Wed, Nov 13, 2024 at 12:07:39PM -0800, Andrii Nakryiko wrote:
> > On Fri, Nov 8, 2024 at 3:26 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Fri, Nov 08, 2024 at 09:35:34AM +0100, Jiri Olsa wrote:
> > > > On Thu, Nov 07, 2024 at 12:04:05PM -0800, Omar Sandoval wrote:
> > > > > On Wed, Nov 06, 2024 at 12:57:34PM +0100, Jiri Olsa wrote:
> > > > > > On Wed, Nov 06, 2024 at 07:12:05AM +0100, Greg KH wrote:
> > > > > > > On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> > > > > > > > On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > > > > > > > > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > > > > > > > > hi,
> > > > > > > > > > sending fix for buildid parsing that affects only stable trees
> > > > > > > > > > after merging upstream fix [1].
> > > > > > > > > >
> > > > > > > > > > Upstream then factored out the whole buildid parsing code, so it
> > > > > > > > > > does not have the problem.
> > > > > > > > >
> > > > > > > > > Why not just take those patches instead?
> > > > > > > >
> > > > > > > > I guess we could, but I thought it's too big for stable
> > > > > > > >
> > > > > > > > we'd need following 2 changes to fix the issue:
> > > > > > > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > > > > > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > > > > > >
> > > > > > > > and there's also few other follow ups:
> > > > > > > >   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
> > > > > > > >   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
> > > > > > > >   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
> > > > > > > >   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> > > > > > > >   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> > > > > > > >
> > > > > > > > which I guess are not strictly needed
> > > > > > >
> > > > > > > Can you verify what exact ones are needed here?  We'll be glad to take
> > > > > > > them if you can verify that they work properly.
> > > > > >
> > > > > > ok, will check
> > > > >
> > > > > Hello,
> > > > >
> > > > > I noticed that the BUILD-ID field in vmcoreinfo is broken on
> > > > > stable/longterm kernels and found this thread. Can we please get this
> > > > > fixed soon?
> > > > >
> > > > > I tried cherry-picking the patches mentioned above ("lib/buildid: add
> > > > > single folio-based file reader abstraction" and "lib/buildid: take into
> > > > > account e_phoff when fetching program headers"), but they don't apply
> > > > > cleanly before 6.11, and they'd need to be reworked for 5.15, which was
> > > > > before folios were introduced. Jiri's minimal fix works for me and seems
> > > > > like a much safer option.
> > > >
> > > > hi,
> > > > thanks for testing
> > > >
> > > > I think for 6.11 we could go with backport of:
> > > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > >
> > > > and with the small fix for the rest
> > > >
> > > > but I still need to figure out why also 60c845b4896b is needed
> > > > to fix the issue on 6.11.. hopefully today
> > >
> > > ok, so the fix the issue in 6.11 with upstream backports we'd need both:
> > >
> > >   1) de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > >   2) 60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > >
> > > 2) is needed because 1) seems to omit ehdr->e_phoff addition (patch below)
> > > which is added back in 2)
> > >
> > > IMO 6.11 is close to upstream and by taking above upstream fixes it will be
> > > easier to backport other possible fixes in the future, for other trees I'd
> > > take the original one line fix I posted
> > 
> > I still maintain that very minimal is the way to go instead of risking
> > bringing new potential regressions by partially backporting folio
> > rework patchset.
> > 
> > Jiri, there is no point in risking this, best to fix this quickly and
> > minimally. If we ever need to backport further fixes, *then* we can
> > think about folio-based implementation backport.
> 
> ok, make sense, the original plan works for me as well
> 
> jirka

Greg, could you please queue up Jiri's one line fixes for 5.15, 6.1,
6.6, and 6.11?

Thanks,
Omar

Re: Fix build ID parsing logic in stable trees

[Greg KH]

On Fri, Nov 15, 2024 at 10:19:25AM -0800, Omar Sandoval wrote:
> On Wed, Nov 13, 2024 at 10:12:39PM +0100, Jiri Olsa wrote:
> > On Wed, Nov 13, 2024 at 12:07:39PM -0800, Andrii Nakryiko wrote:
> > > On Fri, Nov 8, 2024 at 3:26 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >
> > > > On Fri, Nov 08, 2024 at 09:35:34AM +0100, Jiri Olsa wrote:
> > > > > On Thu, Nov 07, 2024 at 12:04:05PM -0800, Omar Sandoval wrote:
> > > > > > On Wed, Nov 06, 2024 at 12:57:34PM +0100, Jiri Olsa wrote:
> > > > > > > On Wed, Nov 06, 2024 at 07:12:05AM +0100, Greg KH wrote:
> > > > > > > > On Tue, Nov 05, 2024 at 10:15:04AM +0100, Jiri Olsa wrote:
> > > > > > > > > On Tue, Nov 05, 2024 at 07:54:48AM +0100, Greg KH wrote:
> > > > > > > > > > On Mon, Nov 04, 2024 at 06:52:52PM +0100, Jiri Olsa wrote:
> > > > > > > > > > > hi,
> > > > > > > > > > > sending fix for buildid parsing that affects only stable trees
> > > > > > > > > > > after merging upstream fix [1].
> > > > > > > > > > >
> > > > > > > > > > > Upstream then factored out the whole buildid parsing code, so it
> > > > > > > > > > > does not have the problem.
> > > > > > > > > >
> > > > > > > > > > Why not just take those patches instead?
> > > > > > > > >
> > > > > > > > > I guess we could, but I thought it's too big for stable
> > > > > > > > >
> > > > > > > > > we'd need following 2 changes to fix the issue:
> > > > > > > > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > > > > > > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > > > > > > >
> > > > > > > > > and there's also few other follow ups:
> > > > > > > > >   5ac9b4e935df lib/buildid: Handle memfd_secret() files in build_id_parse()
> > > > > > > > >   cdbb44f9a74f lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
> > > > > > > > >   ad41251c290d lib/buildid: implement sleepable build_id_parse() API
> > > > > > > > >   45b8fc309654 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> > > > > > > > >   4e9d360c4cdf lib/buildid: remove single-page limit for PHDR search
> > > > > > > > >
> > > > > > > > > which I guess are not strictly needed
> > > > > > > >
> > > > > > > > Can you verify what exact ones are needed here?  We'll be glad to take
> > > > > > > > them if you can verify that they work properly.
> > > > > > >
> > > > > > > ok, will check
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > I noticed that the BUILD-ID field in vmcoreinfo is broken on
> > > > > > stable/longterm kernels and found this thread. Can we please get this
> > > > > > fixed soon?
> > > > > >
> > > > > > I tried cherry-picking the patches mentioned above ("lib/buildid: add
> > > > > > single folio-based file reader abstraction" and "lib/buildid: take into
> > > > > > account e_phoff when fetching program headers"), but they don't apply
> > > > > > cleanly before 6.11, and they'd need to be reworked for 5.15, which was
> > > > > > before folios were introduced. Jiri's minimal fix works for me and seems
> > > > > > like a much safer option.
> > > > >
> > > > > hi,
> > > > > thanks for testing
> > > > >
> > > > > I think for 6.11 we could go with backport of:
> > > > >   de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > > >   60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > > >
> > > > > and with the small fix for the rest
> > > > >
> > > > > but I still need to figure out why also 60c845b4896b is needed
> > > > > to fix the issue on 6.11.. hopefully today
> > > >
> > > > ok, so the fix the issue in 6.11 with upstream backports we'd need both:
> > > >
> > > >   1) de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > >   2) 60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > >
> > > > 2) is needed because 1) seems to omit ehdr->e_phoff addition (patch below)
> > > > which is added back in 2)
> > > >
> > > > IMO 6.11 is close to upstream and by taking above upstream fixes it will be
> > > > easier to backport other possible fixes in the future, for other trees I'd
> > > > take the original one line fix I posted
> > > 
> > > I still maintain that very minimal is the way to go instead of risking
> > > bringing new potential regressions by partially backporting folio
> > > rework patchset.
> > > 
> > > Jiri, there is no point in risking this, best to fix this quickly and
> > > minimally. If we ever need to backport further fixes, *then* we can
> > > think about folio-based implementation backport.
> > 
> > ok, make sense, the original plan works for me as well
> > 
> > jirka
> 
> Greg, could you please queue up Jiri's one line fixes for 5.15, 6.1,
> 6.6, and 6.11?

Ok, will do, but hopefully you all will help out if there's any problems
with the change going forward...

greg k-h

Re: Fix build ID parsing logic in stable trees

[Jiri Olsa]

On Tue, Nov 19, 2024 at 12:58:21PM +0100, Greg KH wrote:

SNIP

> > > > >
> > > > > ok, so the fix the issue in 6.11 with upstream backports we'd need both:
> > > > >
> > > > >   1) de3ec364c3c3 lib/buildid: add single folio-based file reader abstraction
> > > > >   2) 60c845b4896b lib/buildid: take into account e_phoff when fetching program headers
> > > > >
> > > > > 2) is needed because 1) seems to omit ehdr->e_phoff addition (patch below)
> > > > > which is added back in 2)
> > > > >
> > > > > IMO 6.11 is close to upstream and by taking above upstream fixes it will be
> > > > > easier to backport other possible fixes in the future, for other trees I'd
> > > > > take the original one line fix I posted
> > > > 
> > > > I still maintain that very minimal is the way to go instead of risking
> > > > bringing new potential regressions by partially backporting folio
> > > > rework patchset.
> > > > 
> > > > Jiri, there is no point in risking this, best to fix this quickly and
> > > > minimally. If we ever need to backport further fixes, *then* we can
> > > > think about folio-based implementation backport.
> > > 
> > > ok, make sense, the original plan works for me as well
> > > 
> > > jirka
> > 
> > Greg, could you please queue up Jiri's one line fixes for 5.15, 6.1,
> > 6.6, and 6.11?
> 
> Ok, will do, but hopefully you all will help out if there's any problems
> with the change going forward...

no worries, will help with that

thanks,
jirka