On Mar 17, 2023, at 9:52 AM, Jeff Layton [off-list ref] wrote:

On Fri, 2023-03-17 at 13:44 +0000, Chuck Lever III wrote:

quoted

quoted

On Mar 17, 2023, at 6:56 AM, Jeff Layton [off-list ref] wrote:

There's no good way to handle this gracefully, but if rq_next_page ends
up pointing outside the array, we can at least crash the box before it
scribbles over too much else.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
net/sunrpc/svc.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index fea7ce8fba14..864e62945647 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c

@@ -845,6 +845,16 @@ EXPORT_SYMBOL_GPL(svc_set_num_threads);

*/
void svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page)
{
+	struct page **begin, **end;
+
+	/*
+	 * Bounds check: make sure rq_next_page points into the rq_respages
+	 * part of the array.
+	 */
+	begin = rqstp->rq_pages;
+	end = &rqstp->rq_pages[RPCSVC_MAXPAGES];
+	BUG_ON(rqstp->rq_next_page < begin || rqstp->rq_next_page > end);

Linus has stated clearly that he does not want BUG_ON assertions
if the system is not actually in danger... and this is clearly
the result of a software bug, so a crash will occur anyway.

It'll crash, but only after we scribble over some memory.

Actually, it looks like the splice actor can return an error. We could
return -EIO here or something without doing anything if we hit this case
and then let that bubble back up to the read?

Yes, if it's possible to fail just the READ operation, that
would be best. Maybe a emitting a trace event would be better
than a pr_warn.

quoted

Can you make this a pr_warn_once() ?

quoted

+
	if (*rqstp->rq_next_page) {
		if (!pagevec_space(&rqstp->rq_pvec))
			__pagevec_release(&rqstp->rq_pvec);
-- 
2.39.2

--
Chuck Lever

-- 
Jeff Layton [off-list ref]

--
Chuck Lever