[PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Shakeel Butt]

For the sleepable context, convert freader to use __kernel_read()
instead of direct page cache access via read_cache_folio(). This
simplifies the faultable code path by using the standard kernel file
reading interface which handles all the complexity of reading file data.

At the moment we are not changing the code for non-sleepable context
which uses filemap_get_folio() and only succeeds if the target folios
are already in memory and up-to-date. The reason is to keep the patch
simple and easier to backport to stable kernels.

Syzbot repro does not crash the kernel anymore and the selftests run
successfully.

In the follow up we will make __kernel_read() with IOCB_NOWAIT work for
non-sleepable contexts. In addition, I would like to replace the
secretmem check with a more generic approach and will add fstest for the
buildid code.

Reported-by: syzbot+09b7d050e4806540153d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=09b7d050e4806540153d
Fixes: ad41251c290d ("lib/buildid: implement sleepable build_id_parse() API")
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---

Changes since v1:
- Fix handling of buf in freader_fetch_sync as pointed out by Andrii.

 lib/buildid.c | 47 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/lib/buildid.c b/lib/buildid.c
index aaf61dfc0919..93b3a06e7f7a 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
@@ -37,6 +38,29 @@ static void freader_put_folio(struct freader *r)
 	r->folio = NULL;
 }
 
+/*
+ * Data is read directly into r->buf. Returns pointer to the buffer
+ * on success, NULL on failure with r->err set.
+ */
+static const void *freader_fetch_sync(struct freader *r, loff_t file_off, size_t sz)
+{
+	ssize_t ret;
+	loff_t pos = file_off;
+	char *buf = r->buf;
+
+	do {
+		ret = __kernel_read(r->file, buf, sz, &pos);
+		if (ret <= 0) {
+			r->err = ret ?: -EIO;
+			return NULL;
+		}
+		buf += ret;
+		sz -= ret;
+	} while (sz > 0);
+
+	return r->buf;
+}
+
 static int freader_get_folio(struct freader *r, loff_t file_off)
 {
 	/* check if we can just reuse current folio */
@@ -46,20 +70,9 @@ static int freader_get_folio(struct freader *r, loff_t file_off)
 
 	freader_put_folio(r);
 
-	/* reject secretmem folios created with memfd_secret() */
-	if (secretmem_mapping(r->file->f_mapping))
-		return -EFAULT;
-
+	/* only use page cache lookup - fail if not already cached */
 	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 (IS_ERR(r->folio) || !folio_test_uptodate(r->folio)) {
 		if (!IS_ERR(r->folio))
 			folio_put(r->folio);
@@ -97,6 +110,16 @@ const void *freader_fetch(struct freader *r, loff_t file_off, size_t sz)
 		return r->data + file_off;
 	}
 
+	/* reject secretmem folios created with memfd_secret() */
+	if (secretmem_mapping(r->file->f_mapping)) {
+		r->err = -EFAULT;
+		return NULL;
+	}
+
+	/* use __kernel_read() for sleepable context */
+	if (r->may_fault)
+		return freader_fetch_sync(r, file_off, sz);
+
 	/* fetch or reuse folio for given file offset */
 	r->err = freader_get_folio(r, file_off);
 	if (r->err)
-- 
2.47.3

Re: [PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Andrew Morton]

On Thu, 18 Dec 2025 12:55:05 -0800 Shakeel Butt <shakeel.butt@linux.dev> wrote:

> For the sleepable context, convert freader to use __kernel_read()
> instead of direct page cache access via read_cache_folio(). This
> simplifies the faultable code path by using the standard kernel file
> reading interface which handles all the complexity of reading file data.
> 
> At the moment we are not changing the code for non-sleepable context
> which uses filemap_get_folio() and only succeeds if the target folios
> are already in memory and up-to-date. The reason is to keep the patch
> simple and easier to backport to stable kernels.
> 
> Syzbot repro does not crash the kernel anymore and the selftests run
> successfully.
> 
> In the follow up we will make __kernel_read() with IOCB_NOWAIT work for
> non-sleepable contexts. In addition, I would like to replace the
> secretmem check with a more generic approach and will add fstest for the
> buildid code.
> 
> Reported-by: syzbot+09b7d050e4806540153d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=09b7d050e4806540153d
> Fixes: ad41251c290d ("lib/buildid: implement sleepable build_id_parse() API")

v6.12.

> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>

Thanks, I'll add cc:stable to this due to "crashes the kernel".

Re: [PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Matthew Wilcox]

On Thu, Dec 18, 2025 at 12:55:05PM -0800, Shakeel Butt wrote:
> +	do {
> +		ret = __kernel_read(r->file, buf, sz, &pos);
> +		if (ret <= 0) {
> +			r->err = ret ?: -EIO;
> +			return NULL;
> +		}
> +		buf += ret;
> +		sz -= ret;
> +	} while (sz > 0);

Why are you doing a loop around __kernel_read()?  eg kernel_read() does
not do a read around __kernel_read().  The callers of kernel_read()
don't do a loop either.  So what makes you think it needs to have a loop
around it?

Re: [PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Shakeel Butt]

On Thu, Dec 18, 2025 at 11:55:39PM +0000, Matthew Wilcox wrote:
> On Thu, Dec 18, 2025 at 12:55:05PM -0800, Shakeel Butt wrote:
> > +	do {
> > +		ret = __kernel_read(r->file, buf, sz, &pos);
> > +		if (ret <= 0) {
> > +			r->err = ret ?: -EIO;
> > +			return NULL;
> > +		}
> > +		buf += ret;
> > +		sz -= ret;
> > +	} while (sz > 0);
> 
> Why are you doing a loop around __kernel_read()?  eg kernel_read() does
> not do a read around __kernel_read().  The callers of kernel_read()
> don't do a loop either.  So what makes you think it needs to have a loop
> around it?

I am assuming that __kernel_read() can return less data than the
requested. Is that assumption incorrect?

Re: [PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Matthew Wilcox]

On Thu, Dec 18, 2025 at 04:16:40PM -0800, Shakeel Butt wrote:
> On Thu, Dec 18, 2025 at 11:55:39PM +0000, Matthew Wilcox wrote:
> > On Thu, Dec 18, 2025 at 12:55:05PM -0800, Shakeel Butt wrote:
> > > +	do {
> > > +		ret = __kernel_read(r->file, buf, sz, &pos);
> > > +		if (ret <= 0) {
> > > +			r->err = ret ?: -EIO;
> > > +			return NULL;
> > > +		}
> > > +		buf += ret;
> > > +		sz -= ret;
> > > +	} while (sz > 0);
> > 
> > Why are you doing a loop around __kernel_read()?  eg kernel_read() does
> > not do a read around __kernel_read().  The callers of kernel_read()
> > don't do a loop either.  So what makes you think it needs to have a loop
> > around it?
> 
> I am assuming that __kernel_read() can return less data than the
> requested. Is that assumption incorrect?

I think it can, but I don't think a second call will get any more data.
For example, it could hit EOF.  What led you to think that calling it in
a loop was the right approach?

Re: [PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Shakeel Butt]

On Fri, Dec 19, 2025 at 04:07:51AM +0000, Matthew Wilcox wrote:
> On Thu, Dec 18, 2025 at 04:16:40PM -0800, Shakeel Butt wrote:
> > On Thu, Dec 18, 2025 at 11:55:39PM +0000, Matthew Wilcox wrote:
> > > On Thu, Dec 18, 2025 at 12:55:05PM -0800, Shakeel Butt wrote:
> > > > +	do {
> > > > +		ret = __kernel_read(r->file, buf, sz, &pos);
> > > > +		if (ret <= 0) {
> > > > +			r->err = ret ?: -EIO;
> > > > +			return NULL;
> > > > +		}
> > > > +		buf += ret;
> > > > +		sz -= ret;
> > > > +	} while (sz > 0);
> > > 
> > > Why are you doing a loop around __kernel_read()?  eg kernel_read() does
> > > not do a read around __kernel_read().  The callers of kernel_read()
> > > don't do a loop either.  So what makes you think it needs to have a loop
> > > around it?
> > 
> > I am assuming that __kernel_read() can return less data than the
> > requested. Is that assumption incorrect?
> 
> I think it can, but I don't think a second call will get any more data.
> For example, it could hit EOF.  What led you to think that calling it in
> a loop was the right approach?

I am kind of following the convention of a userspace application doing
read() syscall i.e. repeatedly call read() until you hit an error or EOF
in which case 0 will be returned or you successfully read the amount of
data you want. I am handling negative error and 0 and for 0, I am
returning -EIO as that would be unexpected end of an ELF file.

Anyways the question is if __kernel_read() returns less amount of data
than requested, should we return error instead of retrying? I looked
couple of callers of __kernel_read() & kernel_read(). Some are erroring
out if received data is less than requested (e.g. big_key_read()) and
some are calling in the loop (e.g. kernel_read_file()).

Re: [PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Andrii Nakryiko]

On Thu, Dec 18, 2025 at 9:59 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> On Fri, Dec 19, 2025 at 04:07:51AM +0000, Matthew Wilcox wrote:
> > On Thu, Dec 18, 2025 at 04:16:40PM -0800, Shakeel Butt wrote:
> > > On Thu, Dec 18, 2025 at 11:55:39PM +0000, Matthew Wilcox wrote:
> > > > On Thu, Dec 18, 2025 at 12:55:05PM -0800, Shakeel Butt wrote:
> > > > > +       do {
> > > > > +               ret = __kernel_read(r->file, buf, sz, &pos);
> > > > > +               if (ret <= 0) {
> > > > > +                       r->err = ret ?: -EIO;
> > > > > +                       return NULL;
> > > > > +               }
> > > > > +               buf += ret;
> > > > > +               sz -= ret;
> > > > > +       } while (sz > 0);
> > > >
> > > > Why are you doing a loop around __kernel_read()?  eg kernel_read() does
> > > > not do a read around __kernel_read().  The callers of kernel_read()
> > > > don't do a loop either.  So what makes you think it needs to have a loop
> > > > around it?
> > >
> > > I am assuming that __kernel_read() can return less data than the
> > > requested. Is that assumption incorrect?
> >
> > I think it can, but I don't think a second call will get any more data.
> > For example, it could hit EOF.  What led you to think that calling it in
> > a loop was the right approach?
>
> I am kind of following the convention of a userspace application doing
> read() syscall i.e. repeatedly call read() until you hit an error or EOF
> in which case 0 will be returned or you successfully read the amount of
> data you want. I am handling negative error and 0 and for 0, I am
> returning -EIO as that would be unexpected end of an ELF file.
>
> Anyways the question is if __kernel_read() returns less amount of data
> than requested, should we return error instead of retrying? I looked
> couple of callers of __kernel_read() & kernel_read(). Some are erroring
> out if received data is less than requested (e.g. big_key_read()) and
> some are calling in the loop (e.g. kernel_read_file()).

From a user perspective, I'd very much appreciate it if I get exactly
the requested amount of bytes from freader_fetch_sync(), so yeah,
let's please keep the loop. It does seem that ret <= 0 handling is
correct and should not result in an endless loop.

Re: [PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Matthew Wilcox]

On Thu, Dec 18, 2025 at 09:58:43PM -0800, Shakeel Butt wrote:
> On Fri, Dec 19, 2025 at 04:07:51AM +0000, Matthew Wilcox wrote:
> > > I am assuming that __kernel_read() can return less data than the
> > > requested. Is that assumption incorrect?
> > 
> > I think it can, but I don't think a second call will get any more data.
> > For example, it could hit EOF.  What led you to think that calling it in
> > a loop was the right approach?
> 
> I am kind of following the convention of a userspace application doing
> read() syscall i.e. repeatedly call read() until you hit an error or EOF
> in which case 0 will be returned or you successfully read the amount of
> data you want. I am handling negative error and 0 and for 0, I am
> returning -EIO as that would be unexpected end of an ELF file.

Oh, you sweet summer child.  I hope Father Christmas leaves you an
extra special present in your stocking this week!

While it would be lovely to believe that userspace does that kind of loop,
it just doesn't.  That's why mounting NFS filesystems with the "intr"
option (before I made it a no-op) was dangerous -- userspace just isn't
prepared for short reads.  I mean, we're lucky if userspace even checks
for errors, let alone does this kind of advanced "oh, we got fewer bytes
than we wanted, keep trying" scheme.

A filesystem's ->read_iter() implementation can stop short of reading
all bytes requested if:

 - We hit EIO.  No amount of asking will return more bytes, the data's
   just not there any more.
 - We hit EOF.  There's no more data to read.
 - We're unable to access the buffer.  That's only possible for user
   buffers, not kernel buffers.
 - We receive a fatal signal.  I suppose there is the tiniest chance
   that the I/O completes while we're processing the "we returned early"
   loop, but in practice, the user has asked that we die now, and even
   trying again is rude.  Just die as quickly as we can.

I can't think of any other cases.  It's just not allowed to return
short reads to userspace (other than EIO/EOF), and that drives all
the behaviour.

> Anyways the question is if __kernel_read() returns less amount of data
> than requested, should we return error instead of retrying? I looked
> couple of callers of __kernel_read() & kernel_read(). Some are erroring
> out if received data is less than requested (e.g. big_key_read()) and
> some are calling in the loop (e.g. kernel_read_file()).

kernel_read_file() is wrong.  Thanks for reporting it; I'll send a patch.

Re: [PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Matthew Wilcox]

On Fri, Dec 19, 2025 at 09:42:13AM -0800, Andrii Nakryiko wrote:
> From a user perspective, I'd very much appreciate it if I get exactly
> the requested amount of bytes from freader_fetch_sync(), so yeah,
> let's please keep the loop. It does seem that ret <= 0 handling is
> correct and should not result in an endless loop.

No, you don't understand.  If __kernel_read() doesn't get all the data in
one call, it's not there to get.  If a partial amount of data is useful
(I suspect not), we can return it, otherwise an error is the way to go.
The loop just betrays a lack of understanding of the VFS.

Re: [PATCH bpf v2] lib/buildid: use __kernel_read() for sleepable context

[Shakeel Butt]

On Mon, Dec 22, 2025 at 05:31:50AM +0000, Matthew Wilcox wrote:
> On Thu, Dec 18, 2025 at 09:58:43PM -0800, Shakeel Butt wrote:
> > On Fri, Dec 19, 2025 at 04:07:51AM +0000, Matthew Wilcox wrote:
> > > > I am assuming that __kernel_read() can return less data than the
> > > > requested. Is that assumption incorrect?
> > > 
> > > I think it can, but I don't think a second call will get any more data.
> > > For example, it could hit EOF.  What led you to think that calling it in
> > > a loop was the right approach?
> > 
> > I am kind of following the convention of a userspace application doing
> > read() syscall i.e. repeatedly call read() until you hit an error or EOF
> > in which case 0 will be returned or you successfully read the amount of
> > data you want. I am handling negative error and 0 and for 0, I am
> > returning -EIO as that would be unexpected end of an ELF file.
> 
> Oh, you sweet summer child.  I hope Father Christmas leaves you an
> extra special present in your stocking this week!
> 
> While it would be lovely to believe that userspace does that kind of loop,
> it just doesn't.  That's why mounting NFS filesystems with the "intr"
> option (before I made it a no-op) was dangerous -- userspace just isn't
> prepared for short reads.  I mean, we're lucky if userspace even checks
> for errors, let alone does this kind of advanced "oh, we got fewer bytes
> than we wanted, keep trying" scheme.
> 
> A filesystem's ->read_iter() implementation can stop short of reading
> all bytes requested if:
> 
>  - We hit EIO.  No amount of asking will return more bytes, the data's
>    just not there any more.
>  - We hit EOF.  There's no more data to read.
>  - We're unable to access the buffer.  That's only possible for user
>    buffers, not kernel buffers.
>  - We receive a fatal signal.  I suppose there is the tiniest chance
>    that the I/O completes while we're processing the "we returned early"
>    loop, but in practice, the user has asked that we die now, and even
>    trying again is rude.  Just die as quickly as we can.
> 
> I can't think of any other cases.  It's just not allowed to return
> short reads to userspace (other than EIO/EOF), and that drives all
> the behaviour.

Thanks for the explanation. Is calling kernel_read() again after it
returned less amount of data unsafe or unnecessary?

> 
> > Anyways the question is if __kernel_read() returns less amount of data
> > than requested, should we return error instead of retrying? I looked
> > couple of callers of __kernel_read() & kernel_read(). Some are erroring
> > out if received data is less than requested (e.g. big_key_read()) and
> > some are calling in the loop (e.g. kernel_read_file()).
> 
> kernel_read_file() is wrong.  Thanks for reporting it; I'll send a patch.

There are couple more like do_read() and do_verify() in
drivers/usb/gadget/function/f_mass_storage.c. There might be more but I
have not looked into every caller.