Re: [PATCH] mm/filemap: fix NULL pointer dereference in do_read_cache_folio()

[Shakeel Butt]

On Tue, Nov 18, 2025 at 11:27:47AM -0800, Andrii Nakryiko wrote:
> On Tue, Nov 18, 2025 at 7:37 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> >
> > On Tue, Nov 18, 2025 at 05:03:24AM -0800, Christoph Hellwig wrote:
> > > On Mon, Nov 17, 2025 at 10:45:31AM -0800, Andrii Nakryiko wrote:
> > > > As I replied on another email, ideally we'd have some low-level file
> > > > reading interface where we wouldn't have to know about secretmem, or
> > > > XFS+DAX, or whatever other unusual combination of conditions where
> > > > exposed internal APIs like filemap_get_folio() + read_cache_folio()
> > > > can crash.
> > >
> > > The problem is that you did something totally insane and it kinda works
> > > most of the time.
> >
> > ... on 64-bit systems.  The HIGHMEM handling is screwed up too.
> >
> > > But bpf or any other file system consumer has
> > > absolutely not business poking into the page cache to start with.
> >
> > Agreed.
> 
> Then please help make it better, give us interfaces you think are
> appropriate. People do use this functionality in production, it's
> important and we are not going to drop it. In non-sleepable mode it's
> best-effort, if the requested part of the file is paged in, we'll
> successfully read data (such as ELF's build ID), and if not, we'll
> report that to the BPF program as -EFAULT. In sleepable mode, we'll
> wait for that part of the file to be paged in before proceeding.
> PROCMAP_QUERY ioctl() is always in sleepable mode, so it will wait for
> file data to be read.
> 
> If you don't like the implementation, please help improve it, don't
> just request dropping it "because BPF folks" or anything like that.
> 

So, I took a stab at this, particularly based on Willy's suggestions on
IOCB_NOWAIT. This is untested and I am just sharing to show how it looks
like and if there are any concerns. In addition I think I will look into
fstest part as well.

BTW by simple code inspection I already see that IOCB_NOWAIT is not well
respected. For example filemap_read() is doing cond_resched() without
any checks. The readahead i.e. page_cache_sync_ra() can potential take
sleeping locks. Btrfs is taking locks in btrfs_file_read_iter. So, it
seems like this would need extensive testing hopefully for all major
FSes.

Here the draft patch: