On Fri, Aug 23, 2024 at 4:23 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2024-08-14 at 11:54 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > Andrii Nakryiko (10):
> > lib/buildid: harden build ID parsing logic
> > lib/buildid: add single folio-based file reader abstraction
> > lib/buildid: take into account e_phoff when fetching program headers
> > lib/buildid: remove single-page limit for PHDR search
> > lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> > lib/buildid: implement sleepable build_id_parse() API
> > lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
>
> Never worked with lib/buildid before, so not sure how valuable my input is.
> Anyways:
> - I compared the resulting parser with ELF specification and available
> documentation for buildid, all seems correct.
> (with a small caveat that ELF defines Elf{32,64}_Ehdr->e_ehsize field
> to encode actual size of the elf header, and e_phentsize
> to encode actual size of the program header.
> Parser uses sizeof(Elf{32,64}_{Ehdr,Phdr}) instead,
> and this is how it was before, so probably does not matter).
>
> - The `freader` abstraction nicely hides away difference between
> sleepable and non-sleepable contexts.
> (with a caveat, that freader_get_folio() uses read_cache_folio()
> which is documented as expecting mapping->invalidate_lock to be held.
> I assume that this is true for vma's passed to build_id_parse(), right?)
No, I don't think it's automatically true. So good catch, I think I'll
need to add filemap_invalidate_lock_shared() +
filemap_invalidate_unlock_shared() around read_cache_folio().
I'll give Matthew and Andrew a chance to reply to Alexei, and will
post a new revision tomorrow. Thanks for a thorough review!
>
> For what it's worth, full patch-set looks good to me.
>
> Reviewed-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> [...]
>
