Re: [PATCH v3 bpf-next 06/10] lib/buildid: implement sleepable build_id_parse() API

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

On Tue, Jul 30, 2024 at 1:39 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Extend freader with a flag specifying whether it's OK to cause page
> fault to fetch file data that is not already physically present in
> memory. With this, it's now easy to wait for data if the caller is
> running in sleepable (faultable) context.
>
> We utilize read_cache_folio() to bring the desired file page into page
> cache, after which the rest of the logic works just the same at page level.
>
> Suggested-by: Omar Sandoval <osandov@fb.com>
> Cc: Shakeel Butt <shakeel.butt@linux.dev>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  lib/buildid.c | 50 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/lib/buildid.c b/lib/buildid.c
> index 5c869a2a30ab..6b5558cd95bf 100644
> --- a/lib/buildid.c
> +++ b/lib/buildid.c
> @@ -20,6 +20,7 @@ struct freader {
>                         struct page *page;
>                         void *page_addr;
>                         u64 file_off;
> +                       bool may_fault;
>                 };
>                 struct {
>                         const char *data;
> @@ -29,12 +30,13 @@ struct freader {
>  };
>
>  static void freader_init_from_file(struct freader *r, void *buf, u32 buf_sz,
> -                                  struct address_space *mapping)
> +                                  struct address_space *mapping, bool may_fault)
>  {
>         memset(r, 0, sizeof(*r));
>         r->buf = buf;
>         r->buf_sz = buf_sz;
>         r->mapping = mapping;
> +       r->may_fault = may_fault;
>  }
>
>  static void freader_init_from_mem(struct freader *r, const char *data, u64 data_sz)
> @@ -60,6 +62,17 @@ static int freader_get_page(struct freader *r, u64 file_off)
>         freader_put_page(r);
>
>         r->page = find_get_page(r->mapping, pg_off);
> +
> +       if (!r->page && r->may_fault) {
> +               struct folio *folio;
> +
> +               folio = read_cache_folio(r->mapping, pg_off, NULL, NULL);
> +               if (IS_ERR(folio))
> +                       return PTR_ERR(folio);
> +
> +               r->page = folio_file_page(folio, pg_off);
> +       }
> +

mm folks, is this the sane way to do this? Can you please take a look
and provide your ack? Thank you!

>         if (!r->page)
>                 return -EFAULT; /* page not mapped */
>
> @@ -273,18 +286,8 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
>  /* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */
>  #define MAX_FREADER_BUF_SZ 64
>
> -/*
> - * Parse build ID of ELF file mapped to vma
> - * @vma:      vma object
> - * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
> - * @size:     returns actual build id size in case of success
> - *
> - * Assumes no page fault can be taken, so if relevant portions of ELF file are
> - * not already paged in, fetching of build ID fails.
> - *
> - * Return: 0 on success; negative error, otherwise
> - */
> -int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
> +static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
> +                           __u32 *size, bool may_fault)
>  {
>         const Elf32_Ehdr *ehdr;
>         struct freader r;
> @@ -295,7 +298,7 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
>         if (!vma->vm_file)
>                 return -EINVAL;
>
> -       freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file->f_mapping);
> +       freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file->f_mapping, may_fault);
>
>         /* fetch first 18 bytes of ELF header for checks */
>         ehdr = freader_fetch(&r, 0, offsetofend(Elf32_Ehdr, e_type));
> @@ -323,6 +326,22 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
>         return ret;
>  }
>
> +/*
> + * Parse build ID of ELF file mapped to vma
> + * @vma:      vma object
> + * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
> + * @size:     returns actual build id size in case of success
> + *
> + * Assumes no page fault can be taken, so if relevant portions of ELF file are
> + * not already paged in, fetching of build ID fails.
> + *
> + * Return: 0 on success; negative error, otherwise
> + */
> +int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
> +{
> +       return __build_id_parse(vma, build_id, size, false /* !may_fault */);
> +}
> +
>  /*
>   * Parse build ID of ELF file mapped to VMA
>   * @vma:      vma object
> @@ -336,8 +355,7 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
>   */
>  int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
>  {
> -       /* fallback to non-faultable version for now */
> -       return build_id_parse_nofault(vma, build_id, size);
> +       return __build_id_parse(vma, build_id, size, true /* may_fault */);
>  }
>
>  /**
> --
> 2.43.0
>
>