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

[Shakeel Butt <shakeel.butt@linux.dev>]

On Wed, Aug 07, 2024 at 04:40:25PM GMT, Andrii Nakryiko 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 folio into page
> cache, after which the rest of the logic works just the same at folio 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 | 44 ++++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/buildid.c b/lib/buildid.c
> index 5e6f842f56f0..e1c01b23efd8 100644
> --- a/lib/buildid.c
> +++ b/lib/buildid.c
> @@ -20,6 +20,7 @@ struct freader {
>  			struct folio *folio;
>  			void *addr;
>  			loff_t folio_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)
> @@ -63,6 +65,11 @@ static int freader_get_folio(struct freader *r, loff_t file_off)
>  	freader_put_folio(r);
>  
>  	r->folio = filemap_get_folio(r->mapping, file_off >> PAGE_SHIFT);
> +
> +	/* if sleeping is allowed, wait for the page, if necessary */
> +	if (r->may_fault && (IS_ERR(r->folio) || !folio_test_uptodate(r->folio)))
> +		r->folio = read_cache_folio(r->mapping, file_off >> PAGE_SHIFT, NULL, NULL);

Willy's network fs comment is bugging me. If we pass NULL for filler,
the kernel will going to use fs's read_folio() callback. I have checked
read_folio() for fuse and nfs and it seems like for at least these two
filesystems the callback is accessing file->private_data. So, if the elf
file is on these filesystems, we might see null accesses.