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

[Anna Schumaker]

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.

Thanks,
Anna

>
> Anna
> >
> > Best regards,
> > Krzysztof
> >
From d41f4304007d2954f8513f3c3d845028fefe79ec Mon Sep 17 00:00:00 2001
From: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
Date: Wed, 14 Jun 2023 16:49:37 -0400
Subject: [RFC v2] NFS: Add debugging printk()s to trace the xdr->scratch
 buffer

I'm trying to figure out at what point the xdr->scratch buffer is
allocated, freed, set, and reset to figure out why READ_PLUS suddenly
thinks it's a NULL pointer with length 16.

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

diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
index 20aa5e746497..6a4eade2400b 100644
--- a/fs/nfs/nfs42xdr.c
+++ b/fs/nfs/nfs42xdr.c
@@ -1351,6 +1351,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..7b93316a52de 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..37c8f7e519dd 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1440,6 +1440,8 @@ static __be32 *xdr_copy_to_scratch(struct xdr_stream *xdr, size_t nbytes)
 	p = __xdr_inline_decode(xdr, cplen);
 	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;
@@ -1448,6 +1450,8 @@ static __be32 *xdr_copy_to_scratch(struct xdr_stream *xdr, size_t nbytes)
 	p = __xdr_inline_decode(xdr, nbytes);
 	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:
-- 
2.41.0