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

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

[Andrii Nakryiko]

+ bpf@

On Mon, Nov 17, 2025 at 6:10 AM Shaurya Rane <ssrane_b23@ee.vjti.ac.in> wrote:
>
> 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.

It is bad, I'm sorry for this. I was surprised to read this, though,
as I was not aware of any bug related to build ID parsing code, so I
went looking at syzbot history of the issue. August timeframe you are
referring to implies those "Monthly fs report" emails, which
unfortunately I didn't receive as I'm not subscribed to linux-fsdevel,
but I do see that there was earlier report back in April, which I did
get in my inbox, apparently. So I'm sorry again for dropping the ball.
Please feel free to ping me or BPF mailing list next time when you see
something not being addressed in a timely manner.

> >
> > 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.
> >
> Apologies for the process errors regarding the v2 submission. I appreciate the guidance on the workflow and threading; I will ensure the next version is sent as a clean, new thread once we have agreed on the technical solution.
> > 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 agree that DAX needs proper support rather than silent filtering.
> However, investigating the actual syzbot reproducer revealed that the issue extends beyond just DAX. The crash is actually triggering on tmpfs (shmem).I verified via debug logging that the crashing VMA is backed by `shmem_aops`. Looking at `mm/shmem.c`, tmpfs legitimately lacks a `.read_folio` implementation by design.
> It seems there are several "real" filesystems that can contain executables/libraries but lack `.read_folio`:
> 1. tmpfs/shmem
> 2. OverlayFS (delegates I/O)
> 3. DAX filesystems
> Given that this affects multiple filesystem types, handling them all correctly via `kernel_read` might be a larger scope than fixing the immediate crash. I worry about missing edge cases in tmpfs or OverlayFS if we try to implement the fallback immediately in this patch.
> > 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.
> I propose the following approach for v3. It avoids the silent failure you are concerned about, but prevents the kernel panic:
>
> 1. Silent reject for `empty_aops` (procfs/sysfs), as they legitimately can't contain build IDs.
> 2. Loud warning + Error for other cases (DAX, tmpfs, OverlayFS).
>

Tbh, it seems a bit fragile to have to hard-code such file
system-specific logic in higher-level build ID fetching logic, where
all we really ask for from filemap_get_folio() + read_cache_folio()
combo is to give us requested piece of file or let us know (without
crashing) that this was not possible.

But if there is no way to abstract this away, then I think Shaurya
proposed with failing known-not-supported cases and warning on
unexpected ones would be a reasonable solution, I suppose. I see that
Matthew is discussing generalizing kernel_read, so maybe that will be
a better solution, let's see.


> The code would look like this:
>
>     /* pseudo-filesystems */
>     if (vma->vm_file->f_mapping->a_ops == &empty_aops)
>         return -EINVAL;
>
>     /* Real filesystems missing read_folio (DAX, tmpfs, OverlayFS, etc.) */
>     if (!vma->vm_file->f_mapping->a_ops->read_folio) {
>         /*
>          * TODO: Implement kernel_read() fallback for DAX/tmpfs.
>          * For now, fail loudly so we know what we are missing.
>          */
>         pr_warn_once("build_id_parse: filesystem %s lacks read_folio support\n",
>                      vma->vm_file->f_path.dentry->d_sb->s_type->name);
>         return -EOPNOTSUPP;
>     }
>
> This highlights exactly which filesystems are missing support in the logs without crashing the machine
> Thanks,
> Shaurya

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

[Andrii Nakryiko]

+ bpf@

On Mon, Nov 17, 2025 at 10:03 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Nov 17, 2025 at 08:41:55AM -0800, Darrick J. Wong wrote:
> > I wondered why this whole thing opencodes kernel_read, but then I
> > noticed zero fstests for it and decid*******************************
> > *****.
>
> I wondered the same thing!  And the answer is that it's special BPF
> stuff:
>
>         /* if sleeping is allowed, wait for the page, if necessary */
>         if (r->may_fault && (IS_ERR(r->folio) || !folio_test_uptodate(r->folio))) {
>                 filemap_invalidate_lock_shared(r->file->f_mapping);
>                 r->folio = read_cache_folio(r->file->f_mapping, file_off >> PAGE_SHIFT,
>                                             NULL, r->file);
>                 filemap_invalidate_unlock_shared(r->file->f_mapping);
>         }
>
> if 'may_fault' (a misnomer since it really means "may sleep"), then we
> essentially do kernel_read().
>
> Now, maybe the right thing to do here is rip out almost all of
> lib/buildid.c and replace it with an iocb with IOCB_NOWAIT set (or not).
> I was hesitant to suggest this earlier as it's a bit of a big ask of
> someone who was just trying to submit a one-line change.  But now that
> "it's also shmem" has entered the picture, I'm leaning more towards this
> approach anyway.

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 only real limitation is that we'd like to be able to control
whether we are ok sleeping or not, as this code can be called from
pretty much anywhere BPF might run, which includes NMI context.

Would this kiocb_read() approach work under those circumstances?

>
> Looking at it though, it's a bit weird that we don't have a
> kiocb_read().  It feels like __kernel_read() needs to be split into
> half like:
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 833bae068770..a3bf962836a7 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -503,14 +503,29 @@ static int warn_unsupported(struct file *file, const char *op)
>         return -EINVAL;
>  }
>
> -ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
> +ssize_t kiocb_read(struct kiocb *iocb, void *buf, size_t count)
>  {
> +       struct file *file = iocb->ki_filp;
>         struct kvec iov = {
>                 .iov_base       = buf,
>                 .iov_len        = min_t(size_t, count, MAX_RW_COUNT),
>         };
> -       struct kiocb kiocb;
>         struct iov_iter iter;
> +       int ret;
> +
> +       iov_iter_kvec(&iter, ITER_DEST, &iov, 1, iov.iov_len);
> +       ret = file->f_op->read_iter(iocb, &iter);
> +       if (ret > 0) {
> +               fsnotify_access(file);
> +               add_rchar(current, ret);
> +       }
> +       inc_syscr(current);
> +       return ret;
> +}
> +
> +ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
> +{
> +       struct kiocb kiocb;
>         ssize_t ret;
>
>         if (WARN_ON_ONCE(!(file->f_mode & FMODE_READ)))
> @@ -526,15 +541,9 @@ ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
>
>         init_sync_kiocb(&kiocb, file);
>         kiocb.ki_pos = pos ? *pos : 0;
> -       iov_iter_kvec(&iter, ITER_DEST, &iov, 1, iov.iov_len);
> -       ret = file->f_op->read_iter(&kiocb, &iter);
> -       if (ret > 0) {
> -               if (pos)
> -                       *pos = kiocb.ki_pos;
> -               fsnotify_access(file);
> -               add_rchar(current, ret);
> -       }
> -       inc_syscr(current);
> +       ret = kiocb_read(&kiocb, buf, count);
> +       if (pos && ret > 0)
> +               *pos = kiocb.ki_pos;
>         return ret;
>  }
>

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

[Christoph Hellwig]

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.  But bpf or any other file system consumer has
absolutely not business poking into the page cache to start with.

And I'm really pissed off that you wrote and merged this code without
ever bothering to talk to a FS or MM person who have immediately told
you so.  Let's just rip out this buildid junk for now and restart
because the problem isn't actually that easy.
> 
> The only real limitation is that we'd like to be able to control
> whether we are ok sleeping or not, as this code can be called from
> pretty much anywhere BPF might run, which includes NMI context.
> 
> Would this kiocb_read() approach work under those circumstances?

No.  IOCB_NOWAIT is just a hint to avoid blocking function calls.
It is not guarantee and a guarantee is basically impossible.

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

[Matthew Wilcox]

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.

> And I'm really pissed off that you wrote and merged this code without
> ever bothering to talk to a FS or MM person who have immediately told
> you so.  Let's just rip out this buildid junk for now and restart
> because the problem isn't actually that easy.

Oh, they did talk to fs & mm people originally and were told NO, so they
sneaked it in through the BPF tree.

https://lore.kernel.org/all/20230316170149.4106586-1-jolsa@kernel.org/

> > The only real limitation is that we'd like to be able to control
> > whether we are ok sleeping or not, as this code can be called from
> > pretty much anywhere BPF might run, which includes NMI context.
> > 
> > Would this kiocb_read() approach work under those circumstances?
> 
> No.  IOCB_NOWAIT is just a hint to avoid blocking function calls.
> It is not guarantee and a guarantee is basically impossible.

I'm not sure I'd go that far -- I think we're pretty good about not
sleeping when IOCB_NOWAIT is specified and any remaining places can
be fixed up.

But I am inclined to rip out the buildid code, just because the
authors have been so rude.

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

[Darrick J. Wong]

On Tue, Nov 18, 2025 at 03:37:09PM +0000, Matthew Wilcox 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.
> 
> > And I'm really pissed off that you wrote and merged this code without
> > ever bothering to talk to a FS or MM person who have immediately told
> > you so.  Let's just rip out this buildid junk for now and restart
> > because the problem isn't actually that easy.
> 
> Oh, they did talk to fs & mm people originally and were told NO, so they
> sneaked it in through the BPF tree.
> 
> https://lore.kernel.org/all/20230316170149.4106586-1-jolsa@kernel.org/
> 
> > > The only real limitation is that we'd like to be able to control
> > > whether we are ok sleeping or not, as this code can be called from
> > > pretty much anywhere BPF might run, which includes NMI context.
> > > 
> > > Would this kiocb_read() approach work under those circumstances?
> > 
> > No.  IOCB_NOWAIT is just a hint to avoid blocking function calls.
> > It is not guarantee and a guarantee is basically impossible.
> 
> I'm not sure I'd go that far -- I think we're pretty good about not
> sleeping when IOCB_NOWAIT is specified and any remaining places can
> be fixed up.
> 
> But I am inclined to rip out the buildid code, just because the
> authors have been so rude.

Which fstest actually checks the functionality of the buildid code?
I don't find any, which means none of the fs people have a good signal
for breakage in this, um, novel file I/O path.

--D

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

[Andrii Nakryiko]

On Tue, Nov 18, 2025 at 7:37 AM Matthew Wilcox <willy@infradead.org> 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.

>
> > And I'm really pissed off that you wrote and merged this code without
> > ever bothering to talk to a FS or MM person who have immediately told
> > you so.  Let's just rip out this buildid junk for now and restart
> > because the problem isn't actually that easy.
>
> Oh, they did talk to fs & mm people originally and were told NO, so they
> sneaked it in through the BPF tree.

This patch set was never landed and has *NO* relation to the
lib/buildid.c stuff we are discussing. There was no sneaking anything
in. The patch set in question that added folio-based reading logic was
developed in the open with both mm and fsdevel in CC. Matthew himself
looked at it, he NAKed page-based initial implementation but suggested
folio-based one ([0]). Shakeel did review this (the patch set went
through 10 revisions, plenty of time to object).

  [0] https://lore.kernel.org/bpf/ZrOStYOrlFr21jRc@casper.infradead.org/

>
> https://lore.kernel.org/all/20230316170149.4106586-1-jolsa@kernel.org/
>
> > > The only real limitation is that we'd like to be able to control
> > > whether we are ok sleeping or not, as this code can be called from
> > > pretty much anywhere BPF might run, which includes NMI context.
> > >
> > > Would this kiocb_read() approach work under those circumstances?
> >
> > No.  IOCB_NOWAIT is just a hint to avoid blocking function calls.
> > It is not guarantee and a guarantee is basically impossible.
>
> I'm not sure I'd go that far -- I think we're pretty good about not
> sleeping when IOCB_NOWAIT is specified and any remaining places can
> be fixed up.
>
> But I am inclined to rip out the buildid code, just because the
> authors have been so rude.

Can you please elaborate on "authors have been so rude" a bit more?
Besides that link to an absolutely unrelated patch set?..

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

[Andrii Nakryiko]

On Tue, Nov 18, 2025 at 8:12 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Nov 18, 2025 at 03:37:09PM +0000, Matthew Wilcox 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.
> >
> > > And I'm really pissed off that you wrote and merged this code without
> > > ever bothering to talk to a FS or MM person who have immediately told
> > > you so.  Let's just rip out this buildid junk for now and restart
> > > because the problem isn't actually that easy.
> >
> > Oh, they did talk to fs & mm people originally and were told NO, so they
> > sneaked it in through the BPF tree.
> >
> > https://lore.kernel.org/all/20230316170149.4106586-1-jolsa@kernel.org/
> >
> > > > The only real limitation is that we'd like to be able to control
> > > > whether we are ok sleeping or not, as this code can be called from
> > > > pretty much anywhere BPF might run, which includes NMI context.
> > > >
> > > > Would this kiocb_read() approach work under those circumstances?
> > >
> > > No.  IOCB_NOWAIT is just a hint to avoid blocking function calls.
> > > It is not guarantee and a guarantee is basically impossible.
> >
> > I'm not sure I'd go that far -- I think we're pretty good about not
> > sleeping when IOCB_NOWAIT is specified and any remaining places can
> > be fixed up.
> >
> > But I am inclined to rip out the buildid code, just because the
> > authors have been so rude.
>
> Which fstest actually checks the functionality of the buildid code?
> I don't find any, which means none of the fs people have a good signal
> for breakage in this, um, novel file I/O path.

We have plenty of build ID tests in BPF selftest that validate this
functionality:

  - tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
  - tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
  - tools/testing/selftests/bpf/prog_tests/build_id.c

This functionality is exposed to BPF (and PROCMAP_QUERY, which has its
own mm selftests), so that's where we test this. So we'll know at the
very least when trees merge that something is broken.

>
> --D

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

[Christoph Hellwig]

On Tue, Nov 18, 2025 at 11:27:47AM -0800, Andrii Nakryiko wrote:
> 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.

That's pretty demanding:  "If you don't give me the interface that I want
I'll just poke into internals and do broken shit" isn't really the
best way to make friends and win influence.,

> If you don't like the implementation, please help improve it, don't
> just request dropping it "because BPF folks" or anything like that.

Again, you're trying to put a lot of work you should have done on
others.  Everyone here is pretty helpful guiding when asking for help,
but being asked at gunpoint to cleanup the mess your created is not
going to get everyone drop their work and jump onto your project.

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

[Christoph Hellwig]

On Tue, Nov 18, 2025 at 11:38:36AM -0800, Andrii Nakryiko wrote:
> We have plenty of build ID tests in BPF selftest that validate this
> functionality:

And none of them is run on a wide variety of file systems, presumably
because they are only run by BFP developers.

IFF we allow for magic file access methods it needs to be part of
regular file system testing.

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

[Darrick J. Wong]

On Tue, Nov 18, 2025 at 11:38:36AM -0800, Andrii Nakryiko wrote:
> On Tue, Nov 18, 2025 at 8:12 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Tue, Nov 18, 2025 at 03:37:09PM +0000, Matthew Wilcox 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.
> > >
> > > > And I'm really pissed off that you wrote and merged this code without
> > > > ever bothering to talk to a FS or MM person who have immediately told
> > > > you so.  Let's just rip out this buildid junk for now and restart
> > > > because the problem isn't actually that easy.
> > >
> > > Oh, they did talk to fs & mm people originally and were told NO, so they
> > > sneaked it in through the BPF tree.
> > >
> > > https://lore.kernel.org/all/20230316170149.4106586-1-jolsa@kernel.org/
> > >
> > > > > The only real limitation is that we'd like to be able to control
> > > > > whether we are ok sleeping or not, as this code can be called from
> > > > > pretty much anywhere BPF might run, which includes NMI context.
> > > > >
> > > > > Would this kiocb_read() approach work under those circumstances?
> > > >
> > > > No.  IOCB_NOWAIT is just a hint to avoid blocking function calls.
> > > > It is not guarantee and a guarantee is basically impossible.
> > >
> > > I'm not sure I'd go that far -- I think we're pretty good about not
> > > sleeping when IOCB_NOWAIT is specified and any remaining places can
> > > be fixed up.
> > >
> > > But I am inclined to rip out the buildid code, just because the
> > > authors have been so rude.
> >
> > Which fstest actually checks the functionality of the buildid code?
> > I don't find any, which means none of the fs people have a good signal
> > for breakage in this, um, novel file I/O path.
> 
> We have plenty of build ID tests in BPF selftest that validate this
> functionality:
> 
>   - tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
>   - tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
>   - tools/testing/selftests/bpf/prog_tests/build_id.c
> 
> This functionality is exposed to BPF (and PROCMAP_QUERY, which has its
> own mm selftests), so that's where we test this. So we'll know at the
> very least when trees merge that something is broken.

Only if you're testing the buildid functionality with all known file I/O
paths implemented by all filesystems.  Or you could add a new testcase
to fstests and we'd do all that *for* you.

--D

> >
> > --D
> 

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

[Andrii Nakryiko]

On Tue, Nov 18, 2025 at 9:50 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Nov 18, 2025 at 11:27:47AM -0800, Andrii Nakryiko wrote:
> > 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.
>
> That's pretty demanding:  "If you don't give me the interface that I want
> I'll just poke into internals and do broken shit" isn't really the
> best way to make friends and win influence.,

Did you read the second part of my reply? The functionality in
question ([0]) was developed in the open, over multiple revisions,
with both mm and fsdevel mailing list CC'ed. Matthew Wilcox did look
at this, provided feedback and suggestion to use filemap_get_folio() +
read_cache_folio(), which I did incorporate.

  [0] https://lore.kernel.org/bpf/20240829174232.3133883-1-andrii@kernel.org/

>
> > If you don't like the implementation, please help improve it, don't
> > just request dropping it "because BPF folks" or anything like that.
>
> Again, you're trying to put a lot of work you should have done on
> others.  Everyone here is pretty helpful guiding when asking for help,
> but being asked at gunpoint to cleanup the mess your created is not
> going to get everyone drop their work and jump onto your project.

Gunpoint, really?.. Am I not asking for help to improve the code? This
functionality is being used, and we can't "just rip it out" as you
propose. Let's fix it instead.

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@infradead.org> 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:


From 9652cc97a817fe35e53a7e98a5fbb49c7788c744 Mon Sep 17 00:00:00 2001
From: Shakeel Butt <shakeel.butt@linux.dev>
Date: Tue, 16 Dec 2025 16:53:57 -0800
Subject: [PATCH] lib/buildid: convert freader to use __kernel_read()

Convert the freader file reading implementation from direct page cache
access via filemap_get_folio()/read_cache_folio() to using kernel_read
interfaces.

Add a new __kernel_read_nowait() function that uses IOCB_NOWAIT flag
for non-blocking I/O. This is used when may_fault is false to avoid
blocking on I/O - if data is not immediately available, it returns
-EAGAIN.

For the may_fault case, use the standard __kernel_read() which can
block waiting for I/O.

This simplifies the code by removing the need to manage folios,
kmap/kunmap operations, and page cache locking. It also makes the
code work with filesystems that don't use the page cache directly.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 fs/read_write.c         | 18 ++++++++-
 include/linux/buildid.h |  3 --
 include/linux/fs.h      |  1 +
 lib/buildid.c           | 85 +++++++++--------------------------------
 4 files changed, 37 insertions(+), 70 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 833bae068770..7a042cfeefec 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -503,7 +503,8 @@ static int warn_unsupported(struct file *file, const char *op)
 	return -EINVAL;
 }
 
-ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
+static ssize_t __kernel_read_internal(struct file *file, void *buf,
+			size_t count, loff_t *pos, int flags)
 {
 	struct kvec iov = {
 		.iov_base	= buf,
@@ -526,6 +527,7 @@ ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
 
 	init_sync_kiocb(&kiocb, file);
 	kiocb.ki_pos = pos ? *pos : 0;
+	kiocb.ki_flags |= flags;
 	iov_iter_kvec(&iter, ITER_DEST, &iov, 1, iov.iov_len);
 	ret = file->f_op->read_iter(&kiocb, &iter);
 	if (ret > 0) {
@@ -538,6 +540,20 @@ ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
 	return ret;
 }
 
+ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
+{
+	return __kernel_read_internal(file, buf, count, pos, 0);
+}
+
+/*
+ * Non-blocking variant of __kernel_read() using IOCB_NOWAIT.
+ * Returns -EAGAIN if the read would block waiting for I/O.
+ */
+ssize_t __kernel_read_nowait(struct file *file, void *buf, size_t count, loff_t *pos)
+{
+	return __kernel_read_internal(file, buf, count, pos, IOCB_NOWAIT);
+}
+
 ssize_t kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
 {
 	ssize_t ret;
diff --git a/include/linux/buildid.h b/include/linux/buildid.h
index 831c1b4b626c..f1fa220353a2 100644
--- a/include/linux/buildid.h
+++ b/include/linux/buildid.h
@@ -25,9 +25,6 @@ struct freader {
 	union {
 		struct {
 			struct file *file;
-			struct folio *folio;
-			void *addr;
-			loff_t folio_off;
 			bool may_fault;
 		};
 		struct {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f5c9cf28c4dc..498c804fc0b9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2832,6 +2832,7 @@ extern int do_pipe_flags(int *, int);
 
 extern ssize_t kernel_read(struct file *, void *, size_t, loff_t *);
 ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos);
+ssize_t __kernel_read_nowait(struct file *file, void *buf, size_t count, loff_t *pos);
 extern ssize_t kernel_write(struct file *, const void *, size_t, loff_t *);
 extern ssize_t __kernel_write(struct file *, const void *, size_t, loff_t *);
 extern struct file * open_exec(const char *);
diff --git a/lib/buildid.c b/lib/buildid.c
index aaf61dfc0919..c9d4491557fe 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -5,6 +5,7 @@
 #include <linux/elf.h>
 #include <linux/kernel.h>
 #include <linux/pagemap.h>
+#include <linux/fs.h>
 #include <linux/secretmem.h>
 
 #define BUILD_ID 3
@@ -28,55 +29,35 @@ void freader_init_from_mem(struct freader *r, const char *data, u64 data_sz)
 	r->data_sz = data_sz;
 }
 
-static void freader_put_folio(struct freader *r)
-{
-	if (!r->folio)
-		return;
-	kunmap_local(r->addr);
-	folio_put(r->folio);
-	r->folio = NULL;
-}
-
-static int freader_get_folio(struct freader *r, loff_t file_off)
+/*
+ * Read data from file at specified offset into the freader buffer.
+ * Uses non-blocking I/O when may_fault is false.
+ * Returns 0 on success, negative error code on failure.
+ */
+static int freader_read(struct freader *r, loff_t file_off, size_t sz)
 {
-	/* check if we can just reuse current folio */
-	if (r->folio && file_off >= r->folio_off &&
-	    file_off < r->folio_off + folio_size(r->folio))
-		return 0;
-
-	freader_put_folio(r);
+	ssize_t ret;
+	loff_t pos = file_off;
 
 	/* reject secretmem folios created with memfd_secret() */
 	if (secretmem_mapping(r->file->f_mapping))
 		return -EFAULT;
 
-	r->folio = filemap_get_folio(r->file->f_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))) {
-		filemap_invalidate_lock_shared(r->file->f_mapping);
-		r->folio = read_cache_folio(r->file->f_mapping, file_off >> PAGE_SHIFT,
-					    NULL, r->file);
-		filemap_invalidate_unlock_shared(r->file->f_mapping);
-	}
+	if (r->may_fault)
+		ret = __kernel_read(r->file, r->buf, sz, &pos);
+	else
+		ret = __kernel_read_nowait(r->file, r->buf, sz, &pos);
 
-	if (IS_ERR(r->folio) || !folio_test_uptodate(r->folio)) {
-		if (!IS_ERR(r->folio))
-			folio_put(r->folio);
-		r->folio = NULL;
+	if (ret < 0)
+		return ret;
+	if (ret != sz)
 		return -EFAULT;
-	}
-
-	r->folio_off = folio_pos(r->folio);
-	r->addr = kmap_local_folio(r->folio, 0);
 
 	return 0;
 }
 
 const void *freader_fetch(struct freader *r, loff_t file_off, size_t sz)
 {
-	size_t folio_sz;
-
 	/* provided internal temporary buffer should be sized correctly */
 	if (WARN_ON(r->buf && sz > r->buf_sz)) {
 		r->err = -E2BIG;
@@ -97,46 +78,18 @@ const void *freader_fetch(struct freader *r, loff_t file_off, size_t sz)
 		return r->data + file_off;
 	}
 
-	/* fetch or reuse folio for given file offset */
-	r->err = freader_get_folio(r, file_off);
+	/* read data from file into buffer */
+	r->err = freader_read(r, file_off, sz);
 	if (r->err)
 		return NULL;
 
-	/* if requested data is crossing folio boundaries, we have to copy
-	 * everything into our local buffer to keep a simple linear memory
-	 * access interface
-	 */
-	folio_sz = folio_size(r->folio);
-	if (file_off + sz > r->folio_off + folio_sz) {
-		u64 part_sz = r->folio_off + folio_sz - file_off, off;
-
-		memcpy(r->buf, r->addr + file_off - r->folio_off, part_sz);
-		off = part_sz;
-
-		while (off < sz) {
-			/* fetch next folio */
-			r->err = freader_get_folio(r, r->folio_off + folio_sz);
-			if (r->err)
-				return NULL;
-			folio_sz = folio_size(r->folio);
-			part_sz = min_t(u64, sz - off, folio_sz);
-			memcpy(r->buf + off, r->addr, part_sz);
-			off += part_sz;
-		}
-
-		return r->buf;
-	}
-
-	/* if data fits in a single folio, just return direct pointer */
-	return r->addr + (file_off - r->folio_off);
+	return r->buf;
 }
 
 void freader_cleanup(struct freader *r)
 {
 	if (!r->buf)
 		return; /* non-file-backed mode */
-
-	freader_put_folio(r);
 }
 
 /*
-- 
2.47.3