On Mar 17, 2023, at 2:04 PM, Jeff Layton [off-list ref] wrote:

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

quoted

quoted

On Mar 17, 2023, at 1:13 PM, Jeff Layton [off-list ref] wrote:

If rq_next_page ends up pointing outside the array, then we can
corrupt
memory when we go to change its value. Ensure that it hasn't strayed
outside the array, and have svc_rqst_replace_page return -EIO
without
changing anything if it has.

Fix up nfsd_splice_actor (the only caller) to handle this case by
either
returning an error or a short splice when this happens.

IMO it's not worth the extra complexity to return a short splice.
This is a "should never happen" scenario in a hot I/O path. Let's
keep this code as simple as possible, and use unlikely() for the
error cases in both nfsd_splice_actor and svc_rqst_replace_page().

Are there any issues with just returning an error even though we have
successfully spliced some of the data into the buffer? I guess the
caller will just see an EIO or whatever instead of the short read in
that case?

NFSv4 READ is probably going to truncate the XDR buffer. I'm not
sure NFSv3 is so clever, so you should test it.

quoted

Also, since "nfsd_splice_actor ... [is] the only caller", a WARN_ON
stack trace is not adding value. I still think a tracepoint is more
appropriate. I suggest:

  trace_svc_replace_page_err(rqst);

Ok, I can look at adding a conditional tracepoint.

I thought about that: it doesn't help much, since you have to
explicitly test anyway to see whether to return an error.

quoted

quoted

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
fs/nfsd/vfs.c              | 23 +++++++++++++++++------
include/linux/sunrpc/svc.h |  2 +-
net/sunrpc/svc.c           | 14 +++++++++++++-
3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 97b38b47c563..0ebd7a65a9f0 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c

@@ -939,6 +939,7 @@ nfsd_splice_actor(struct pipe_inode_info *pipe,

struct pipe_buffer *buf,
	struct page *page = buf->page;	// may be a compound one
	unsigned offset = buf->offset;
	struct page *last_page;
+	int ret = 0, consumed = 0;

	last_page = page + (offset + sd->len - 1) / PAGE_SIZE;
	for (page += offset / PAGE_SIZE; page <= last_page; page++)
{

@@ -946,13 +947,23 @@ nfsd_splice_actor(struct pipe_inode_info

*pipe, struct pipe_buffer *buf,
		 * Skip page replacement when extending the
contents
		 * of the current page.
		 */
-		if (page != *(rqstp->rq_next_page - 1))
-			svc_rqst_replace_page(rqstp, page);
+		if (page != *(rqstp->rq_next_page - 1)) {
+			ret = svc_rqst_replace_page(rqstp, page);
+			if (ret)
+				break;
+		}
+		consumed += min_t(int,
+				  PAGE_SIZE -
offset_in_page(offset),
+				  sd->len - consumed);
+		offset = 0;
	}
-	if (rqstp->rq_res.page_len == 0)	// first call
-		rqstp->rq_res.page_base = offset % PAGE_SIZE;
-	rqstp->rq_res.page_len += sd->len;
-	return sd->len;
+	if (consumed) {
+		if (rqstp->rq_res.page_len == 0)	// first
call
+			rqstp->rq_res.page_base = offset %
PAGE_SIZE;
+		rqstp->rq_res.page_len += consumed;
+		return consumed;
+	}
+	return ret;
}

static int nfsd_direct_splice_actor(struct pipe_inode_info *pipe,

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 877891536c2f..9ea52f143f49 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h

@@ -422,7 +422,7 @@ struct svc_serv *svc_create(struct svc_program

*, unsigned int,
			    int (*threadfn)(void *data));
struct svc_rqst *svc_rqst_alloc(struct svc_serv *serv,
					struct svc_pool *pool, int
node);
-void		   svc_rqst_replace_page(struct svc_rqst *rqstp,
+int		   svc_rqst_replace_page(struct svc_rqst *rqstp,
					 struct page *page);
void		   svc_rqst_free(struct svc_rqst *);
void		   svc_exit_thread(struct svc_rqst *);

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

@@ -843,8 +843,19 @@ EXPORT_SYMBOL_GPL(svc_set_num_threads);
 * When replacing a page in rq_pages, batch the release of the
 * replaced pages to avoid hammering the page allocator.
 */
-void svc_rqst_replace_page(struct svc_rqst *rqstp, struct page

*page)
+int 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];
+	if (WARN_ON_ONCE(rqstp->rq_next_page < begin || rqstp-

quoted

rq_next_page > end))

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

@@ -853,6 +864,7 @@ void svc_rqst_replace_page(struct svc_rqst

*rqstp, struct page *page)

	get_page(page);
	*(rqstp->rq_next_page++) = page;
+	return 0;
}
EXPORT_SYMBOL_GPL(svc_rqst_replace_page);

-- 
2.39.2

--
Chuck Lever

-- 
Jeff Layton [off-list ref]

--
Chuck Lever