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

[Anna Schumaker]

Hi Krzysztof,

On Sat, Jun 10, 2023 at 6:15 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 06/03/2023 18:09, Anna Schumaker wrote:
> > Hi Krzysztof,
> >
> > On Tue, Feb 14, 2023 at 6:02 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>
> >> On 12/02/2023 15:05, Anna Schumaker wrote:
> >>>>> From ac2d6c501dbcdb306480edaee625b5496f1fb4f5 Mon Sep 17 00:00:00 2001
> >>>>> From: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
> >>>>> Date: Fri, 10 Feb 2023 15:50:22 -0500
> >>>>> Subject: [PATCH] NFSv4.2: Rework scratch handling for READ_PLUS
> >>>>>
> >>>>
> >>>> Patch is corrupted - maybe mail program reformatted it when sending:
> >>>>
> >>>> Applying: NFSv4.2: Rework scratch handling for READ_PLUS
> >>>> error: corrupt patch at line 12
> >>>> Patch failed at 0001 NFSv4.2: Rework scratch handling for READ_PLUS
> >>>
> >>> That's weird. I wasn't expecting gmail to reformat the patch but I
> >>> guess it did. I've added it as an attachment so that shouldn't happen
> >>> again.
> >>
> >> 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.

Thanks,
Anna

>
> Best regards,
> Krzysztof
>
From 985248165e53a67e7cb6a18ec2813aba8b26da33 Mon Sep 17 00:00:00 2001
From: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
Date: Wed, 14 Jun 2023 16:49:37 -0400
Subject: [RFC] 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 | 7 +++++++
 fs/nfs/read.c     | 8 +++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
index 20aa5e746497..6f63c816b0a5 100644
--- a/fs/nfs/nfs42xdr.c
+++ b/fs/nfs/nfs42xdr.c
@@ -1351,21 +1351,28 @@ 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);
 
+	printk(KERN_INFO "    buf = {%px, %zd}\n", xdr->scratch.iov_base, xdr->scratch.iov_len);
 	status = decode_compound_hdr(xdr, &hdr);
 	if (status)
 		goto out;
+	printk(KERN_INFO "    buf = {%px, %zd}\n", xdr->scratch.iov_base, xdr->scratch.iov_len);
 	status = decode_sequence(xdr, &res->seq_res, rqstp);
 	if (status)
 		goto out;
+	printk(KERN_INFO "    buf = {%px, %zd}\n", xdr->scratch.iov_base, xdr->scratch.iov_len);
 	status = decode_putfh(xdr);
 	if (status)
 		goto out;
+	printk(KERN_INFO "    buf = {%px, %zd}\n", xdr->scratch.iov_base, xdr->scratch.iov_len);
 	status = decode_read_plus(xdr, res);
 	if (!status)
 		status = res->count;
 out:
+	printk(KERN_INFO "    buf = {%px, %zd}\n", xdr->scratch.iov_base, xdr->scratch.iov_len);
 	return status;
 }
 
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);
-- 
2.41.0