[PATCH v2 0/9] Fix various issues in the SUNRPC xdr code

[PATCH v2 0/9] Fix various issues in the SUNRPC xdr code

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

When looking at the issues raised by Tigran's testing of the NFS client
updates, I noticed a couple of things in the generic SUNRPC xdr code
that want to be fixed. This patch series replaces an earlier series that
attempted to just fix the XDR padding in the NFS code.

This series fixes up a number of issues w.r.t. bounds checking in the
xdr_stream code. It corrects the behaviour of xdr_read_pages() for the
case where the XDR object size is larger than the buffer page array
length and simplifies the code.

v2:
 - Clean up the handling of page padding in rpc_prepare_reply_pages()
 - Fix up the initial read_plus() page alignment

Trond Myklebust (9):
  NFSv4: Fix the alignment of page data in the getdeviceinfo reply
  SUNRPC: Fix up typo in xdr_init_decode()
  SUNRPC: Clean up helpers xdr_set_iov() and xdr_set_page_base()
  SUNRPC: Fix up xdr_read_pages() to take arbitrary object lengths
  SUNRPC: Clean up the handling of page padding in
    rpc_prepare_reply_pages()
  SUNRPC: Fix up xdr_set_page()
  SUNRPC: Fix open coded xdr_stream_remaining()
  NFSv4: Fix open coded xdr_stream_remaining()
  NFSv4.2: Fix up read_plus() page alignment

 fs/nfs/nfs2xdr.c  |  19 ++++-----
 fs/nfs/nfs3xdr.c  |  29 +++++++------
 fs/nfs/nfs42xdr.c |   4 +-
 fs/nfs/nfs4xdr.c  |  48 ++++++++++++----------
 net/sunrpc/clnt.c |   5 +--
 net/sunrpc/xdr.c  | 101 +++++++++++++++++++++-------------------------
 6 files changed, 103 insertions(+), 103 deletions(-)

-- 
2.28.0

[PATCH v2 1/9] NFSv4: Fix the alignment of page data in the getdeviceinfo reply

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

We can fit the device_addr4 opaque data padding in the pages.

Fixes: cf500bac8fd4 ("SUNRPC: Introduce rpc_prepare_reply_pages()")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs4xdr.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index c6dbfcae7517..c16b93df1bc1 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3009,15 +3009,19 @@ static void nfs4_xdr_enc_getdeviceinfo(struct rpc_rqst *req,
 	struct compound_hdr hdr = {
 		.minorversion = nfs4_xdr_minorversion(&args->seq_args),
 	};
+	uint32_t replen;
 
 	encode_compound_hdr(xdr, req, &hdr);
 	encode_sequence(xdr, &args->seq_args, &hdr);
+
+	replen = hdr.replen + op_decode_hdr_maxsz;
+
 	encode_getdeviceinfo(xdr, args, &hdr);
 
-	/* set up reply kvec. Subtract notification bitmap max size (2)
-	 * so that notification bitmap is put in xdr_buf tail */
+	/* set up reply kvec. device_addr4 opaque data is read into the
+	 * pages */
 	rpc_prepare_reply_pages(req, args->pdev->pages, args->pdev->pgbase,
-				args->pdev->pglen, hdr.replen - 2);
+				args->pdev->pglen, replen + 2 + 1);
 	encode_nops(&hdr);
 }
 
-- 
2.28.0

[PATCH v2 2/9] SUNRPC: Fix up typo in xdr_init_decode()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

We already know that the head buffer and page are empty, so if there is
any data, it is in the tail.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index b1cda6d85ded..bc7a622016ee 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1060,7 +1060,7 @@ void xdr_init_decode(struct xdr_stream *xdr, struct xdr_buf *buf, __be32 *p,
 	else if (buf->page_len != 0)
 		xdr_set_page_base(xdr, 0, buf->len);
 	else
-		xdr_set_iov(xdr, buf->head, buf->len);
+		xdr_set_iov(xdr, buf->tail, buf->len);
 	if (p != NULL && p > xdr->p && xdr->end >= p) {
 		xdr->nwords -= p - xdr->p;
 		xdr->p = p;
-- 
2.28.0

[PATCH v2 3/9] SUNRPC: Clean up helpers xdr_set_iov() and xdr_set_page_base()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Allow xdr_set_iov() to set a base so that we can use it to set the
cursor to a specific position in the kvec buffer.

If the new base overflows the kvec/pages buffer in either xdr_set_iov()
or xdr_set_page_base(), then truncate it so that we point to the end of
the buffer.

Finally, change both function to return the number of bytes remaining to
read in their buffers.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xdr.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index bc7a622016ee..394297ec1cb9 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -970,19 +970,22 @@ void xdr_write_pages(struct xdr_stream *xdr, struct page **pages, unsigned int b
 }
 EXPORT_SYMBOL_GPL(xdr_write_pages);
 
-static void xdr_set_iov(struct xdr_stream *xdr, struct kvec *iov,
-		unsigned int len)
+static unsigned int xdr_set_iov(struct xdr_stream *xdr, struct kvec *iov,
+				unsigned int base, unsigned int len)
 {
 	if (len > iov->iov_len)
 		len = iov->iov_len;
-	xdr->p = (__be32*)iov->iov_base;
+	if (unlikely(base > len))
+		base = len;
+	xdr->p = (__be32*)(iov->iov_base + base);
 	xdr->end = (__be32*)(iov->iov_base + len);
 	xdr->iov = iov;
 	xdr->page_ptr = NULL;
+	return len - base;
 }
 
-static int xdr_set_page_base(struct xdr_stream *xdr,
-		unsigned int base, unsigned int len)
+static unsigned int xdr_set_page_base(struct xdr_stream *xdr,
+				      unsigned int base, unsigned int len)
 {
 	unsigned int pgnr;
 	unsigned int maxlen;
@@ -991,9 +994,11 @@ static int xdr_set_page_base(struct xdr_stream *xdr,
 	void *kaddr;
 
 	maxlen = xdr->buf->page_len;
-	if (base >= maxlen)
-		return -EINVAL;
-	maxlen -= base;
+	if (base >= maxlen) {
+		base = maxlen;
+		maxlen = 0;
+	} else
+		maxlen -= base;
 	if (len > maxlen)
 		len = maxlen;
 
@@ -1011,14 +1016,14 @@ static int xdr_set_page_base(struct xdr_stream *xdr,
 		pgend = PAGE_SIZE;
 	xdr->end = (__be32*)(kaddr + pgend);
 	xdr->iov = NULL;
-	return 0;
+	return len;
 }
 
 static void xdr_set_page(struct xdr_stream *xdr, unsigned int base,
 			 unsigned int len)
 {
-	if (xdr_set_page_base(xdr, base, len) < 0)
-		xdr_set_iov(xdr, xdr->buf->tail, xdr->nwords << 2);
+	if (xdr_set_page_base(xdr, base, len) == 0)
+		xdr_set_iov(xdr, xdr->buf->tail, 0, xdr_stream_remaining(xdr));
 }
 
 static void xdr_set_next_page(struct xdr_stream *xdr)
@@ -1055,12 +1060,9 @@ void xdr_init_decode(struct xdr_stream *xdr, struct xdr_buf *buf, __be32 *p,
 	xdr->scratch.iov_base = NULL;
 	xdr->scratch.iov_len = 0;
 	xdr->nwords = XDR_QUADLEN(buf->len);
-	if (buf->head[0].iov_len != 0)
-		xdr_set_iov(xdr, buf->head, buf->len);
-	else if (buf->page_len != 0)
-		xdr_set_page_base(xdr, 0, buf->len);
-	else
-		xdr_set_iov(xdr, buf->tail, buf->len);
+	if (xdr_set_iov(xdr, buf->head, 0, buf->len) == 0 &&
+	    xdr_set_page_base(xdr, 0, buf->len) == 0)
+		xdr_set_iov(xdr, buf->tail, 0, buf->len);
 	if (p != NULL && p > xdr->p && xdr->end >= p) {
 		xdr->nwords -= p - xdr->p;
 		xdr->p = p;
-- 
2.28.0

[PATCH v2 4/9] SUNRPC: Fix up xdr_read_pages() to take arbitrary object lengths

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Fix up xdr_read_pages() so that it can handle object lengths that are
larger than the page length, by simply aligning to the next object in
the buffer tail.
The function will continue to return the length of the truncate object
data that actually fit into the pages.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xdr.c | 41 +++++++++++++++--------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 394297ec1cb9..3ce0a5daa9eb 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1219,44 +1219,33 @@ static unsigned int xdr_align_pages(struct xdr_stream *xdr, unsigned int len)
 }
 
 /**
- * xdr_read_pages - Ensure page-based XDR data to decode is aligned at current pointer position
+ * xdr_read_pages - align page-based XDR data to current pointer position
  * @xdr: pointer to xdr_stream struct
  * @len: number of bytes of page data
  *
  * Moves data beyond the current pointer position from the XDR head[] buffer
- * into the page list. Any data that lies beyond current position + "len"
- * bytes is moved into the XDR tail[].
+ * into the page list. Any data that lies beyond current position + @len
+ * bytes is moved into the XDR tail[]. The xdr_stream current position is
+ * then advanced past that data to align to the next XDR object in the tail.
  *
  * Returns the number of XDR encoded bytes now contained in the pages
  */
 unsigned int xdr_read_pages(struct xdr_stream *xdr, unsigned int len)
 {
-	struct xdr_buf *buf = xdr->buf;
-	struct kvec *iov;
-	unsigned int nwords;
-	unsigned int end;
-	unsigned int padding;
+	unsigned int nwords = XDR_QUADLEN(len);
+	unsigned int base, end, pglen;
 
-	len = xdr_align_pages(xdr, len);
-	if (len == 0)
+	pglen = xdr_align_pages(xdr, nwords << 2);
+	if (pglen == 0)
 		return 0;
-	nwords = XDR_QUADLEN(len);
-	padding = (nwords << 2) - len;
-	xdr->iov = iov = buf->tail;
-	/* Compute remaining message length.  */
-	end = ((xdr->nwords - nwords) << 2) + padding;
-	if (end > iov->iov_len)
-		end = iov->iov_len;
 
-	/*
-	 * Position current pointer at beginning of tail, and
-	 * set remaining message length.
-	 */
-	xdr->p = (__be32 *)((char *)iov->iov_base + padding);
-	xdr->end = (__be32 *)((char *)iov->iov_base + end);
-	xdr->page_ptr = NULL;
-	xdr->nwords = XDR_QUADLEN(end - padding);
-	return len;
+	xdr->nwords -= nwords;
+	base = (nwords << 2) - pglen;
+	end = xdr_stream_remaining(xdr) - pglen;
+
+	if (xdr_set_iov(xdr, xdr->buf->tail, base, end) == 0)
+		xdr->nwords = 0;
+	return len <= pglen ? len : pglen;
 }
 EXPORT_SYMBOL_GPL(xdr_read_pages);
 
-- 
2.28.0

[PATCH v2 5/9] SUNRPC: Clean up the handling of page padding in rpc_prepare_reply_pages()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

rpc_prepare_reply_pages() currently expects the 'hdrsize' argument to
contain the length of the data that we expect to want placed in the head
kvec plus a count of 1 word of padding that is placed after the page data.
This is very confusing when trying to read the code, and sometimes leads
to callers adding an arbitrary value of '1' just in order to satisfy the
requirement (whether or not the page data actually needs such padding).

This patch aims to clarify the code by changing the 'hdrsize' argument
to remove that 1 word of padding. This means we need to subtract the
padding from all the existing callers.

Fixes: 02ef04e432ba ("NFS: Account for XDR pad of buf->pages")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs2xdr.c  | 19 ++++++++++---------
 fs/nfs/nfs3xdr.c  | 29 ++++++++++++++++-------------
 fs/nfs/nfs4xdr.c  | 36 +++++++++++++++++++-----------------
 net/sunrpc/clnt.c |  5 +----
 net/sunrpc/xdr.c  |  3 ---
 5 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
index f6676af37d5d..7fba7711e6b3 100644
--- a/fs/nfs/nfs2xdr.c
+++ b/fs/nfs/nfs2xdr.c
@@ -34,6 +34,7 @@
  * Declare the space requirements for NFS arguments and replies as
  * number of 32bit-words
  */
+#define NFS_pagepad_sz		(1) /* Page padding */
 #define NFS_fhandle_sz		(8)
 #define NFS_sattr_sz		(8)
 #define NFS_filename_sz		(1+(NFS2_MAXNAMLEN>>2))
@@ -56,11 +57,11 @@
 
 #define NFS_attrstat_sz		(1+NFS_fattr_sz)
 #define NFS_diropres_sz		(1+NFS_fhandle_sz+NFS_fattr_sz)
-#define NFS_readlinkres_sz	(2+1)
-#define NFS_readres_sz		(1+NFS_fattr_sz+1+1)
+#define NFS_readlinkres_sz	(2+NFS_pagepad_sz)
+#define NFS_readres_sz		(1+NFS_fattr_sz+1+NFS_pagepad_sz)
 #define NFS_writeres_sz         (NFS_attrstat_sz)
 #define NFS_stat_sz		(1)
-#define NFS_readdirres_sz	(1+1)
+#define NFS_readdirres_sz	(1+NFS_pagepad_sz)
 #define NFS_statfsres_sz	(1+NFS_info_sz)
 
 static int nfs_stat_to_errno(enum nfs_stat);
@@ -592,8 +593,8 @@ static void nfs2_xdr_enc_readlinkargs(struct rpc_rqst *req,
 	const struct nfs_readlinkargs *args = data;
 
 	encode_fhandle(xdr, args->fh);
-	rpc_prepare_reply_pages(req, args->pages, args->pgbase,
-				args->pglen, NFS_readlinkres_sz);
+	rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->pglen,
+				NFS_readlinkres_sz - NFS_pagepad_sz);
 }
 
 /*
@@ -628,8 +629,8 @@ static void nfs2_xdr_enc_readargs(struct rpc_rqst *req,
 	const struct nfs_pgio_args *args = data;
 
 	encode_readargs(xdr, args);
-	rpc_prepare_reply_pages(req, args->pages, args->pgbase,
-				args->count, NFS_readres_sz);
+	rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->count,
+				NFS_readres_sz - NFS_pagepad_sz);
 	req->rq_rcv_buf.flags |= XDRBUF_READ;
 }
 
@@ -786,8 +787,8 @@ static void nfs2_xdr_enc_readdirargs(struct rpc_rqst *req,
 	const struct nfs_readdirargs *args = data;
 
 	encode_readdirargs(xdr, args);
-	rpc_prepare_reply_pages(req, args->pages, 0,
-				args->count, NFS_readdirres_sz);
+	rpc_prepare_reply_pages(req, args->pages, 0, args->count,
+				NFS_readdirres_sz - NFS_pagepad_sz);
 }
 
 /*
diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
index 69971f6c840d..ca10072644ff 100644
--- a/fs/nfs/nfs3xdr.c
+++ b/fs/nfs/nfs3xdr.c
@@ -33,6 +33,7 @@
  * Declare the space requirements for NFS arguments and replies as
  * number of 32bit-words
  */
+#define NFS3_pagepad_sz		(1) /* Page padding */
 #define NFS3_fhandle_sz		(1+16)
 #define NFS3_fh_sz		(NFS3_fhandle_sz)	/* shorthand */
 #define NFS3_sattr_sz		(15)
@@ -69,13 +70,13 @@
 #define NFS3_removeres_sz	(NFS3_setattrres_sz)
 #define NFS3_lookupres_sz	(1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
 #define NFS3_accessres_sz	(1+NFS3_post_op_attr_sz+1)
-#define NFS3_readlinkres_sz	(1+NFS3_post_op_attr_sz+1+1)
-#define NFS3_readres_sz		(1+NFS3_post_op_attr_sz+3+1)
+#define NFS3_readlinkres_sz	(1+NFS3_post_op_attr_sz+1+NFS3_pagepad_sz)
+#define NFS3_readres_sz		(1+NFS3_post_op_attr_sz+3+NFS3_pagepad_sz)
 #define NFS3_writeres_sz	(1+NFS3_wcc_data_sz+4)
 #define NFS3_createres_sz	(1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
 #define NFS3_renameres_sz	(1+(2 * NFS3_wcc_data_sz))
 #define NFS3_linkres_sz		(1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
-#define NFS3_readdirres_sz	(1+NFS3_post_op_attr_sz+2+1)
+#define NFS3_readdirres_sz	(1+NFS3_post_op_attr_sz+2+NFS3_pagepad_sz)
 #define NFS3_fsstatres_sz	(1+NFS3_post_op_attr_sz+13)
 #define NFS3_fsinfores_sz	(1+NFS3_post_op_attr_sz+12)
 #define NFS3_pathconfres_sz	(1+NFS3_post_op_attr_sz+6)
@@ -85,7 +86,8 @@
 #define ACL3_setaclargs_sz	(NFS3_fh_sz+1+ \
 				XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
 #define ACL3_getaclres_sz	(1+NFS3_post_op_attr_sz+1+ \
-				XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
+				XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+\
+				NFS3_pagepad_sz)
 #define ACL3_setaclres_sz	(1+NFS3_post_op_attr_sz)
 
 static int nfs3_stat_to_errno(enum nfs_stat);
@@ -909,8 +911,8 @@ static void nfs3_xdr_enc_readlink3args(struct rpc_rqst *req,
 	const struct nfs3_readlinkargs *args = data;
 
 	encode_nfs_fh3(xdr, args->fh);
-	rpc_prepare_reply_pages(req, args->pages, args->pgbase,
-				args->pglen, NFS3_readlinkres_sz);
+	rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->pglen,
+				NFS3_readlinkres_sz - NFS3_pagepad_sz);
 }
 
 /*
@@ -939,7 +941,8 @@ static void nfs3_xdr_enc_read3args(struct rpc_rqst *req,
 				   const void *data)
 {
 	const struct nfs_pgio_args *args = data;
-	unsigned int replen = args->replen ? args->replen : NFS3_readres_sz;
+	unsigned int replen = args->replen ? args->replen :
+					     NFS3_readres_sz - NFS3_pagepad_sz;
 
 	encode_read3args(xdr, args);
 	rpc_prepare_reply_pages(req, args->pages, args->pgbase,
@@ -1239,8 +1242,8 @@ static void nfs3_xdr_enc_readdir3args(struct rpc_rqst *req,
 	const struct nfs3_readdirargs *args = data;
 
 	encode_readdir3args(xdr, args);
-	rpc_prepare_reply_pages(req, args->pages, 0,
-				args->count, NFS3_readdirres_sz);
+	rpc_prepare_reply_pages(req, args->pages, 0, args->count,
+				NFS3_readdirres_sz - NFS3_pagepad_sz);
 }
 
 /*
@@ -1281,8 +1284,8 @@ static void nfs3_xdr_enc_readdirplus3args(struct rpc_rqst *req,
 	const struct nfs3_readdirargs *args = data;
 
 	encode_readdirplus3args(xdr, args);
-	rpc_prepare_reply_pages(req, args->pages, 0,
-				args->count, NFS3_readdirres_sz);
+	rpc_prepare_reply_pages(req, args->pages, 0, args->count,
+				NFS3_readdirres_sz - NFS3_pagepad_sz);
 }
 
 /*
@@ -1328,7 +1331,7 @@ static void nfs3_xdr_enc_getacl3args(struct rpc_rqst *req,
 	if (args->mask & (NFS_ACL | NFS_DFACL)) {
 		rpc_prepare_reply_pages(req, args->pages, 0,
 					NFSACL_MAXPAGES << PAGE_SHIFT,
-					ACL3_getaclres_sz);
+					ACL3_getaclres_sz - NFS3_pagepad_sz);
 		req->rq_rcv_buf.flags |= XDRBUF_SPARSE_PAGES;
 	}
 }
@@ -1648,7 +1651,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
 	result->op_status = status;
 	if (status != NFS3_OK)
 		goto out_status;
-	result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
+	result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
 	error = decode_read3resok(xdr, result);
 out:
 	return error;
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index c16b93df1bc1..3899ef3047f4 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -84,6 +84,7 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
 /* lock,open owner id:
  * we currently use size 2 (u64) out of (NFS4_OPAQUE_LIMIT  >> 2)
  */
+#define pagepad_maxsz		(1)
 #define open_owner_id_maxsz	(1 + 2 + 1 + 1 + 2)
 #define lock_owner_id_maxsz	(1 + 1 + 4)
 #define decode_lockowner_maxsz	(1 + XDR_QUADLEN(IDMAP_NAMESZ))
@@ -215,14 +216,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
 				 nfs4_fattr_bitmap_maxsz)
 #define encode_read_maxsz	(op_encode_hdr_maxsz + \
 				 encode_stateid_maxsz + 3)
-#define decode_read_maxsz	(op_decode_hdr_maxsz + 2 + 1)
+#define decode_read_maxsz	(op_decode_hdr_maxsz + 2 + pagepad_maxsz)
 #define encode_readdir_maxsz	(op_encode_hdr_maxsz + \
 				 2 + encode_verifier_maxsz + 5 + \
 				nfs4_label_maxsz)
 #define decode_readdir_maxsz	(op_decode_hdr_maxsz + \
-				 decode_verifier_maxsz + 1)
+				 decode_verifier_maxsz + pagepad_maxsz)
 #define encode_readlink_maxsz	(op_encode_hdr_maxsz)
-#define decode_readlink_maxsz	(op_decode_hdr_maxsz + 1 + 1)
+#define decode_readlink_maxsz	(op_decode_hdr_maxsz + 1 + pagepad_maxsz)
 #define encode_write_maxsz	(op_encode_hdr_maxsz + \
 				 encode_stateid_maxsz + 4)
 #define decode_write_maxsz	(op_decode_hdr_maxsz + \
@@ -284,14 +285,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
 #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
 #define encode_getacl_maxsz	(encode_getattr_maxsz)
 #define decode_getacl_maxsz	(op_decode_hdr_maxsz + \
-				 nfs4_fattr_bitmap_maxsz + 1 + 1)
+				 nfs4_fattr_bitmap_maxsz + 1 + pagepad_maxsz)
 #define encode_setacl_maxsz	(op_encode_hdr_maxsz + \
 				 encode_stateid_maxsz + 3)
 #define decode_setacl_maxsz	(decode_setattr_maxsz)
 #define encode_fs_locations_maxsz \
 				(encode_getattr_maxsz)
 #define decode_fs_locations_maxsz \
-				(1)
+				(pagepad_maxsz)
 #define encode_secinfo_maxsz	(op_encode_hdr_maxsz + nfs4_name_maxsz)
 #define decode_secinfo_maxsz	(op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
 
@@ -393,12 +394,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
 				  /* devaddr4 payload is read into page */ \
 				1 /* notification bitmap length */ + \
 				1 /* notification bitmap, word 0 */ + \
-				1 /* possible XDR padding */)
+				pagepad_maxsz /* possible XDR padding */)
 #define encode_layoutget_maxsz	(op_encode_hdr_maxsz + 10 + \
 				encode_stateid_maxsz)
 #define decode_layoutget_maxsz	(op_decode_hdr_maxsz + 8 + \
 				decode_stateid_maxsz + \
-				XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
+				XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + \
+				pagepad_maxsz)
 #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
 				2 /* offset */ + \
 				2 /* length */ + \
@@ -2342,7 +2344,7 @@ static void nfs4_xdr_enc_open(struct rpc_rqst *req, struct xdr_stream *xdr,
 		encode_layoutget(xdr, args->lg_args, &hdr);
 		rpc_prepare_reply_pages(req, args->lg_args->layout.pages, 0,
 					args->lg_args->layout.pglen,
-					hdr.replen);
+					hdr.replen - pagepad_maxsz);
 	}
 	encode_nops(&hdr);
 }
@@ -2388,7 +2390,7 @@ static void nfs4_xdr_enc_open_noattr(struct rpc_rqst *req,
 		encode_layoutget(xdr, args->lg_args, &hdr);
 		rpc_prepare_reply_pages(req, args->lg_args->layout.pages, 0,
 					args->lg_args->layout.pglen,
-					hdr.replen);
+					hdr.replen - pagepad_maxsz);
 	}
 	encode_nops(&hdr);
 }
@@ -2499,7 +2501,7 @@ static void nfs4_xdr_enc_readlink(struct rpc_rqst *req, struct xdr_stream *xdr,
 	encode_readlink(xdr, args, req, &hdr);
 
 	rpc_prepare_reply_pages(req, args->pages, args->pgbase,
-				args->pglen, hdr.replen);
+				args->pglen, hdr.replen - pagepad_maxsz);
 	encode_nops(&hdr);
 }
 
@@ -2520,7 +2522,7 @@ static void nfs4_xdr_enc_readdir(struct rpc_rqst *req, struct xdr_stream *xdr,
 	encode_readdir(xdr, args, req, &hdr);
 
 	rpc_prepare_reply_pages(req, args->pages, args->pgbase,
-				args->count, hdr.replen);
+				args->count, hdr.replen - pagepad_maxsz);
 	encode_nops(&hdr);
 }
 
@@ -2541,7 +2543,7 @@ static void nfs4_xdr_enc_read(struct rpc_rqst *req, struct xdr_stream *xdr,
 	encode_read(xdr, args, &hdr);
 
 	rpc_prepare_reply_pages(req, args->pages, args->pgbase,
-				args->count, hdr.replen);
+				args->count, hdr.replen - pagepad_maxsz);
 	req->rq_rcv_buf.flags |= XDRBUF_READ;
 	encode_nops(&hdr);
 }
@@ -2588,7 +2590,7 @@ static void nfs4_xdr_enc_getacl(struct rpc_rqst *req, struct xdr_stream *xdr,
 			ARRAY_SIZE(nfs4_acl_bitmap), &hdr);
 
 	rpc_prepare_reply_pages(req, args->acl_pages, 0,
-				args->acl_len, replen + 1);
+				args->acl_len, replen);
 	encode_nops(&hdr);
 }
 
@@ -2810,7 +2812,7 @@ static void nfs4_xdr_enc_fs_locations(struct rpc_rqst *req,
 	}
 
 	rpc_prepare_reply_pages(req, (struct page **)&args->page, 0,
-				PAGE_SIZE, replen + 1);
+				PAGE_SIZE, replen);
 	encode_nops(&hdr);
 }
 
@@ -3014,14 +3016,14 @@ static void nfs4_xdr_enc_getdeviceinfo(struct rpc_rqst *req,
 	encode_compound_hdr(xdr, req, &hdr);
 	encode_sequence(xdr, &args->seq_args, &hdr);
 
-	replen = hdr.replen + op_decode_hdr_maxsz;
+	replen = hdr.replen + op_decode_hdr_maxsz + 2;
 
 	encode_getdeviceinfo(xdr, args, &hdr);
 
 	/* set up reply kvec. device_addr4 opaque data is read into the
 	 * pages */
 	rpc_prepare_reply_pages(req, args->pdev->pages, args->pdev->pgbase,
-				args->pdev->pglen, replen + 2 + 1);
+				args->pdev->pglen, replen);
 	encode_nops(&hdr);
 }
 
@@ -3043,7 +3045,7 @@ static void nfs4_xdr_enc_layoutget(struct rpc_rqst *req,
 	encode_layoutget(xdr, args, &hdr);
 
 	rpc_prepare_reply_pages(req, args->layout.pages, 0,
-				args->layout.pglen, hdr.replen);
+				args->layout.pglen, hdr.replen - pagepad_maxsz);
 	encode_nops(&hdr);
 }
 
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 3259120462ed..612f0a641f4c 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1251,10 +1251,7 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
 			     unsigned int base, unsigned int len,
 			     unsigned int hdrsize)
 {
-	/* Subtract one to force an extra word of buffer space for the
-	 * payload's XDR pad to fall into the rcv_buf's tail iovec.
-	 */
-	hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_ralign - 1;
+	hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_ralign;
 
 	xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
 	trace_rpc_xdr_reply_pages(req->rq_task, &req->rq_rcv_buf);
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 3ce0a5daa9eb..5a450055469f 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -193,9 +193,6 @@ xdr_inline_pages(struct xdr_buf *xdr, unsigned int offset,
 
 	tail->iov_base = buf + offset;
 	tail->iov_len = buflen - offset;
-	if ((xdr->page_len & 3) == 0)
-		tail->iov_len -= sizeof(__be32);
-
 	xdr->buflen += len;
 }
 EXPORT_SYMBOL_GPL(xdr_inline_pages);
-- 
2.28.0

[PATCH v2 6/9] SUNRPC: Fix up xdr_set_page()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

While we always want to align to the next page and/or the beginning of
the tail buffer when we call xdr_set_next_page(), the functions
xdr_align_data() and xdr_expand_hole() really want to align to the next
object in that next page or tail.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xdr.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 5a450055469f..ddd5cc2281ab 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1019,8 +1019,10 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr,
 static void xdr_set_page(struct xdr_stream *xdr, unsigned int base,
 			 unsigned int len)
 {
-	if (xdr_set_page_base(xdr, base, len) == 0)
-		xdr_set_iov(xdr, xdr->buf->tail, 0, xdr_stream_remaining(xdr));
+	if (xdr_set_page_base(xdr, base, len) == 0) {
+		base -= xdr->buf->page_len;
+		xdr_set_iov(xdr, xdr->buf->tail, base, len);
+	}
 }
 
 static void xdr_set_next_page(struct xdr_stream *xdr)
@@ -1029,17 +1031,18 @@ static void xdr_set_next_page(struct xdr_stream *xdr)
 
 	newbase = (1 + xdr->page_ptr - xdr->buf->pages) << PAGE_SHIFT;
 	newbase -= xdr->buf->page_base;
-
-	xdr_set_page(xdr, newbase, PAGE_SIZE);
+	if (newbase < xdr->buf->page_len)
+		xdr_set_page_base(xdr, newbase, xdr_stream_remaining(xdr));
+	else
+		xdr_set_iov(xdr, xdr->buf->tail, 0, xdr_stream_remaining(xdr));
 }
 
 static bool xdr_set_next_buffer(struct xdr_stream *xdr)
 {
 	if (xdr->page_ptr != NULL)
 		xdr_set_next_page(xdr);
-	else if (xdr->iov == xdr->buf->head) {
-		xdr_set_page(xdr, 0, PAGE_SIZE);
-	}
+	else if (xdr->iov == xdr->buf->head)
+		xdr_set_page(xdr, 0, xdr_stream_remaining(xdr));
 	return xdr->p != xdr->end;
 }
 
@@ -1277,7 +1280,7 @@ uint64_t xdr_align_data(struct xdr_stream *xdr, uint64_t offset, uint32_t length
 	}
 
 	xdr->nwords -= XDR_QUADLEN(length);
-	xdr_set_page(xdr, from + length, PAGE_SIZE);
+	xdr_set_page(xdr, from + length, xdr_stream_remaining(xdr));
 	return length;
 }
 EXPORT_SYMBOL_GPL(xdr_align_data);
@@ -1314,7 +1317,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
 	_zero_pages(buf->pages, buf->page_base + offset, length);
 
 	buf->len += length - (from - offset) - truncated;
-	xdr_set_page(xdr, offset + length, PAGE_SIZE);
+	xdr_set_page(xdr, offset + length, xdr_stream_remaining(xdr));
 	return length;
 }
 EXPORT_SYMBOL_GPL(xdr_expand_hole);
-- 
2.28.0

[PATCH v2 7/9] SUNRPC: Fix open coded xdr_stream_remaining()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xdr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index ddd5cc2281ab..c852d199c789 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1261,7 +1261,7 @@ uint64_t xdr_align_data(struct xdr_stream *xdr, uint64_t offset, uint32_t length
 
 	xdr_realign_pages(xdr);
 	from = xdr_page_pos(xdr);
-	bytes = xdr->nwords << 2;
+	bytes = xdr_stream_remaining(xdr);
 	if (length < bytes)
 		bytes = length;
 
@@ -1298,7 +1298,7 @@ uint64_t xdr_expand_hole(struct xdr_stream *xdr, uint64_t offset, uint64_t lengt
 
 	xdr_realign_pages(xdr);
 	from = xdr_page_pos(xdr);
-	bytes = xdr->nwords << 2;
+	bytes = xdr_stream_remaining(xdr);
 
 	if (offset + length + bytes > buf->page_len) {
 		unsigned int shift = (offset + length + bytes) - buf->page_len;
-- 
2.28.0

[PATCH v2 8/9] NFSv4: Fix open coded xdr_stream_remaining()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs4xdr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 3899ef3047f4..de69276cfd22 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5337,11 +5337,11 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 		res->acl_len = attrlen;
 
 		/* Check for receive buffer overflow */
-		if (res->acl_len > (xdr->nwords << 2) ||
+		if (res->acl_len > xdr_stream_remaining(xdr) ||
 		    res->acl_len + res->acl_data_offset > xdr->buf->page_len) {
 			res->acl_flags |= NFS4_ACL_TRUNC;
-			dprintk("NFS: acl reply: attrlen %u > page_len %u\n",
-					attrlen, xdr->nwords << 2);
+			dprintk("NFS: acl reply: attrlen %u > page_len %zu\n",
+				attrlen, xdr_stream_remaining(xdr));
 		}
 	} else
 		status = -EOPNOTSUPP;
-- 
2.28.0

[PATCH v2 9/9] NFSv4.2: Fix up read_plus() page alignment

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Assume that the first record is a data record, and that we have to read
the 8+4 first bytes of that record before the call to nfs_align_pages().

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs42xdr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
index 6e060a88f98c..f5abace015d7 100644
--- a/fs/nfs/nfs42xdr.c
+++ b/fs/nfs/nfs42xdr.c
@@ -760,14 +760,16 @@ static void nfs4_xdr_enc_read_plus(struct rpc_rqst *req,
 	struct compound_hdr hdr = {
 		.minorversion = nfs4_xdr_minorversion(&args->seq_args),
 	};
+	unsigned int replen;
 
 	encode_compound_hdr(xdr, req, &hdr);
 	encode_sequence(xdr, &args->seq_args, &hdr);
 	encode_putfh(xdr, args->fh, &hdr);
+	replen = hdr.replen + op_decode_hdr_maxsz + 2 + 1 + 3;
 	encode_read_plus(xdr, args, &hdr);
 
 	rpc_prepare_reply_pages(req, args->pages, args->pgbase,
-				args->count, hdr.replen);
+				args->count, replen);
 	encode_nops(&hdr);
 }
 
-- 
2.28.0

Re: [PATCH v2 0/9] Fix various issues in the SUNRPC xdr code

[J. Bruce Fields]

On Tue, Nov 24, 2020 at 08:50:16AM -0500, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> When looking at the issues raised by Tigran's testing of the NFS client
> updates, I noticed a couple of things in the generic SUNRPC xdr code
> that want to be fixed. This patch series replaces an earlier series that
> attempted to just fix the XDR padding in the NFS code.
> 
> This series fixes up a number of issues w.r.t. bounds checking in the
> xdr_stream code. It corrects the behaviour of xdr_read_pages() for the
> case where the XDR object size is larger than the buffer page array
> length and simplifies the code.

I'm seeing this on the client with recent upstream + these patches.

--b.


[  517.213581] ==================================================================
[  517.214699] BUG: KASAN: slab-out-of-bounds in xdr_set_page+0x327/0x370 [sunrpc]
[  517.215875] Read of size 8 at addr ffff888035929680 by task kworker/u4:7/1423

[  517.216958] CPU: 0 PID: 1423 Comm: kworker/u4:7 Not tainted 5.10.0-rc5-16550-gf864315df3e6 #3058
[  517.218027] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-3.fc33 04/01/2014
[  517.219124] Workqueue: rpciod rpc_async_schedule [sunrpc]
[  517.220079] Call Trace:
[  517.220485]  dump_stack+0x9a/0xcc
[  517.221030]  ? xdr_set_page+0x327/0x370 [sunrpc]
[  517.221712]  print_address_description.constprop.0+0x1c/0x1f0
[  517.222492]  ? xdr_set_page+0x327/0x370 [sunrpc]
[  517.223088]  ? xdr_set_page+0x327/0x370 [sunrpc]
[  517.223677]  kasan_report.cold+0x1f/0x37
[  517.224270]  ? xdr_set_page+0x327/0x370 [sunrpc]
[  517.224872]  xdr_set_page+0x327/0x370 [sunrpc]
[  517.225476]  xdr_align_data+0x1c9/0x8e0 [sunrpc]
[  517.226073]  ? lockdep_hardirqs_on_prepare+0x17b/0x400
[  517.226730]  ? kfree+0x118/0x220
[  517.227172]  ? lockdep_hardirqs_on+0x79/0x100
[  517.227745]  ? __decode_op_hdr+0x24/0x4d0 [nfsv4]
[  517.228427]  nfs4_xdr_dec_read_plus+0x360/0x5a0 [nfsv4]
[  517.229117]  ? nfs4_xdr_dec_offload_cancel+0x160/0x160 [nfsv4]
[  517.229877]  gss_unwrap_resp+0x145/0x220 [auth_rpcgss]
[  517.230558]  call_decode+0x5d2/0x830 [sunrpc]
[  517.231127]  ? rpc_decode_header+0x17c0/0x17c0 [sunrpc]
[  517.231785]  ? lockdep_hardirqs_on_prepare+0x400/0x400
[  517.232563]  ? rpc_decode_header+0x17c0/0x17c0 [sunrpc]
[  517.233236]  __rpc_execute+0x1b8/0xf10 [sunrpc]
[  517.233831]  ? rpc_exit+0x110/0x110 [sunrpc]
[  517.234390]  ? lock_downgrade+0x690/0x690
[  517.234918]  rpc_async_schedule+0x9f/0x140 [sunrpc]
[  517.235539]  process_one_work+0x7ac/0x12d0
[  517.236106]  ? lock_release+0x6c0/0x6c0
[  517.236601]  ? queue_delayed_work_on+0x90/0x90
[  517.237170]  ? rwlock_bug.part.0+0x90/0x90
[  517.237694]  worker_thread+0x590/0xf80
[  517.238204]  ? rescuer_thread+0xb80/0xb80
[  517.238714]  kthread+0x375/0x450
[  517.239124]  ? _raw_spin_unlock_irq+0x24/0x50
[  517.239673]  ? kthread_create_worker_on_cpu+0xb0/0xb0
[  517.240392]  ret_from_fork+0x22/0x30

[  517.241072] Allocated by task 9053:
[  517.241533]  kasan_save_stack+0x1b/0x40
[  517.242018]  __kasan_kmalloc.constprop.0+0xbf/0xd0
[  517.242667]  __kmalloc+0x11e/0x210
[  517.243111]  nfs_generic_pgio+0x943/0xe10 [nfs]
[  517.243691]  nfs_generic_pg_pgios+0xea/0x3f0 [nfs]
[  517.244375]  nfs_pageio_doio+0xe3/0x240 [nfs]
[  517.244929]  nfs_pageio_complete+0x143/0x580 [nfs]
[  517.245562]  nfs_readpages+0x331/0x5b0 [nfs]
[  517.246135]  read_pages+0x4ab/0xa40
[  517.246583]  page_cache_ra_unbounded+0x361/0x620
[  517.247165]  generic_file_buffered_read+0x377/0x1e90
[  517.247791]  nfs_file_read+0x144/0x240 [nfs]
[  517.248396]  new_sync_read+0x352/0x5d0
[  517.248870]  vfs_read+0x202/0x3f0
[  517.249290]  ksys_read+0xe9/0x1b0
[  517.249708]  do_syscall_64+0x33/0x40
[  517.251015]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

[  517.252201] The buggy address belongs to the object at ffff888035929600
                which belongs to the cache kmalloc-128 of size 128
[  517.253807] The buggy address is located 0 bytes to the right of
                128-byte region [ffff888035929600, ffff888035929680)
[  517.255217] The buggy address belongs to the page:
[  517.255819] page:00000000ab6145f3 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x35929
[  517.257070] flags: 0x4000000000000200(slab)
[  517.257600] raw: 4000000000000200 ffffea00003970d8 ffffea00004582e8 ffff888007840400
[  517.258582] raw: 0000000000000000 ffff888035929000 0000000100000010
[  517.259362] page dumped because: kasan: bad access detected

[  517.260315] Memory state around the buggy address:
[  517.260912]  ffff888035929580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[  517.261833]  ffff888035929600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  517.262755] >ffff888035929680: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[  517.263655]                    ^
[  517.264133]  ffff888035929700: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[  517.265066]  ffff888035929780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[  517.265967] ==================================================================

Re: [PATCH v2 0/9] Fix various issues in the SUNRPC xdr code

[J. Bruce Fields]

On Tue, Nov 24, 2020 at 11:12:50AM -0500, bfields wrote:
> On Tue, Nov 24, 2020 at 08:50:16AM -0500, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > When looking at the issues raised by Tigran's testing of the NFS client
> > updates, I noticed a couple of things in the generic SUNRPC xdr code
> > that want to be fixed. This patch series replaces an earlier series that
> > attempted to just fix the XDR padding in the NFS code.
> > 
> > This series fixes up a number of issues w.r.t. bounds checking in the
> > xdr_stream code. It corrects the behaviour of xdr_read_pages() for the
> > case where the XDR object size is larger than the buffer page array
> > length and simplifies the code.
> 
> I'm seeing this on the client with recent upstream + these patches.

Unfortunately that was in the middle of a series of tests, and I'm not
sure exactly what triggered it--I'm guessing cthon special over krb5i.
I'll let you know what else I can figure out.

--b.

> [  517.213581] ==================================================================
> [  517.214699] BUG: KASAN: slab-out-of-bounds in xdr_set_page+0x327/0x370 [sunrpc]
> [  517.215875] Read of size 8 at addr ffff888035929680 by task kworker/u4:7/1423
> 
> [  517.216958] CPU: 0 PID: 1423 Comm: kworker/u4:7 Not tainted 5.10.0-rc5-16550-gf864315df3e6 #3058
> [  517.218027] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-3.fc33 04/01/2014
> [  517.219124] Workqueue: rpciod rpc_async_schedule [sunrpc]
> [  517.220079] Call Trace:
> [  517.220485]  dump_stack+0x9a/0xcc
> [  517.221030]  ? xdr_set_page+0x327/0x370 [sunrpc]
> [  517.221712]  print_address_description.constprop.0+0x1c/0x1f0
> [  517.222492]  ? xdr_set_page+0x327/0x370 [sunrpc]
> [  517.223088]  ? xdr_set_page+0x327/0x370 [sunrpc]
> [  517.223677]  kasan_report.cold+0x1f/0x37
> [  517.224270]  ? xdr_set_page+0x327/0x370 [sunrpc]
> [  517.224872]  xdr_set_page+0x327/0x370 [sunrpc]
> [  517.225476]  xdr_align_data+0x1c9/0x8e0 [sunrpc]
> [  517.226073]  ? lockdep_hardirqs_on_prepare+0x17b/0x400
> [  517.226730]  ? kfree+0x118/0x220
> [  517.227172]  ? lockdep_hardirqs_on+0x79/0x100
> [  517.227745]  ? __decode_op_hdr+0x24/0x4d0 [nfsv4]
> [  517.228427]  nfs4_xdr_dec_read_plus+0x360/0x5a0 [nfsv4]
> [  517.229117]  ? nfs4_xdr_dec_offload_cancel+0x160/0x160 [nfsv4]
> [  517.229877]  gss_unwrap_resp+0x145/0x220 [auth_rpcgss]
> [  517.230558]  call_decode+0x5d2/0x830 [sunrpc]
> [  517.231127]  ? rpc_decode_header+0x17c0/0x17c0 [sunrpc]
> [  517.231785]  ? lockdep_hardirqs_on_prepare+0x400/0x400
> [  517.232563]  ? rpc_decode_header+0x17c0/0x17c0 [sunrpc]
> [  517.233236]  __rpc_execute+0x1b8/0xf10 [sunrpc]
> [  517.233831]  ? rpc_exit+0x110/0x110 [sunrpc]
> [  517.234390]  ? lock_downgrade+0x690/0x690
> [  517.234918]  rpc_async_schedule+0x9f/0x140 [sunrpc]
> [  517.235539]  process_one_work+0x7ac/0x12d0
> [  517.236106]  ? lock_release+0x6c0/0x6c0
> [  517.236601]  ? queue_delayed_work_on+0x90/0x90
> [  517.237170]  ? rwlock_bug.part.0+0x90/0x90
> [  517.237694]  worker_thread+0x590/0xf80
> [  517.238204]  ? rescuer_thread+0xb80/0xb80
> [  517.238714]  kthread+0x375/0x450
> [  517.239124]  ? _raw_spin_unlock_irq+0x24/0x50
> [  517.239673]  ? kthread_create_worker_on_cpu+0xb0/0xb0
> [  517.240392]  ret_from_fork+0x22/0x30
> 
> [  517.241072] Allocated by task 9053:
> [  517.241533]  kasan_save_stack+0x1b/0x40
> [  517.242018]  __kasan_kmalloc.constprop.0+0xbf/0xd0
> [  517.242667]  __kmalloc+0x11e/0x210
> [  517.243111]  nfs_generic_pgio+0x943/0xe10 [nfs]
> [  517.243691]  nfs_generic_pg_pgios+0xea/0x3f0 [nfs]
> [  517.244375]  nfs_pageio_doio+0xe3/0x240 [nfs]
> [  517.244929]  nfs_pageio_complete+0x143/0x580 [nfs]
> [  517.245562]  nfs_readpages+0x331/0x5b0 [nfs]
> [  517.246135]  read_pages+0x4ab/0xa40
> [  517.246583]  page_cache_ra_unbounded+0x361/0x620
> [  517.247165]  generic_file_buffered_read+0x377/0x1e90
> [  517.247791]  nfs_file_read+0x144/0x240 [nfs]
> [  517.248396]  new_sync_read+0x352/0x5d0
> [  517.248870]  vfs_read+0x202/0x3f0
> [  517.249290]  ksys_read+0xe9/0x1b0
> [  517.249708]  do_syscall_64+0x33/0x40
> [  517.251015]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> [  517.252201] The buggy address belongs to the object at ffff888035929600
>                 which belongs to the cache kmalloc-128 of size 128
> [  517.253807] The buggy address is located 0 bytes to the right of
>                 128-byte region [ffff888035929600, ffff888035929680)
> [  517.255217] The buggy address belongs to the page:
> [  517.255819] page:00000000ab6145f3 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x35929
> [  517.257070] flags: 0x4000000000000200(slab)
> [  517.257600] raw: 4000000000000200 ffffea00003970d8 ffffea00004582e8 ffff888007840400
> [  517.258582] raw: 0000000000000000 ffff888035929000 0000000100000010
> [  517.259362] page dumped because: kasan: bad access detected
> 
> [  517.260315] Memory state around the buggy address:
> [  517.260912]  ffff888035929580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [  517.261833]  ffff888035929600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [  517.262755] >ffff888035929680: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [  517.263655]                    ^
> [  517.264133]  ffff888035929700: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [  517.265066]  ffff888035929780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [  517.265967] ==================================================================
> 

Re: [PATCH v2 5/9] SUNRPC: Clean up the handling of page padding in rpc_prepare_reply_pages()

[Anna Schumaker]

Hi Trond,

On Tue, Nov 24, 2020 at 8:54 AM <trondmy@kernel.org> wrote:
>
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> rpc_prepare_reply_pages() currently expects the 'hdrsize' argument to
> contain the length of the data that we expect to want placed in the head
> kvec plus a count of 1 word of padding that is placed after the page data.
> This is very confusing when trying to read the code, and sometimes leads
> to callers adding an arbitrary value of '1' just in order to satisfy the
> requirement (whether or not the page data actually needs such padding).
>
> This patch aims to clarify the code by changing the 'hdrsize' argument
> to remove that 1 word of padding. This means we need to subtract the
> padding from all the existing callers.

After applying this patch, xfstests generic/013 on NFS v4.2 gives me:

umount.nfs4: /mnt/test: device is busy
rm: cannot remove
'/mnt/test/fsstress.161220.1/p0/d4/d8XXXXXXXXXXXXXXXXXXXXXXXXX':
Directory not empty
rm: cannot remove '/mnt/test/fsstress.161220.2/p3/d6XX': Directory not empty
rm: cannot remove '/mnt/test/fsstress.161220.2/p5': Directory not empty
rm: cannot remove '/mnt/test/fsstress.161220.2/p6/d4XXXXXXXXXX':
Directory not empty
rm: cannot remove
'/mnt/test/fsstress.161220.2/pc/d2/d4XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d13X':
Directory not empty
rm: cannot remove
'/mnt/test/fsstress.161220.2/pe/d1/dcXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX':
Directory not empty
rm: cannot remove '/mnt/test/fsstress.161220.2/p11/d1': Directory not empty
rm: cannot remove
'/mnt/test/fsstress.161220.2/p9/d0XXXXXXXXXXXXXXXXX/d1XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d17XXXXXXXXX/d19XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX':
Directory not empty

Thanks,
Anna

>
> Fixes: 02ef04e432ba ("NFS: Account for XDR pad of buf->pages")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/nfs2xdr.c  | 19 ++++++++++---------
>  fs/nfs/nfs3xdr.c  | 29 ++++++++++++++++-------------
>  fs/nfs/nfs4xdr.c  | 36 +++++++++++++++++++-----------------
>  net/sunrpc/clnt.c |  5 +----
>  net/sunrpc/xdr.c  |  3 ---
>  5 files changed, 46 insertions(+), 46 deletions(-)
>
> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> index f6676af37d5d..7fba7711e6b3 100644
> --- a/fs/nfs/nfs2xdr.c
> +++ b/fs/nfs/nfs2xdr.c
> @@ -34,6 +34,7 @@
>   * Declare the space requirements for NFS arguments and replies as
>   * number of 32bit-words
>   */
> +#define NFS_pagepad_sz         (1) /* Page padding */
>  #define NFS_fhandle_sz         (8)
>  #define NFS_sattr_sz           (8)
>  #define NFS_filename_sz                (1+(NFS2_MAXNAMLEN>>2))
> @@ -56,11 +57,11 @@
>
>  #define NFS_attrstat_sz                (1+NFS_fattr_sz)
>  #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> -#define NFS_readlinkres_sz     (2+1)
> -#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> +#define NFS_readlinkres_sz     (2+NFS_pagepad_sz)
> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+NFS_pagepad_sz)
>  #define NFS_writeres_sz         (NFS_attrstat_sz)
>  #define NFS_stat_sz            (1)
> -#define NFS_readdirres_sz      (1+1)
> +#define NFS_readdirres_sz      (1+NFS_pagepad_sz)
>  #define NFS_statfsres_sz       (1+NFS_info_sz)
>
>  static int nfs_stat_to_errno(enum nfs_stat);
> @@ -592,8 +593,8 @@ static void nfs2_xdr_enc_readlinkargs(struct rpc_rqst *req,
>         const struct nfs_readlinkargs *args = data;
>
>         encode_fhandle(xdr, args->fh);
> -       rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> -                               args->pglen, NFS_readlinkres_sz);
> +       rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->pglen,
> +                               NFS_readlinkres_sz - NFS_pagepad_sz);
>  }
>
>  /*
> @@ -628,8 +629,8 @@ static void nfs2_xdr_enc_readargs(struct rpc_rqst *req,
>         const struct nfs_pgio_args *args = data;
>
>         encode_readargs(xdr, args);
> -       rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> -                               args->count, NFS_readres_sz);
> +       rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->count,
> +                               NFS_readres_sz - NFS_pagepad_sz);
>         req->rq_rcv_buf.flags |= XDRBUF_READ;
>  }
>
> @@ -786,8 +787,8 @@ static void nfs2_xdr_enc_readdirargs(struct rpc_rqst *req,
>         const struct nfs_readdirargs *args = data;
>
>         encode_readdirargs(xdr, args);
> -       rpc_prepare_reply_pages(req, args->pages, 0,
> -                               args->count, NFS_readdirres_sz);
> +       rpc_prepare_reply_pages(req, args->pages, 0, args->count,
> +                               NFS_readdirres_sz - NFS_pagepad_sz);
>  }
>
>  /*
> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> index 69971f6c840d..ca10072644ff 100644
> --- a/fs/nfs/nfs3xdr.c
> +++ b/fs/nfs/nfs3xdr.c
> @@ -33,6 +33,7 @@
>   * Declare the space requirements for NFS arguments and replies as
>   * number of 32bit-words
>   */
> +#define NFS3_pagepad_sz                (1) /* Page padding */
>  #define NFS3_fhandle_sz                (1+16)
>  #define NFS3_fh_sz             (NFS3_fhandle_sz)       /* shorthand */
>  #define NFS3_sattr_sz          (15)
> @@ -69,13 +70,13 @@
>  #define NFS3_removeres_sz      (NFS3_setattrres_sz)
>  #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
>  #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+NFS3_pagepad_sz)
> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+NFS3_pagepad_sz)
>  #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
>  #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>  #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
>  #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+NFS3_pagepad_sz)
>  #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
>  #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
>  #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> @@ -85,7 +86,8 @@
>  #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
>                                 XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>  #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+\
> +                               NFS3_pagepad_sz)
>  #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
>
>  static int nfs3_stat_to_errno(enum nfs_stat);
> @@ -909,8 +911,8 @@ static void nfs3_xdr_enc_readlink3args(struct rpc_rqst *req,
>         const struct nfs3_readlinkargs *args = data;
>
>         encode_nfs_fh3(xdr, args->fh);
> -       rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> -                               args->pglen, NFS3_readlinkres_sz);
> +       rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->pglen,
> +                               NFS3_readlinkres_sz - NFS3_pagepad_sz);
>  }
>
>  /*
> @@ -939,7 +941,8 @@ static void nfs3_xdr_enc_read3args(struct rpc_rqst *req,
>                                    const void *data)
>  {
>         const struct nfs_pgio_args *args = data;
> -       unsigned int replen = args->replen ? args->replen : NFS3_readres_sz;
> +       unsigned int replen = args->replen ? args->replen :
> +                                            NFS3_readres_sz - NFS3_pagepad_sz;
>
>         encode_read3args(xdr, args);
>         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> @@ -1239,8 +1242,8 @@ static void nfs3_xdr_enc_readdir3args(struct rpc_rqst *req,
>         const struct nfs3_readdirargs *args = data;
>
>         encode_readdir3args(xdr, args);
> -       rpc_prepare_reply_pages(req, args->pages, 0,
> -                               args->count, NFS3_readdirres_sz);
> +       rpc_prepare_reply_pages(req, args->pages, 0, args->count,
> +                               NFS3_readdirres_sz - NFS3_pagepad_sz);
>  }
>
>  /*
> @@ -1281,8 +1284,8 @@ static void nfs3_xdr_enc_readdirplus3args(struct rpc_rqst *req,
>         const struct nfs3_readdirargs *args = data;
>
>         encode_readdirplus3args(xdr, args);
> -       rpc_prepare_reply_pages(req, args->pages, 0,
> -                               args->count, NFS3_readdirres_sz);
> +       rpc_prepare_reply_pages(req, args->pages, 0, args->count,
> +                               NFS3_readdirres_sz - NFS3_pagepad_sz);
>  }
>
>  /*
> @@ -1328,7 +1331,7 @@ static void nfs3_xdr_enc_getacl3args(struct rpc_rqst *req,
>         if (args->mask & (NFS_ACL | NFS_DFACL)) {
>                 rpc_prepare_reply_pages(req, args->pages, 0,
>                                         NFSACL_MAXPAGES << PAGE_SHIFT,
> -                                       ACL3_getaclres_sz);
> +                                       ACL3_getaclres_sz - NFS3_pagepad_sz);
>                 req->rq_rcv_buf.flags |= XDRBUF_SPARSE_PAGES;
>         }
>  }
> @@ -1648,7 +1651,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
>         result->op_status = status;
>         if (status != NFS3_OK)
>                 goto out_status;
> -       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> +       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
>         error = decode_read3resok(xdr, result);
>  out:
>         return error;
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index c16b93df1bc1..3899ef3047f4 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -84,6 +84,7 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>  /* lock,open owner id:
>   * we currently use size 2 (u64) out of (NFS4_OPAQUE_LIMIT  >> 2)
>   */
> +#define pagepad_maxsz          (1)
>  #define open_owner_id_maxsz    (1 + 2 + 1 + 1 + 2)
>  #define lock_owner_id_maxsz    (1 + 1 + 4)
>  #define decode_lockowner_maxsz (1 + XDR_QUADLEN(IDMAP_NAMESZ))
> @@ -215,14 +216,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>                                  nfs4_fattr_bitmap_maxsz)
>  #define encode_read_maxsz      (op_encode_hdr_maxsz + \
>                                  encode_stateid_maxsz + 3)
> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + pagepad_maxsz)
>  #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
>                                  2 + encode_verifier_maxsz + 5 + \
>                                 nfs4_label_maxsz)
>  #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> -                                decode_verifier_maxsz + 1)
> +                                decode_verifier_maxsz + pagepad_maxsz)
>  #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + pagepad_maxsz)
>  #define encode_write_maxsz     (op_encode_hdr_maxsz + \
>                                  encode_stateid_maxsz + 4)
>  #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> @@ -284,14 +285,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>  #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
>  #define encode_getacl_maxsz    (encode_getattr_maxsz)
>  #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> -                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> +                                nfs4_fattr_bitmap_maxsz + 1 + pagepad_maxsz)
>  #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
>                                  encode_stateid_maxsz + 3)
>  #define decode_setacl_maxsz    (decode_setattr_maxsz)
>  #define encode_fs_locations_maxsz \
>                                 (encode_getattr_maxsz)
>  #define decode_fs_locations_maxsz \
> -                               (1)
> +                               (pagepad_maxsz)
>  #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
>  #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>
> @@ -393,12 +394,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>                                   /* devaddr4 payload is read into page */ \
>                                 1 /* notification bitmap length */ + \
>                                 1 /* notification bitmap, word 0 */ + \
> -                               1 /* possible XDR padding */)
> +                               pagepad_maxsz /* possible XDR padding */)
>  #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
>                                 encode_stateid_maxsz)
>  #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
>                                 decode_stateid_maxsz + \
> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + \
> +                               pagepad_maxsz)
>  #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
>                                 2 /* offset */ + \
>                                 2 /* length */ + \
> @@ -2342,7 +2344,7 @@ static void nfs4_xdr_enc_open(struct rpc_rqst *req, struct xdr_stream *xdr,
>                 encode_layoutget(xdr, args->lg_args, &hdr);
>                 rpc_prepare_reply_pages(req, args->lg_args->layout.pages, 0,
>                                         args->lg_args->layout.pglen,
> -                                       hdr.replen);
> +                                       hdr.replen - pagepad_maxsz);
>         }
>         encode_nops(&hdr);
>  }
> @@ -2388,7 +2390,7 @@ static void nfs4_xdr_enc_open_noattr(struct rpc_rqst *req,
>                 encode_layoutget(xdr, args->lg_args, &hdr);
>                 rpc_prepare_reply_pages(req, args->lg_args->layout.pages, 0,
>                                         args->lg_args->layout.pglen,
> -                                       hdr.replen);
> +                                       hdr.replen - pagepad_maxsz);
>         }
>         encode_nops(&hdr);
>  }
> @@ -2499,7 +2501,7 @@ static void nfs4_xdr_enc_readlink(struct rpc_rqst *req, struct xdr_stream *xdr,
>         encode_readlink(xdr, args, req, &hdr);
>
>         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> -                               args->pglen, hdr.replen);
> +                               args->pglen, hdr.replen - pagepad_maxsz);
>         encode_nops(&hdr);
>  }
>
> @@ -2520,7 +2522,7 @@ static void nfs4_xdr_enc_readdir(struct rpc_rqst *req, struct xdr_stream *xdr,
>         encode_readdir(xdr, args, req, &hdr);
>
>         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> -                               args->count, hdr.replen);
> +                               args->count, hdr.replen - pagepad_maxsz);
>         encode_nops(&hdr);
>  }
>
> @@ -2541,7 +2543,7 @@ static void nfs4_xdr_enc_read(struct rpc_rqst *req, struct xdr_stream *xdr,
>         encode_read(xdr, args, &hdr);
>
>         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> -                               args->count, hdr.replen);
> +                               args->count, hdr.replen - pagepad_maxsz);
>         req->rq_rcv_buf.flags |= XDRBUF_READ;
>         encode_nops(&hdr);
>  }
> @@ -2588,7 +2590,7 @@ static void nfs4_xdr_enc_getacl(struct rpc_rqst *req, struct xdr_stream *xdr,
>                         ARRAY_SIZE(nfs4_acl_bitmap), &hdr);
>
>         rpc_prepare_reply_pages(req, args->acl_pages, 0,
> -                               args->acl_len, replen + 1);
> +                               args->acl_len, replen);
>         encode_nops(&hdr);
>  }
>
> @@ -2810,7 +2812,7 @@ static void nfs4_xdr_enc_fs_locations(struct rpc_rqst *req,
>         }
>
>         rpc_prepare_reply_pages(req, (struct page **)&args->page, 0,
> -                               PAGE_SIZE, replen + 1);
> +                               PAGE_SIZE, replen);
>         encode_nops(&hdr);
>  }
>
> @@ -3014,14 +3016,14 @@ static void nfs4_xdr_enc_getdeviceinfo(struct rpc_rqst *req,
>         encode_compound_hdr(xdr, req, &hdr);
>         encode_sequence(xdr, &args->seq_args, &hdr);
>
> -       replen = hdr.replen + op_decode_hdr_maxsz;
> +       replen = hdr.replen + op_decode_hdr_maxsz + 2;
>
>         encode_getdeviceinfo(xdr, args, &hdr);
>
>         /* set up reply kvec. device_addr4 opaque data is read into the
>          * pages */
>         rpc_prepare_reply_pages(req, args->pdev->pages, args->pdev->pgbase,
> -                               args->pdev->pglen, replen + 2 + 1);
> +                               args->pdev->pglen, replen);
>         encode_nops(&hdr);
>  }
>
> @@ -3043,7 +3045,7 @@ static void nfs4_xdr_enc_layoutget(struct rpc_rqst *req,
>         encode_layoutget(xdr, args, &hdr);
>
>         rpc_prepare_reply_pages(req, args->layout.pages, 0,
> -                               args->layout.pglen, hdr.replen);
> +                               args->layout.pglen, hdr.replen - pagepad_maxsz);
>         encode_nops(&hdr);
>  }
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 3259120462ed..612f0a641f4c 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1251,10 +1251,7 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
>                              unsigned int base, unsigned int len,
>                              unsigned int hdrsize)
>  {
> -       /* Subtract one to force an extra word of buffer space for the
> -        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> -        */
> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_ralign - 1;
> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_ralign;
>
>         xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
>         trace_rpc_xdr_reply_pages(req->rq_task, &req->rq_rcv_buf);
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 3ce0a5daa9eb..5a450055469f 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -193,9 +193,6 @@ xdr_inline_pages(struct xdr_buf *xdr, unsigned int offset,
>
>         tail->iov_base = buf + offset;
>         tail->iov_len = buflen - offset;
> -       if ((xdr->page_len & 3) == 0)
> -               tail->iov_len -= sizeof(__be32);
> -
>         xdr->buflen += len;
>  }
>  EXPORT_SYMBOL_GPL(xdr_inline_pages);
> --
> 2.28.0
>

Re: [PATCH v2 5/9] SUNRPC: Clean up the handling of page padding in rpc_prepare_reply_pages()

[Anna Schumaker]

On Tue, Nov 24, 2020 at 1:02 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> Is that triggering the read plus code? This patch does not attempt to fix that.

Looks like it's running fsstress, so probably. I'll try again after
applying patch 9 to see if that makes a difference.

> ________________________________
> From: Anna Schumaker <schumaker.anna@gmail.com>
> Sent: Tuesday, November 24, 2020 12:52
> To: trondmy@kernel.org <trondmy@kernel.org>
> Cc: Linux NFS Mailing List <linux-nfs@vger.kernel.org>
> Subject: Re: [PATCH v2 5/9] SUNRPC: Clean up the handling of page padding in rpc_prepare_reply_pages()
>
> Hi Trond,
>
> On Tue, Nov 24, 2020 at 8:54 AM <trondmy@kernel.org> wrote:
> >
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> >
> > rpc_prepare_reply_pages() currently expects the 'hdrsize' argument to
> > contain the length of the data that we expect to want placed in the head
> > kvec plus a count of 1 word of padding that is placed after the page data.
> > This is very confusing when trying to read the code, and sometimes leads
> > to callers adding an arbitrary value of '1' just in order to satisfy the
> > requirement (whether or not the page data actually needs such padding).
> >
> > This patch aims to clarify the code by changing the 'hdrsize' argument
> > to remove that 1 word of padding. This means we need to subtract the
> > padding from all the existing callers.
>
> After applying this patch, xfstests generic/013 on NFS v4.2 gives me:
>
> umount.nfs4: /mnt/test: device is busy
> rm: cannot remove
> '/mnt/test/fsstress.161220.1/p0/d4/d8XXXXXXXXXXXXXXXXXXXXXXXXX':
> Directory not empty
> rm: cannot remove '/mnt/test/fsstress.161220.2/p3/d6XX': Directory not empty
> rm: cannot remove '/mnt/test/fsstress.161220.2/p5': Directory not empty
> rm: cannot remove '/mnt/test/fsstress.161220.2/p6/d4XXXXXXXXXX':
> Directory not empty
> rm: cannot remove
> '/mnt/test/fsstress.161220.2/pc/d2/d4XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d13X':
> Directory not empty
> rm: cannot remove
> '/mnt/test/fsstress.161220.2/pe/d1/dcXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX':
> Directory not empty
> rm: cannot remove '/mnt/test/fsstress.161220.2/p11/d1': Directory not empty
> rm: cannot remove
> '/mnt/test/fsstress.161220.2/p9/d0XXXXXXXXXXXXXXXXX/d1XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d17XXXXXXXXX/d19XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX':
> Directory not empty
>
> Thanks,
> Anna
>
> >
> > Fixes: 02ef04e432ba ("NFS: Account for XDR pad of buf->pages")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/nfs2xdr.c  | 19 ++++++++++---------
> >  fs/nfs/nfs3xdr.c  | 29 ++++++++++++++++-------------
> >  fs/nfs/nfs4xdr.c  | 36 +++++++++++++++++++-----------------
> >  net/sunrpc/clnt.c |  5 +----
> >  net/sunrpc/xdr.c  |  3 ---
> >  5 files changed, 46 insertions(+), 46 deletions(-)
> >
> > diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> > index f6676af37d5d..7fba7711e6b3 100644
> > --- a/fs/nfs/nfs2xdr.c
> > +++ b/fs/nfs/nfs2xdr.c
> > @@ -34,6 +34,7 @@
> >   * Declare the space requirements for NFS arguments and replies as
> >   * number of 32bit-words
> >   */
> > +#define NFS_pagepad_sz         (1) /* Page padding */
> >  #define NFS_fhandle_sz         (8)
> >  #define NFS_sattr_sz           (8)
> >  #define NFS_filename_sz                (1+(NFS2_MAXNAMLEN>>2))
> > @@ -56,11 +57,11 @@
> >
> >  #define NFS_attrstat_sz                (1+NFS_fattr_sz)
> >  #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> > -#define NFS_readlinkres_sz     (2+1)
> > -#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> > +#define NFS_readlinkres_sz     (2+NFS_pagepad_sz)
> > +#define NFS_readres_sz         (1+NFS_fattr_sz+1+NFS_pagepad_sz)
> >  #define NFS_writeres_sz         (NFS_attrstat_sz)
> >  #define NFS_stat_sz            (1)
> > -#define NFS_readdirres_sz      (1+1)
> > +#define NFS_readdirres_sz      (1+NFS_pagepad_sz)
> >  #define NFS_statfsres_sz       (1+NFS_info_sz)
> >
> >  static int nfs_stat_to_errno(enum nfs_stat);
> > @@ -592,8 +593,8 @@ static void nfs2_xdr_enc_readlinkargs(struct rpc_rqst *req,
> >         const struct nfs_readlinkargs *args = data;
> >
> >         encode_fhandle(xdr, args->fh);
> > -       rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > -                               args->pglen, NFS_readlinkres_sz);
> > +       rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->pglen,
> > +                               NFS_readlinkres_sz - NFS_pagepad_sz);
> >  }
> >
> >  /*
> > @@ -628,8 +629,8 @@ static void nfs2_xdr_enc_readargs(struct rpc_rqst *req,
> >         const struct nfs_pgio_args *args = data;
> >
> >         encode_readargs(xdr, args);
> > -       rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > -                               args->count, NFS_readres_sz);
> > +       rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->count,
> > +                               NFS_readres_sz - NFS_pagepad_sz);
> >         req->rq_rcv_buf.flags |= XDRBUF_READ;
> >  }
> >
> > @@ -786,8 +787,8 @@ static void nfs2_xdr_enc_readdirargs(struct rpc_rqst *req,
> >         const struct nfs_readdirargs *args = data;
> >
> >         encode_readdirargs(xdr, args);
> > -       rpc_prepare_reply_pages(req, args->pages, 0,
> > -                               args->count, NFS_readdirres_sz);
> > +       rpc_prepare_reply_pages(req, args->pages, 0, args->count,
> > +                               NFS_readdirres_sz - NFS_pagepad_sz);
> >  }
> >
> >  /*
> > diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> > index 69971f6c840d..ca10072644ff 100644
> > --- a/fs/nfs/nfs3xdr.c
> > +++ b/fs/nfs/nfs3xdr.c
> > @@ -33,6 +33,7 @@
> >   * Declare the space requirements for NFS arguments and replies as
> >   * number of 32bit-words
> >   */
> > +#define NFS3_pagepad_sz                (1) /* Page padding */
> >  #define NFS3_fhandle_sz                (1+16)
> >  #define NFS3_fh_sz             (NFS3_fhandle_sz)       /* shorthand */
> >  #define NFS3_sattr_sz          (15)
> > @@ -69,13 +70,13 @@
> >  #define NFS3_removeres_sz      (NFS3_setattrres_sz)
> >  #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
> >  #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> > -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> > -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> > +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+NFS3_pagepad_sz)
> > +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+NFS3_pagepad_sz)
> >  #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
> >  #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> >  #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
> >  #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> > -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> > +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+NFS3_pagepad_sz)
> >  #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
> >  #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
> >  #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> > @@ -85,7 +86,8 @@
> >  #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
> >                                 XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> >  #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> > -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> > +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+\
> > +                               NFS3_pagepad_sz)
> >  #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
> >
> >  static int nfs3_stat_to_errno(enum nfs_stat);
> > @@ -909,8 +911,8 @@ static void nfs3_xdr_enc_readlink3args(struct rpc_rqst *req,
> >         const struct nfs3_readlinkargs *args = data;
> >
> >         encode_nfs_fh3(xdr, args->fh);
> > -       rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > -                               args->pglen, NFS3_readlinkres_sz);
> > +       rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->pglen,
> > +                               NFS3_readlinkres_sz - NFS3_pagepad_sz);
> >  }
> >
> >  /*
> > @@ -939,7 +941,8 @@ static void nfs3_xdr_enc_read3args(struct rpc_rqst *req,
> >                                    const void *data)
> >  {
> >         const struct nfs_pgio_args *args = data;
> > -       unsigned int replen = args->replen ? args->replen : NFS3_readres_sz;
> > +       unsigned int replen = args->replen ? args->replen :
> > +                                            NFS3_readres_sz - NFS3_pagepad_sz;
> >
> >         encode_read3args(xdr, args);
> >         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > @@ -1239,8 +1242,8 @@ static void nfs3_xdr_enc_readdir3args(struct rpc_rqst *req,
> >         const struct nfs3_readdirargs *args = data;
> >
> >         encode_readdir3args(xdr, args);
> > -       rpc_prepare_reply_pages(req, args->pages, 0,
> > -                               args->count, NFS3_readdirres_sz);
> > +       rpc_prepare_reply_pages(req, args->pages, 0, args->count,
> > +                               NFS3_readdirres_sz - NFS3_pagepad_sz);
> >  }
> >
> >  /*
> > @@ -1281,8 +1284,8 @@ static void nfs3_xdr_enc_readdirplus3args(struct rpc_rqst *req,
> >         const struct nfs3_readdirargs *args = data;
> >
> >         encode_readdirplus3args(xdr, args);
> > -       rpc_prepare_reply_pages(req, args->pages, 0,
> > -                               args->count, NFS3_readdirres_sz);
> > +       rpc_prepare_reply_pages(req, args->pages, 0, args->count,
> > +                               NFS3_readdirres_sz - NFS3_pagepad_sz);
> >  }
> >
> >  /*
> > @@ -1328,7 +1331,7 @@ static void nfs3_xdr_enc_getacl3args(struct rpc_rqst *req,
> >         if (args->mask & (NFS_ACL | NFS_DFACL)) {
> >                 rpc_prepare_reply_pages(req, args->pages, 0,
> >                                         NFSACL_MAXPAGES << PAGE_SHIFT,
> > -                                       ACL3_getaclres_sz);
> > +                                       ACL3_getaclres_sz - NFS3_pagepad_sz);
> >                 req->rq_rcv_buf.flags |= XDRBUF_SPARSE_PAGES;
> >         }
> >  }
> > @@ -1648,7 +1651,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
> >         result->op_status = status;
> >         if (status != NFS3_OK)
> >                 goto out_status;
> > -       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> > +       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> >         error = decode_read3resok(xdr, result);
> >  out:
> >         return error;
> > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > index c16b93df1bc1..3899ef3047f4 100644
> > --- a/fs/nfs/nfs4xdr.c
> > +++ b/fs/nfs/nfs4xdr.c
> > @@ -84,6 +84,7 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >  /* lock,open owner id:
> >   * we currently use size 2 (u64) out of (NFS4_OPAQUE_LIMIT  >> 2)
> >   */
> > +#define pagepad_maxsz          (1)
> >  #define open_owner_id_maxsz    (1 + 2 + 1 + 1 + 2)
> >  #define lock_owner_id_maxsz    (1 + 1 + 4)
> >  #define decode_lockowner_maxsz (1 + XDR_QUADLEN(IDMAP_NAMESZ))
> > @@ -215,14 +216,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >                                  nfs4_fattr_bitmap_maxsz)
> >  #define encode_read_maxsz      (op_encode_hdr_maxsz + \
> >                                  encode_stateid_maxsz + 3)
> > -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> > +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + pagepad_maxsz)
> >  #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
> >                                  2 + encode_verifier_maxsz + 5 + \
> >                                 nfs4_label_maxsz)
> >  #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> > -                                decode_verifier_maxsz + 1)
> > +                                decode_verifier_maxsz + pagepad_maxsz)
> >  #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> > -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> > +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + pagepad_maxsz)
> >  #define encode_write_maxsz     (op_encode_hdr_maxsz + \
> >                                  encode_stateid_maxsz + 4)
> >  #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> > @@ -284,14 +285,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >  #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> >  #define encode_getacl_maxsz    (encode_getattr_maxsz)
> >  #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> > -                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> > +                                nfs4_fattr_bitmap_maxsz + 1 + pagepad_maxsz)
> >  #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
> >                                  encode_stateid_maxsz + 3)
> >  #define decode_setacl_maxsz    (decode_setattr_maxsz)
> >  #define encode_fs_locations_maxsz \
> >                                 (encode_getattr_maxsz)
> >  #define decode_fs_locations_maxsz \
> > -                               (1)
> > +                               (pagepad_maxsz)
> >  #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
> >  #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> >
> > @@ -393,12 +394,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >                                   /* devaddr4 payload is read into page */ \
> >                                 1 /* notification bitmap length */ + \
> >                                 1 /* notification bitmap, word 0 */ + \
> > -                               1 /* possible XDR padding */)
> > +                               pagepad_maxsz /* possible XDR padding */)
> >  #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
> >                                 encode_stateid_maxsz)
> >  #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
> >                                 decode_stateid_maxsz + \
> > -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> > +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + \
> > +                               pagepad_maxsz)
> >  #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
> >                                 2 /* offset */ + \
> >                                 2 /* length */ + \
> > @@ -2342,7 +2344,7 @@ static void nfs4_xdr_enc_open(struct rpc_rqst *req, struct xdr_stream *xdr,
> >                 encode_layoutget(xdr, args->lg_args, &hdr);
> >                 rpc_prepare_reply_pages(req, args->lg_args->layout.pages, 0,
> >                                         args->lg_args->layout.pglen,
> > -                                       hdr.replen);
> > +                                       hdr.replen - pagepad_maxsz);
> >         }
> >         encode_nops(&hdr);
> >  }
> > @@ -2388,7 +2390,7 @@ static void nfs4_xdr_enc_open_noattr(struct rpc_rqst *req,
> >                 encode_layoutget(xdr, args->lg_args, &hdr);
> >                 rpc_prepare_reply_pages(req, args->lg_args->layout.pages, 0,
> >                                         args->lg_args->layout.pglen,
> > -                                       hdr.replen);
> > +                                       hdr.replen - pagepad_maxsz);
> >         }
> >         encode_nops(&hdr);
> >  }
> > @@ -2499,7 +2501,7 @@ static void nfs4_xdr_enc_readlink(struct rpc_rqst *req, struct xdr_stream *xdr,
> >         encode_readlink(xdr, args, req, &hdr);
> >
> >         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > -                               args->pglen, hdr.replen);
> > +                               args->pglen, hdr.replen - pagepad_maxsz);
> >         encode_nops(&hdr);
> >  }
> >
> > @@ -2520,7 +2522,7 @@ static void nfs4_xdr_enc_readdir(struct rpc_rqst *req, struct xdr_stream *xdr,
> >         encode_readdir(xdr, args, req, &hdr);
> >
> >         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > -                               args->count, hdr.replen);
> > +                               args->count, hdr.replen - pagepad_maxsz);
> >         encode_nops(&hdr);
> >  }
> >
> > @@ -2541,7 +2543,7 @@ static void nfs4_xdr_enc_read(struct rpc_rqst *req, struct xdr_stream *xdr,
> >         encode_read(xdr, args, &hdr);
> >
> >         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > -                               args->count, hdr.replen);
> > +                               args->count, hdr.replen - pagepad_maxsz);
> >         req->rq_rcv_buf.flags |= XDRBUF_READ;
> >         encode_nops(&hdr);
> >  }
> > @@ -2588,7 +2590,7 @@ static void nfs4_xdr_enc_getacl(struct rpc_rqst *req, struct xdr_stream *xdr,
> >                         ARRAY_SIZE(nfs4_acl_bitmap), &hdr);
> >
> >         rpc_prepare_reply_pages(req, args->acl_pages, 0,
> > -                               args->acl_len, replen + 1);
> > +                               args->acl_len, replen);
> >         encode_nops(&hdr);
> >  }
> >
> > @@ -2810,7 +2812,7 @@ static void nfs4_xdr_enc_fs_locations(struct rpc_rqst *req,
> >         }
> >
> >         rpc_prepare_reply_pages(req, (struct page **)&args->page, 0,
> > -                               PAGE_SIZE, replen + 1);
> > +                               PAGE_SIZE, replen);
> >         encode_nops(&hdr);
> >  }
> >
> > @@ -3014,14 +3016,14 @@ static void nfs4_xdr_enc_getdeviceinfo(struct rpc_rqst *req,
> >         encode_compound_hdr(xdr, req, &hdr);
> >         encode_sequence(xdr, &args->seq_args, &hdr);
> >
> > -       replen = hdr.replen + op_decode_hdr_maxsz;
> > +       replen = hdr.replen + op_decode_hdr_maxsz + 2;
> >
> >         encode_getdeviceinfo(xdr, args, &hdr);
> >
> >         /* set up reply kvec. device_addr4 opaque data is read into the
> >          * pages */
> >         rpc_prepare_reply_pages(req, args->pdev->pages, args->pdev->pgbase,
> > -                               args->pdev->pglen, replen + 2 + 1);
> > +                               args->pdev->pglen, replen);
> >         encode_nops(&hdr);
> >  }
> >
> > @@ -3043,7 +3045,7 @@ static void nfs4_xdr_enc_layoutget(struct rpc_rqst *req,
> >         encode_layoutget(xdr, args, &hdr);
> >
> >         rpc_prepare_reply_pages(req, args->layout.pages, 0,
> > -                               args->layout.pglen, hdr.replen);
> > +                               args->layout.pglen, hdr.replen - pagepad_maxsz);
> >         encode_nops(&hdr);
> >  }
> >
> > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > index 3259120462ed..612f0a641f4c 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -1251,10 +1251,7 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
> >                              unsigned int base, unsigned int len,
> >                              unsigned int hdrsize)
> >  {
> > -       /* Subtract one to force an extra word of buffer space for the
> > -        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> > -        */
> > -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_ralign - 1;
> > +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_ralign;
> >
> >         xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
> >         trace_rpc_xdr_reply_pages(req->rq_task, &req->rq_rcv_buf);
> > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > index 3ce0a5daa9eb..5a450055469f 100644
> > --- a/net/sunrpc/xdr.c
> > +++ b/net/sunrpc/xdr.c
> > @@ -193,9 +193,6 @@ xdr_inline_pages(struct xdr_buf *xdr, unsigned int offset,
> >
> >         tail->iov_base = buf + offset;
> >         tail->iov_len = buflen - offset;
> > -       if ((xdr->page_len & 3) == 0)
> > -               tail->iov_len -= sizeof(__be32);
> > -
> >         xdr->buflen += len;
> >  }
> >  EXPORT_SYMBOL_GPL(xdr_inline_pages);
> > --
> > 2.28.0
> >

Re: [PATCH v2 5/9] SUNRPC: Clean up the handling of page padding in rpc_prepare_reply_pages()

[Anna Schumaker]

On Tue, Nov 24, 2020 at 1:04 PM Anna Schumaker <schumaker.anna@gmail.com> wrote:
>
> On Tue, Nov 24, 2020 at 1:02 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
> >
> > Is that triggering the read plus code? This patch does not attempt to fix that.
>
> Looks like it's running fsstress, so probably. I'll try again after
> applying patch 9 to see if that makes a difference.

I'm getting the same error after applying all the patches in the series.

Anna

>
> > ________________________________
> > From: Anna Schumaker <schumaker.anna@gmail.com>
> > Sent: Tuesday, November 24, 2020 12:52
> > To: trondmy@kernel.org <trondmy@kernel.org>
> > Cc: Linux NFS Mailing List <linux-nfs@vger.kernel.org>
> > Subject: Re: [PATCH v2 5/9] SUNRPC: Clean up the handling of page padding in rpc_prepare_reply_pages()
> >
> > Hi Trond,
> >
> > On Tue, Nov 24, 2020 at 8:54 AM <trondmy@kernel.org> wrote:
> > >
> > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > >
> > > rpc_prepare_reply_pages() currently expects the 'hdrsize' argument to
> > > contain the length of the data that we expect to want placed in the head
> > > kvec plus a count of 1 word of padding that is placed after the page data.
> > > This is very confusing when trying to read the code, and sometimes leads
> > > to callers adding an arbitrary value of '1' just in order to satisfy the
> > > requirement (whether or not the page data actually needs such padding).
> > >
> > > This patch aims to clarify the code by changing the 'hdrsize' argument
> > > to remove that 1 word of padding. This means we need to subtract the
> > > padding from all the existing callers.
> >
> > After applying this patch, xfstests generic/013 on NFS v4.2 gives me:
> >
> > umount.nfs4: /mnt/test: device is busy
> > rm: cannot remove
> > '/mnt/test/fsstress.161220.1/p0/d4/d8XXXXXXXXXXXXXXXXXXXXXXXXX':
> > Directory not empty
> > rm: cannot remove '/mnt/test/fsstress.161220.2/p3/d6XX': Directory not empty
> > rm: cannot remove '/mnt/test/fsstress.161220.2/p5': Directory not empty
> > rm: cannot remove '/mnt/test/fsstress.161220.2/p6/d4XXXXXXXXXX':
> > Directory not empty
> > rm: cannot remove
> > '/mnt/test/fsstress.161220.2/pc/d2/d4XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d13X':
> > Directory not empty
> > rm: cannot remove
> > '/mnt/test/fsstress.161220.2/pe/d1/dcXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX':
> > Directory not empty
> > rm: cannot remove '/mnt/test/fsstress.161220.2/p11/d1': Directory not empty
> > rm: cannot remove
> > '/mnt/test/fsstress.161220.2/p9/d0XXXXXXXXXXXXXXXXX/d1XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d17XXXXXXXXX/d19XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX':
> > Directory not empty
> >
> > Thanks,
> > Anna
> >
> > >
> > > Fixes: 02ef04e432ba ("NFS: Account for XDR pad of buf->pages")
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > ---
> > >  fs/nfs/nfs2xdr.c  | 19 ++++++++++---------
> > >  fs/nfs/nfs3xdr.c  | 29 ++++++++++++++++-------------
> > >  fs/nfs/nfs4xdr.c  | 36 +++++++++++++++++++-----------------
> > >  net/sunrpc/clnt.c |  5 +----
> > >  net/sunrpc/xdr.c  |  3 ---
> > >  5 files changed, 46 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> > > index f6676af37d5d..7fba7711e6b3 100644
> > > --- a/fs/nfs/nfs2xdr.c
> > > +++ b/fs/nfs/nfs2xdr.c
> > > @@ -34,6 +34,7 @@
> > >   * Declare the space requirements for NFS arguments and replies as
> > >   * number of 32bit-words
> > >   */
> > > +#define NFS_pagepad_sz         (1) /* Page padding */
> > >  #define NFS_fhandle_sz         (8)
> > >  #define NFS_sattr_sz           (8)
> > >  #define NFS_filename_sz                (1+(NFS2_MAXNAMLEN>>2))
> > > @@ -56,11 +57,11 @@
> > >
> > >  #define NFS_attrstat_sz                (1+NFS_fattr_sz)
> > >  #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> > > -#define NFS_readlinkres_sz     (2+1)
> > > -#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> > > +#define NFS_readlinkres_sz     (2+NFS_pagepad_sz)
> > > +#define NFS_readres_sz         (1+NFS_fattr_sz+1+NFS_pagepad_sz)
> > >  #define NFS_writeres_sz         (NFS_attrstat_sz)
> > >  #define NFS_stat_sz            (1)
> > > -#define NFS_readdirres_sz      (1+1)
> > > +#define NFS_readdirres_sz      (1+NFS_pagepad_sz)
> > >  #define NFS_statfsres_sz       (1+NFS_info_sz)
> > >
> > >  static int nfs_stat_to_errno(enum nfs_stat);
> > > @@ -592,8 +593,8 @@ static void nfs2_xdr_enc_readlinkargs(struct rpc_rqst *req,
> > >         const struct nfs_readlinkargs *args = data;
> > >
> > >         encode_fhandle(xdr, args->fh);
> > > -       rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > > -                               args->pglen, NFS_readlinkres_sz);
> > > +       rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->pglen,
> > > +                               NFS_readlinkres_sz - NFS_pagepad_sz);
> > >  }
> > >
> > >  /*
> > > @@ -628,8 +629,8 @@ static void nfs2_xdr_enc_readargs(struct rpc_rqst *req,
> > >         const struct nfs_pgio_args *args = data;
> > >
> > >         encode_readargs(xdr, args);
> > > -       rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > > -                               args->count, NFS_readres_sz);
> > > +       rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->count,
> > > +                               NFS_readres_sz - NFS_pagepad_sz);
> > >         req->rq_rcv_buf.flags |= XDRBUF_READ;
> > >  }
> > >
> > > @@ -786,8 +787,8 @@ static void nfs2_xdr_enc_readdirargs(struct rpc_rqst *req,
> > >         const struct nfs_readdirargs *args = data;
> > >
> > >         encode_readdirargs(xdr, args);
> > > -       rpc_prepare_reply_pages(req, args->pages, 0,
> > > -                               args->count, NFS_readdirres_sz);
> > > +       rpc_prepare_reply_pages(req, args->pages, 0, args->count,
> > > +                               NFS_readdirres_sz - NFS_pagepad_sz);
> > >  }
> > >
> > >  /*
> > > diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> > > index 69971f6c840d..ca10072644ff 100644
> > > --- a/fs/nfs/nfs3xdr.c
> > > +++ b/fs/nfs/nfs3xdr.c
> > > @@ -33,6 +33,7 @@
> > >   * Declare the space requirements for NFS arguments and replies as
> > >   * number of 32bit-words
> > >   */
> > > +#define NFS3_pagepad_sz                (1) /* Page padding */
> > >  #define NFS3_fhandle_sz                (1+16)
> > >  #define NFS3_fh_sz             (NFS3_fhandle_sz)       /* shorthand */
> > >  #define NFS3_sattr_sz          (15)
> > > @@ -69,13 +70,13 @@
> > >  #define NFS3_removeres_sz      (NFS3_setattrres_sz)
> > >  #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
> > >  #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> > > -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> > > -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> > > +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+NFS3_pagepad_sz)
> > > +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+NFS3_pagepad_sz)
> > >  #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
> > >  #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> > >  #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
> > >  #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> > > -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> > > +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+NFS3_pagepad_sz)
> > >  #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
> > >  #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
> > >  #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> > > @@ -85,7 +86,8 @@
> > >  #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
> > >                                 XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> > >  #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> > > -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> > > +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+\
> > > +                               NFS3_pagepad_sz)
> > >  #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
> > >
> > >  static int nfs3_stat_to_errno(enum nfs_stat);
> > > @@ -909,8 +911,8 @@ static void nfs3_xdr_enc_readlink3args(struct rpc_rqst *req,
> > >         const struct nfs3_readlinkargs *args = data;
> > >
> > >         encode_nfs_fh3(xdr, args->fh);
> > > -       rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > > -                               args->pglen, NFS3_readlinkres_sz);
> > > +       rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->pglen,
> > > +                               NFS3_readlinkres_sz - NFS3_pagepad_sz);
> > >  }
> > >
> > >  /*
> > > @@ -939,7 +941,8 @@ static void nfs3_xdr_enc_read3args(struct rpc_rqst *req,
> > >                                    const void *data)
> > >  {
> > >         const struct nfs_pgio_args *args = data;
> > > -       unsigned int replen = args->replen ? args->replen : NFS3_readres_sz;
> > > +       unsigned int replen = args->replen ? args->replen :
> > > +                                            NFS3_readres_sz - NFS3_pagepad_sz;
> > >
> > >         encode_read3args(xdr, args);
> > >         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > > @@ -1239,8 +1242,8 @@ static void nfs3_xdr_enc_readdir3args(struct rpc_rqst *req,
> > >         const struct nfs3_readdirargs *args = data;
> > >
> > >         encode_readdir3args(xdr, args);
> > > -       rpc_prepare_reply_pages(req, args->pages, 0,
> > > -                               args->count, NFS3_readdirres_sz);
> > > +       rpc_prepare_reply_pages(req, args->pages, 0, args->count,
> > > +                               NFS3_readdirres_sz - NFS3_pagepad_sz);
> > >  }
> > >
> > >  /*
> > > @@ -1281,8 +1284,8 @@ static void nfs3_xdr_enc_readdirplus3args(struct rpc_rqst *req,
> > >         const struct nfs3_readdirargs *args = data;
> > >
> > >         encode_readdirplus3args(xdr, args);
> > > -       rpc_prepare_reply_pages(req, args->pages, 0,
> > > -                               args->count, NFS3_readdirres_sz);
> > > +       rpc_prepare_reply_pages(req, args->pages, 0, args->count,
> > > +                               NFS3_readdirres_sz - NFS3_pagepad_sz);
> > >  }
> > >
> > >  /*
> > > @@ -1328,7 +1331,7 @@ static void nfs3_xdr_enc_getacl3args(struct rpc_rqst *req,
> > >         if (args->mask & (NFS_ACL | NFS_DFACL)) {
> > >                 rpc_prepare_reply_pages(req, args->pages, 0,
> > >                                         NFSACL_MAXPAGES << PAGE_SHIFT,
> > > -                                       ACL3_getaclres_sz);
> > > +                                       ACL3_getaclres_sz - NFS3_pagepad_sz);
> > >                 req->rq_rcv_buf.flags |= XDRBUF_SPARSE_PAGES;
> > >         }
> > >  }
> > > @@ -1648,7 +1651,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
> > >         result->op_status = status;
> > >         if (status != NFS3_OK)
> > >                 goto out_status;
> > > -       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> > > +       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> > >         error = decode_read3resok(xdr, result);
> > >  out:
> > >         return error;
> > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > > index c16b93df1bc1..3899ef3047f4 100644
> > > --- a/fs/nfs/nfs4xdr.c
> > > +++ b/fs/nfs/nfs4xdr.c
> > > @@ -84,6 +84,7 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > >  /* lock,open owner id:
> > >   * we currently use size 2 (u64) out of (NFS4_OPAQUE_LIMIT  >> 2)
> > >   */
> > > +#define pagepad_maxsz          (1)
> > >  #define open_owner_id_maxsz    (1 + 2 + 1 + 1 + 2)
> > >  #define lock_owner_id_maxsz    (1 + 1 + 4)
> > >  #define decode_lockowner_maxsz (1 + XDR_QUADLEN(IDMAP_NAMESZ))
> > > @@ -215,14 +216,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > >                                  nfs4_fattr_bitmap_maxsz)
> > >  #define encode_read_maxsz      (op_encode_hdr_maxsz + \
> > >                                  encode_stateid_maxsz + 3)
> > > -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> > > +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + pagepad_maxsz)
> > >  #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
> > >                                  2 + encode_verifier_maxsz + 5 + \
> > >                                 nfs4_label_maxsz)
> > >  #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> > > -                                decode_verifier_maxsz + 1)
> > > +                                decode_verifier_maxsz + pagepad_maxsz)
> > >  #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> > > -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> > > +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + pagepad_maxsz)
> > >  #define encode_write_maxsz     (op_encode_hdr_maxsz + \
> > >                                  encode_stateid_maxsz + 4)
> > >  #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> > > @@ -284,14 +285,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > >  #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> > >  #define encode_getacl_maxsz    (encode_getattr_maxsz)
> > >  #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> > > -                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> > > +                                nfs4_fattr_bitmap_maxsz + 1 + pagepad_maxsz)
> > >  #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
> > >                                  encode_stateid_maxsz + 3)
> > >  #define decode_setacl_maxsz    (decode_setattr_maxsz)
> > >  #define encode_fs_locations_maxsz \
> > >                                 (encode_getattr_maxsz)
> > >  #define decode_fs_locations_maxsz \
> > > -                               (1)
> > > +                               (pagepad_maxsz)
> > >  #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
> > >  #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> > >
> > > @@ -393,12 +394,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > >                                   /* devaddr4 payload is read into page */ \
> > >                                 1 /* notification bitmap length */ + \
> > >                                 1 /* notification bitmap, word 0 */ + \
> > > -                               1 /* possible XDR padding */)
> > > +                               pagepad_maxsz /* possible XDR padding */)
> > >  #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
> > >                                 encode_stateid_maxsz)
> > >  #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
> > >                                 decode_stateid_maxsz + \
> > > -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> > > +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + \
> > > +                               pagepad_maxsz)
> > >  #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
> > >                                 2 /* offset */ + \
> > >                                 2 /* length */ + \
> > > @@ -2342,7 +2344,7 @@ static void nfs4_xdr_enc_open(struct rpc_rqst *req, struct xdr_stream *xdr,
> > >                 encode_layoutget(xdr, args->lg_args, &hdr);
> > >                 rpc_prepare_reply_pages(req, args->lg_args->layout.pages, 0,
> > >                                         args->lg_args->layout.pglen,
> > > -                                       hdr.replen);
> > > +                                       hdr.replen - pagepad_maxsz);
> > >         }
> > >         encode_nops(&hdr);
> > >  }
> > > @@ -2388,7 +2390,7 @@ static void nfs4_xdr_enc_open_noattr(struct rpc_rqst *req,
> > >                 encode_layoutget(xdr, args->lg_args, &hdr);
> > >                 rpc_prepare_reply_pages(req, args->lg_args->layout.pages, 0,
> > >                                         args->lg_args->layout.pglen,
> > > -                                       hdr.replen);
> > > +                                       hdr.replen - pagepad_maxsz);
> > >         }
> > >         encode_nops(&hdr);
> > >  }
> > > @@ -2499,7 +2501,7 @@ static void nfs4_xdr_enc_readlink(struct rpc_rqst *req, struct xdr_stream *xdr,
> > >         encode_readlink(xdr, args, req, &hdr);
> > >
> > >         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > > -                               args->pglen, hdr.replen);
> > > +                               args->pglen, hdr.replen - pagepad_maxsz);
> > >         encode_nops(&hdr);
> > >  }
> > >
> > > @@ -2520,7 +2522,7 @@ static void nfs4_xdr_enc_readdir(struct rpc_rqst *req, struct xdr_stream *xdr,
> > >         encode_readdir(xdr, args, req, &hdr);
> > >
> > >         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > > -                               args->count, hdr.replen);
> > > +                               args->count, hdr.replen - pagepad_maxsz);
> > >         encode_nops(&hdr);
> > >  }
> > >
> > > @@ -2541,7 +2543,7 @@ static void nfs4_xdr_enc_read(struct rpc_rqst *req, struct xdr_stream *xdr,
> > >         encode_read(xdr, args, &hdr);
> > >
> > >         rpc_prepare_reply_pages(req, args->pages, args->pgbase,
> > > -                               args->count, hdr.replen);
> > > +                               args->count, hdr.replen - pagepad_maxsz);
> > >         req->rq_rcv_buf.flags |= XDRBUF_READ;
> > >         encode_nops(&hdr);
> > >  }
> > > @@ -2588,7 +2590,7 @@ static void nfs4_xdr_enc_getacl(struct rpc_rqst *req, struct xdr_stream *xdr,
> > >                         ARRAY_SIZE(nfs4_acl_bitmap), &hdr);
> > >
> > >         rpc_prepare_reply_pages(req, args->acl_pages, 0,
> > > -                               args->acl_len, replen + 1);
> > > +                               args->acl_len, replen);
> > >         encode_nops(&hdr);
> > >  }
> > >
> > > @@ -2810,7 +2812,7 @@ static void nfs4_xdr_enc_fs_locations(struct rpc_rqst *req,
> > >         }
> > >
> > >         rpc_prepare_reply_pages(req, (struct page **)&args->page, 0,
> > > -                               PAGE_SIZE, replen + 1);
> > > +                               PAGE_SIZE, replen);
> > >         encode_nops(&hdr);
> > >  }
> > >
> > > @@ -3014,14 +3016,14 @@ static void nfs4_xdr_enc_getdeviceinfo(struct rpc_rqst *req,
> > >         encode_compound_hdr(xdr, req, &hdr);
> > >         encode_sequence(xdr, &args->seq_args, &hdr);
> > >
> > > -       replen = hdr.replen + op_decode_hdr_maxsz;
> > > +       replen = hdr.replen + op_decode_hdr_maxsz + 2;
> > >
> > >         encode_getdeviceinfo(xdr, args, &hdr);
> > >
> > >         /* set up reply kvec. device_addr4 opaque data is read into the
> > >          * pages */
> > >         rpc_prepare_reply_pages(req, args->pdev->pages, args->pdev->pgbase,
> > > -                               args->pdev->pglen, replen + 2 + 1);
> > > +                               args->pdev->pglen, replen);
> > >         encode_nops(&hdr);
> > >  }
> > >
> > > @@ -3043,7 +3045,7 @@ static void nfs4_xdr_enc_layoutget(struct rpc_rqst *req,
> > >         encode_layoutget(xdr, args, &hdr);
> > >
> > >         rpc_prepare_reply_pages(req, args->layout.pages, 0,
> > > -                               args->layout.pglen, hdr.replen);
> > > +                               args->layout.pglen, hdr.replen - pagepad_maxsz);
> > >         encode_nops(&hdr);
> > >  }
> > >
> > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > > index 3259120462ed..612f0a641f4c 100644
> > > --- a/net/sunrpc/clnt.c
> > > +++ b/net/sunrpc/clnt.c
> > > @@ -1251,10 +1251,7 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
> > >                              unsigned int base, unsigned int len,
> > >                              unsigned int hdrsize)
> > >  {
> > > -       /* Subtract one to force an extra word of buffer space for the
> > > -        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> > > -        */
> > > -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_ralign - 1;
> > > +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_ralign;
> > >
> > >         xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
> > >         trace_rpc_xdr_reply_pages(req->rq_task, &req->rq_rcv_buf);
> > > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > > index 3ce0a5daa9eb..5a450055469f 100644
> > > --- a/net/sunrpc/xdr.c
> > > +++ b/net/sunrpc/xdr.c
> > > @@ -193,9 +193,6 @@ xdr_inline_pages(struct xdr_buf *xdr, unsigned int offset,
> > >
> > >         tail->iov_base = buf + offset;
> > >         tail->iov_len = buflen - offset;
> > > -       if ((xdr->page_len & 3) == 0)
> > > -               tail->iov_len -= sizeof(__be32);
> > > -
> > >         xdr->buflen += len;
> > >  }
> > >  EXPORT_SYMBOL_GPL(xdr_inline_pages);
> > > --
> > > 2.28.0
> > >

Re: [PATCH v2 0/9] Fix various issues in the SUNRPC xdr code

[J. Bruce Fields]

On Tue, Nov 24, 2020 at 11:18:09AM -0500, J. Bruce Fields wrote:
> On Tue, Nov 24, 2020 at 11:12:50AM -0500, bfields wrote:
> > On Tue, Nov 24, 2020 at 08:50:16AM -0500, trondmy@kernel.org wrote:
> > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > 
> > > When looking at the issues raised by Tigran's testing of the NFS client
> > > updates, I noticed a couple of things in the generic SUNRPC xdr code
> > > that want to be fixed. This patch series replaces an earlier series that
> > > attempted to just fix the XDR padding in the NFS code.
> > > 
> > > This series fixes up a number of issues w.r.t. bounds checking in the
> > > xdr_stream code. It corrects the behaviour of xdr_read_pages() for the
> > > case where the XDR object size is larger than the buffer page array
> > > length and simplifies the code.
> > 
> > I'm seeing this on the client with recent upstream + these patches.
> 
> Unfortunately that was in the middle of a series of tests, and I'm not
> sure exactly what triggered it--I'm guessing cthon special over krb5i.
> I'll let you know what else I can figure out.

Yeah, reproduceable by running cthon -s over krb5i, and it first shows
up with the last patch, "NFSv4.2: Fix up read_plus() page alignment".

--b.

> 
> --b.
> 
> > [  517.213581] ==================================================================
> > [  517.214699] BUG: KASAN: slab-out-of-bounds in xdr_set_page+0x327/0x370 [sunrpc]
> > [  517.215875] Read of size 8 at addr ffff888035929680 by task kworker/u4:7/1423
> > 
> > [  517.216958] CPU: 0 PID: 1423 Comm: kworker/u4:7 Not tainted 5.10.0-rc5-16550-gf864315df3e6 #3058
> > [  517.218027] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-3.fc33 04/01/2014
> > [  517.219124] Workqueue: rpciod rpc_async_schedule [sunrpc]
> > [  517.220079] Call Trace:
> > [  517.220485]  dump_stack+0x9a/0xcc
> > [  517.221030]  ? xdr_set_page+0x327/0x370 [sunrpc]
> > [  517.221712]  print_address_description.constprop.0+0x1c/0x1f0
> > [  517.222492]  ? xdr_set_page+0x327/0x370 [sunrpc]
> > [  517.223088]  ? xdr_set_page+0x327/0x370 [sunrpc]
> > [  517.223677]  kasan_report.cold+0x1f/0x37
> > [  517.224270]  ? xdr_set_page+0x327/0x370 [sunrpc]
> > [  517.224872]  xdr_set_page+0x327/0x370 [sunrpc]
> > [  517.225476]  xdr_align_data+0x1c9/0x8e0 [sunrpc]
> > [  517.226073]  ? lockdep_hardirqs_on_prepare+0x17b/0x400
> > [  517.226730]  ? kfree+0x118/0x220
> > [  517.227172]  ? lockdep_hardirqs_on+0x79/0x100
> > [  517.227745]  ? __decode_op_hdr+0x24/0x4d0 [nfsv4]
> > [  517.228427]  nfs4_xdr_dec_read_plus+0x360/0x5a0 [nfsv4]
> > [  517.229117]  ? nfs4_xdr_dec_offload_cancel+0x160/0x160 [nfsv4]
> > [  517.229877]  gss_unwrap_resp+0x145/0x220 [auth_rpcgss]
> > [  517.230558]  call_decode+0x5d2/0x830 [sunrpc]
> > [  517.231127]  ? rpc_decode_header+0x17c0/0x17c0 [sunrpc]
> > [  517.231785]  ? lockdep_hardirqs_on_prepare+0x400/0x400
> > [  517.232563]  ? rpc_decode_header+0x17c0/0x17c0 [sunrpc]
> > [  517.233236]  __rpc_execute+0x1b8/0xf10 [sunrpc]
> > [  517.233831]  ? rpc_exit+0x110/0x110 [sunrpc]
> > [  517.234390]  ? lock_downgrade+0x690/0x690
> > [  517.234918]  rpc_async_schedule+0x9f/0x140 [sunrpc]
> > [  517.235539]  process_one_work+0x7ac/0x12d0
> > [  517.236106]  ? lock_release+0x6c0/0x6c0
> > [  517.236601]  ? queue_delayed_work_on+0x90/0x90
> > [  517.237170]  ? rwlock_bug.part.0+0x90/0x90
> > [  517.237694]  worker_thread+0x590/0xf80
> > [  517.238204]  ? rescuer_thread+0xb80/0xb80
> > [  517.238714]  kthread+0x375/0x450
> > [  517.239124]  ? _raw_spin_unlock_irq+0x24/0x50
> > [  517.239673]  ? kthread_create_worker_on_cpu+0xb0/0xb0
> > [  517.240392]  ret_from_fork+0x22/0x30
> > 
> > [  517.241072] Allocated by task 9053:
> > [  517.241533]  kasan_save_stack+0x1b/0x40
> > [  517.242018]  __kasan_kmalloc.constprop.0+0xbf/0xd0
> > [  517.242667]  __kmalloc+0x11e/0x210
> > [  517.243111]  nfs_generic_pgio+0x943/0xe10 [nfs]
> > [  517.243691]  nfs_generic_pg_pgios+0xea/0x3f0 [nfs]
> > [  517.244375]  nfs_pageio_doio+0xe3/0x240 [nfs]
> > [  517.244929]  nfs_pageio_complete+0x143/0x580 [nfs]
> > [  517.245562]  nfs_readpages+0x331/0x5b0 [nfs]
> > [  517.246135]  read_pages+0x4ab/0xa40
> > [  517.246583]  page_cache_ra_unbounded+0x361/0x620
> > [  517.247165]  generic_file_buffered_read+0x377/0x1e90
> > [  517.247791]  nfs_file_read+0x144/0x240 [nfs]
> > [  517.248396]  new_sync_read+0x352/0x5d0
> > [  517.248870]  vfs_read+0x202/0x3f0
> > [  517.249290]  ksys_read+0xe9/0x1b0
> > [  517.249708]  do_syscall_64+0x33/0x40
> > [  517.251015]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > 
> > [  517.252201] The buggy address belongs to the object at ffff888035929600
> >                 which belongs to the cache kmalloc-128 of size 128
> > [  517.253807] The buggy address is located 0 bytes to the right of
> >                 128-byte region [ffff888035929600, ffff888035929680)
> > [  517.255217] The buggy address belongs to the page:
> > [  517.255819] page:00000000ab6145f3 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x35929
> > [  517.257070] flags: 0x4000000000000200(slab)
> > [  517.257600] raw: 4000000000000200 ffffea00003970d8 ffffea00004582e8 ffff888007840400
> > [  517.258582] raw: 0000000000000000 ffff888035929000 0000000100000010
> > [  517.259362] page dumped because: kasan: bad access detected
> > 
> > [  517.260315] Memory state around the buggy address:
> > [  517.260912]  ffff888035929580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [  517.261833]  ffff888035929600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  517.262755] >ffff888035929680: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [  517.263655]                    ^
> > [  517.264133]  ffff888035929700: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > [  517.265066]  ffff888035929780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > [  517.265967] ==================================================================
> > 

Re: [PATCH v2 0/9] Fix various issues in the SUNRPC xdr code

[Trond Myklebust]

On Tue, 2020-11-24 at 15:26 -0500, J. Bruce Fields wrote:
> On Tue, Nov 24, 2020 at 11:18:09AM -0500, J. Bruce Fields wrote:
> > On Tue, Nov 24, 2020 at 11:12:50AM -0500, bfields wrote:
> > > On Tue, Nov 24, 2020 at 08:50:16AM -0500,
> > > trondmy@kernel.org wrote:
> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > 
> > > > When looking at the issues raised by Tigran's testing of the
> > > > NFS client
> > > > updates, I noticed a couple of things in the generic SUNRPC xdr
> > > > code
> > > > that want to be fixed. This patch series replaces an earlier
> > > > series that
> > > > attempted to just fix the XDR padding in the NFS code.
> > > > 
> > > > This series fixes up a number of issues w.r.t. bounds checking
> > > > in the
> > > > xdr_stream code. It corrects the behaviour of xdr_read_pages()
> > > > for the
> > > > case where the XDR object size is larger than the buffer page
> > > > array
> > > > length and simplifies the code.
> > > 
> > > I'm seeing this on the client with recent upstream + these
> > > patches.
> > 
> > Unfortunately that was in the middle of a series of tests, and I'm
> > not
> > sure exactly what triggered it--I'm guessing cthon special over
> > krb5i.
> > I'll let you know what else I can figure out.
> 
> Yeah, reproduceable by running cthon -s over krb5i, and it first
> shows
> up with the last patch, "NFSv4.2: Fix up read_plus() page alignment".

OK, thanks! I'll just drop that one then. I don't think it really
suffices to fix READ_PLUS as it stands.


-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v2 0/9] Fix various issues in the SUNRPC xdr code

[Mkrtchyan, Tigran]

Just in case I did a mistake during bisecting I had re-done it and got again

c567552612ece787b178e3b147b5854ad422a836
Author: Anna Schumaker <Anna.Schumaker@Netapp.com>
Date:   Wed May 28 13:41:22 2014 -0400

    NFS: Add READ_PLUS data segment support

But change doesn't looks like it can break getdeviceinfo.

Tigran.

----- Original Message -----
> From: "trondmy" <trondmy@hammerspace.com>
> To: "J. Bruce Fields" <bfields@fieldses.org>
> Cc: schumakeranna@gmail.com, "linux-nfs" <linux-nfs@vger.kernel.org>
> Sent: Wednesday, 25 November, 2020 01:36:42
> Subject: Re: [PATCH v2 0/9] Fix various issues in the SUNRPC xdr code

> On Tue, 2020-11-24 at 15:26 -0500, J. Bruce Fields wrote:
>> On Tue, Nov 24, 2020 at 11:18:09AM -0500, J. Bruce Fields wrote:
>> > On Tue, Nov 24, 2020 at 11:12:50AM -0500, bfields wrote:
>> > > On Tue, Nov 24, 2020 at 08:50:16AM -0500,
>> > > trondmy@kernel.org wrote:
>> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
>> > > > 
>> > > > When looking at the issues raised by Tigran's testing of the
>> > > > NFS client
>> > > > updates, I noticed a couple of things in the generic SUNRPC xdr
>> > > > code
>> > > > that want to be fixed. This patch series replaces an earlier
>> > > > series that
>> > > > attempted to just fix the XDR padding in the NFS code.
>> > > > 
>> > > > This series fixes up a number of issues w.r.t. bounds checking
>> > > > in the
>> > > > xdr_stream code. It corrects the behaviour of xdr_read_pages()
>> > > > for the
>> > > > case where the XDR object size is larger than the buffer page
>> > > > array
>> > > > length and simplifies the code.
>> > > 
>> > > I'm seeing this on the client with recent upstream + these
>> > > patches.
>> > 
>> > Unfortunately that was in the middle of a series of tests, and I'm
>> > not
>> > sure exactly what triggered it--I'm guessing cthon special over
>> > krb5i.
>> > I'll let you know what else I can figure out.
>> 
>> Yeah, reproduceable by running cthon -s over krb5i, and it first
>> shows
>> up with the last patch, "NFSv4.2: Fix up read_plus() page alignment".
> 
> OK, thanks! I'll just drop that one then. I don't think it really
> suffices to fix READ_PLUS as it stands.
> 
> 
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com