Re: Regression: NULL pointer dereference after NFS_V4_2_READ_PLUS (commit 7fd461c47)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 15, 2023 at 1:16 PM Anna Schumaker <schumaker.anna@xxxxxxxxx> wrote:
>
> On Thu, Jun 15, 2023 at 1:04 PM Anna Schumaker <schumaker.anna@xxxxxxxxx> wrote:
> >
> > On Thu, Jun 15, 2023 at 9:01 AM Anna Schumaker <schumaker.anna@xxxxxxxxx> wrote:
> > >
> > > On Thu, Jun 15, 2023 at 4:55 AM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > > >
> > > > On 15/06/2023 10:52, Krzysztof Kozlowski wrote:
> > > > > On 14/06/2023 22:55, Anna Schumaker wrote:
> > > > >>>>> Still null ptr (built on 420b2d4 with your patch):
> > > > >>>>
> > > > >>>> We're through the merge window and at rc1 now, so I can spend more
> > > > >>>> time scratching my head over your bug again. We've come up with a
> > > > >>>> patch (attached) that adds a bunch of printks to show us what the
> > > > >>>> kernel thinks is going on. Do you mind trying it out and letting us
> > > > >>>> know what gets printed out? You'll need to make sure
> > > > >>>> CONFIG_NFS_V4_2_READ_PLUS is enabled when compiling the kernel.
> > > > >>>
> > > > >>> The patch does not apply. I tried: v6.4-rc1, v6.4-rc5, next-20230609.
> > > > >>
> > > > >> Can you try the attached patch on top of my 3-patch series from the
> > > > >> other day, and let me know what gets printed out? It adds a bunch of
> > > > >> printk()s at strategic points to print out what is going on with the
> > > > >> xdr scratch buffer since it's suddenly a bad memory address after
> > > > >> working for a bit on your machine.
> > > > >>
> > > > >
> > > > > Here you have entire log - attached (113 kB, I hope goes past mailing
> > > > > lists/spam filters).
> > > >
> > > > As expected this bounced from the mailing lists, but I hope you got it.
> > > > If not, let me know.
> > >
> > > I did still receive it. Thanks!
> >
> > Can you swap out yesterday's patch with this patch? I've adjusted what
> > gets printed out, and added printk()s to xdr_copy_to_scratch().  I'm
> > starting to think that the xdr scratch buffer is fine, and that it's
> > the other pointer passed to memcpy() in that function that's the
> > problem, and the output from this patch will confirm for me.
>
> Oh, and can you add this one on top of the v2 patch as well?

Sorry about the noise today. Can you use this patch instead of the two
I attached earlier? I cleaned up the output and cut down on extra
output..

>
> Thanks,
> Anna
>
> >
> > Thanks,
> > Anna
> >
> > >
> > > Anna
> > > >
> > > > Best regards,
> > > > Krzysztof
> > > >
From 22f96107e7fe5874d4f7915294576e93422d9fbc Mon Sep 17 00:00:00 2001
From: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
Date: Wed, 14 Jun 2023 16:49:37 -0400
Subject: [PATCH] NFS: Add debugging printk()s to trace the READ_PLUS xdr
 decoding

In addition to tracing the lifetime of the xdr->scratch buffer, it adds
information about the xdr stream such as remaining bytes, current decode
offset, and some other (hopefully) helpful stuff.

Signed-off-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
---
 fs/nfs/nfs42xdr.c          | 18 ++++++++++++++++--
 fs/nfs/read.c              |  8 +++++++-
 include/linux/sunrpc/xdr.h |  1 +
 net/sunrpc/xdr.c           | 17 +++++++++++++----
 4 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
index 20aa5e746497..8786a27fe731 100644
--- a/fs/nfs/nfs42xdr.c
+++ b/fs/nfs/nfs42xdr.c
@@ -789,6 +789,11 @@ static void nfs4_xdr_enc_read_plus(struct rpc_rqst *req,
 
 	rpc_prepare_reply_pages(req, args->pages, args->pgbase, args->count,
 				hdr.replen - READ_PLUS_SEGMENT_SIZE_DIFF);
+	printk(KERN_INFO "  %s(hdr=%px, decode_read_plus_maxsz=%d)\n"
+		"    xdr->buf = {head->iov_len=%zd, page_base=%u, page_len=%u}\n",
+		__func__, container_of(args, struct nfs_pgio_header, args),
+		decode_read_plus_maxsz, xdr->buf->head->iov_len,
+		xdr->buf->page_base, xdr->buf->page_len);
 	encode_nops(&hdr);
 }
 
@@ -1061,10 +1066,12 @@ static inline uint64_t read_plus_segment_length(struct read_plus_segment *seg)
 }
 
 static int decode_read_plus_segment(struct xdr_stream *xdr,
-				    struct read_plus_segment *seg)
+				    struct read_plus_segment *seg,
+				    unsigned int n)
 {
 	__be32 *p;
 
+	printk(KERN_INFO "    %s(n=%u)\n", __func__, n);
 	p = xdr_inline_decode(xdr, 4);
 	if (!p)
 		return -EIO;
@@ -1079,6 +1086,8 @@ static int decode_read_plus_segment(struct xdr_stream *xdr,
 		struct xdr_buf buf;
 		uint32_t len = be32_to_cpup(p);
 
+		printk(KERN_INFO "      %s(n=%u): DATA {offset=%lld, length=%u}\n",
+			__func__, n, seg->offset, len);
 		seg->data.length = len;
 		seg->data.from = xdr_stream_pos(xdr);
 
@@ -1086,8 +1095,11 @@ static int decode_read_plus_segment(struct xdr_stream *xdr,
 			return -EIO;
 	} else if (seg->type == NFS4_CONTENT_HOLE) {
 		xdr_decode_hyper(p, &seg->hole.length);
+		printk(KERN_INFO "      %s(n=%u): HOLE {offset=%lld, length=%llu}\n",
+			__func__, n, seg->offset, seg->hole.length);
 	} else
 		return -EINVAL;
+	printk(KERN_INFO "      %s(n=%u): xdr->nwords = %d", __func__, n, xdr->nwords);
 	return 0;
 }
 
@@ -1149,7 +1161,7 @@ static int decode_read_plus(struct xdr_stream *xdr, struct nfs_pgio_res *res)
 		return -ENOMEM;
 
 	for (i = 0; i < segments; i++) {
-		status = decode_read_plus_segment(xdr, &segs[i]);
+		status = decode_read_plus_segment(xdr, &segs[i], i);
 		if (status < 0)
 			goto out;
 	}
@@ -1351,6 +1363,8 @@ static int nfs4_xdr_dec_read_plus(struct rpc_rqst *rqstp,
 	struct compound_hdr hdr;
 	int status;
 
+	printk(KERN_INFO "  %s(hdr=%px, scratch=%px)\n", __func__,
+		container_of(res, struct nfs_pgio_header, res), res->scratch);
 	xdr_set_scratch_buffer(xdr, res->scratch, READ_PLUS_SCRATCH_SIZE);
 
 	status = decode_compound_hdr(xdr, &hdr);
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 7dc21a48e3e7..0d8c86be4242 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -47,8 +47,11 @@ static struct nfs_pgio_header *nfs_readhdr_alloc(void)
 
 static void nfs_readhdr_free(struct nfs_pgio_header *rhdr)
 {
-	if (rhdr->res.scratch != NULL)
+	if (rhdr->res.scratch != NULL) {
+		printk(KERN_INFO "  %s(hdr=%px, scratch=%px)\n",
+			__func__, rhdr, rhdr->res.scratch);
 		kfree(rhdr->res.scratch);
+	}
 	kmem_cache_free(nfs_rdata_cachep, rhdr);
 }
 
@@ -114,6 +117,9 @@ bool nfs_read_alloc_scratch(struct nfs_pgio_header *hdr, size_t size)
 {
 	WARN_ON(hdr->res.scratch != NULL);
 	hdr->res.scratch = kmalloc(size, GFP_KERNEL);
+	printk(KERN_INFO "\n");
+	printk(KERN_INFO "%s(hdr=%px, size=%zd) = %px\n",
+		__func__, hdr, size, hdr->res.scratch);
 	return hdr->res.scratch != NULL;
 }
 EXPORT_SYMBOL_GPL(nfs_read_alloc_scratch);
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index d917618a3058..1c9a54e9efac 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -286,6 +286,7 @@ extern unsigned int xdr_stream_zero(struct xdr_stream *xdr, unsigned int offset,
 static inline void
 xdr_set_scratch_buffer(struct xdr_stream *xdr, void *buf, size_t buflen)
 {
+	WARN_ON(buf != NULL && xdr->scratch.iov_base != NULL);
 	xdr->scratch.iov_base = buf;
 	xdr->scratch.iov_len = buflen;
 }
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 391b336d97de..ff8406e668b2 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1416,12 +1416,17 @@ void xdr_init_decode_pages(struct xdr_stream *xdr, struct xdr_buf *buf,
 }
 EXPORT_SYMBOL_GPL(xdr_init_decode_pages);
 
-static __be32 * __xdr_inline_decode(struct xdr_stream *xdr, size_t nbytes)
+static __be32 * __xdr_inline_decode(struct xdr_stream *xdr, size_t nbytes,
+				    bool debug_print)
 {
 	unsigned int nwords = XDR_QUADLEN(nbytes);
 	__be32 *p = xdr->p;
 	__be32 *q = p + nwords;
 
+	if (unlikely(debug_print))
+		printk(KERN_INFO "        %s(nbytes=%zd, nwords=%d, xdr, q=%px)\n"
+			"          xdr = {nwords=%u, p=%px, end=%px}\n",
+	 		__func__, nbytes, nwords, q, xdr->nwords, xdr->p, xdr->end);
 	if (unlikely(nwords > xdr->nwords || q > xdr->end || q < p))
 		return NULL;
 	xdr->p = q;
@@ -1437,17 +1442,21 @@ static __be32 *xdr_copy_to_scratch(struct xdr_stream *xdr, size_t nbytes)
 
 	if (nbytes > xdr->scratch.iov_len)
 		goto out_overflow;
-	p = __xdr_inline_decode(xdr, cplen);
+	p = __xdr_inline_decode(xdr, cplen, true);
 	if (p == NULL)
 		return NULL;
+	printk(KERN_INFO "      %s(%d): memcpy(%px, %px, %zd)\n",
+		__func__, __LINE__, cpdest, p, cplen);
 	memcpy(cpdest, p, cplen);
 	if (!xdr_set_next_buffer(xdr))
 		goto out_overflow;
 	cpdest += cplen;
 	nbytes -= cplen;
-	p = __xdr_inline_decode(xdr, nbytes);
+	p = __xdr_inline_decode(xdr, nbytes, true);
 	if (p == NULL)
 		return NULL;
+	printk(KERN_INFO "      %s(%d): memcpy(%px, %px, %zd)\n",
+		__func__, __LINE__, cpdest, p, nbytes);
 	memcpy(cpdest, p, nbytes);
 	return xdr->scratch.iov_base;
 out_overflow:
@@ -1473,7 +1482,7 @@ __be32 * xdr_inline_decode(struct xdr_stream *xdr, size_t nbytes)
 		return xdr->p;
 	if (xdr->p == xdr->end && !xdr_set_next_buffer(xdr))
 		goto out_overflow;
-	p = __xdr_inline_decode(xdr, nbytes);
+	p = __xdr_inline_decode(xdr, nbytes, false);
 	if (p != NULL)
 		return p;
 	return xdr_copy_to_scratch(xdr, nbytes);
-- 
2.41.0


[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux