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

Darrick J. Wong
2025-11-17T16:41:56Z

On Sun, Nov 16, 2025 at 10:32:12PM +0000, Matthew Wilcox wrote:
> First, some process things ;-)
>
> 1. Thank you for working on this. Andrii has been ignoring it since
> August, which is bad. So thank you for picking it up.
>
> 2. Sending a v2 while we're having a discussion is generally a bad idea.
> It's fine to send a patch as a reply, but going as far as a v2 isn't
> necessary. If conversation has died down, then a v2 is definitely
> warranted, but you and I are still having a discussion ;-)
>
> 3. When you do send a v2 (or, now that you've sent a v2, send a v3),
> do it as a new thread rather then in reply to the v1 thread. That plays
> better with the tooling we have like b4 which will pull in all patches
> in a thread.
>
> With that over with, on to the fun technical stuff.
>
> On Sun, Nov 16, 2025 at 11:13:42AM +0530, SHAURYA RANE wrote:
> > On Sat, Nov 15, 2025 at 2:14 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Sat, Nov 15, 2025 at 01:07:29AM +0530, ssrane_b23@ee.vjti.ac.in wrote:
> > > > When read_cache_folio() is called with a NULL filler function on a
> > > > mapping that does not implement read_folio, a NULL pointer
> > > > dereference occurs in filemap_read_folio().
> > > >
> > > > The crash occurs when:
> > > >
> > > > build_id_parse() is called on a VMA backed by a file from a
> > > > filesystem that does not implement ->read_folio() (e.g. procfs,
> > > > sysfs, or other virtual filesystems).
> > >
> > > Not a fan of this approach, to be honest. This should be caught at
> > > a higher level. In __build_id_parse(), there's already a check:
> > >
> > > /* only works for page backed storage */
> > > if (!vma->vm_file)
> > > return -EINVAL;
> > >
> > > which is funny because the comment is correct, but the code is not.
> > > I suspect the right answer is to add right after it:
> > >
> > > + if (vma->vm_file->f_mapping->a_ops == &empty_aops)
> > > + return -EINVAL;
> > >
> > > Want to test that out?
> > Thanks for the suggestion.
> > Checking for
> > a_ops == &empty_aops
> > is not enough. Certain filesystems for example XFS with DAX use
> > their own a_ops table (not empty_aops) but still do not implement
> > ->read_folio(). In those cases read_cache_folio() still ends up with
> > filler = NULL and filemap_read_folio(NULL) crashes.
>
> Ah, right. I had assumed that the only problem was synthetic
> filesystems like sysfs and procfs which can't have buildids because
> buildids only exist in executables. And neither procfs nor sysfs
> contain executables.
>
> But DAX is different. You can absolutely put executables on a DAX
> filesystem. So we shouldn't filter out DAX here. And we definitely
> shouldn't *silently* fail for DAX. Otherwise nobody will ever realise
> that the buildid people just couldn't be bothered to make DAX work.
>
> I don't think it's necessarily all that hard to make buildid work
> for DAX. It's probably something like:
>
> if (IS_DAX(file_inode(file)))
> kernel_read(file, buf, count, &pos);
>
> but that's just off the top of my head.

I wondered why this whole thing opencodes kernel_read, but then I
noticed zero fstests for it and decid*******************************
*****.

--D

>
> I really don't want the check for filler being NULL in read_cache_folio().
> I want it to crash noisily if callers are doing something stupid.
>