Re: [PATCH v6 bpf-next 00/10] Harden and extend ELF build ID parsing logic

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Aug 23, 2024 at 4:23 PM Eduard Zingerman <eddyz87@gmail.com> 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@gmail.com>
>
> [...]
>