lib/buildid: use __kernel_read() for sleepable context

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 [email protected]

Closes https://syzkaller.appspot.com/bug?extid=09b7d050e4806540153d

Fixes ad41251c290d ("lib/buildid: implement sleepable build_id_parse() API")

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Shakeel Butt <[email protected]>

lib/buildid.c static void freader_put_folio(struct freader *r) · line 59

Quoted 9 lines · Shakeel Butt

lib/buildid.c +9 -0

+	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?

Quoted 15 lines · Matthew Wilcox

lib/buildid.c static void freader_put_folio(struct freader *r) · line 59

Quoted 9 lines · Shakeel Butt

lib/buildid.c +9 -0

+	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?

Quoted 17 lines · Shakeel Butt

Quoted 15 lines · Matthew Wilcox

lib/buildid.c static void freader_put_folio(struct freader *r) · line 59

Quoted 9 lines · Shakeel Butt

lib/buildid.c +9 -0

+	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?

Quoted 2 lines

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?

Quoted 18 lines · Matthew Wilcox

Quoted 17 lines · Shakeel Butt

Quoted 15 lines · Matthew Wilcox

lib/buildid.c static void freader_put_folio(struct freader *r) · line 59

Quoted 9 lines · Shakeel Butt

lib/buildid.c +9 -0

+       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?

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines

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()).

Quoted 8 lines · Shakeel Butt

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines

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?

Quoted 5 lines

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.

Quoted 6 lines

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.