Re: Regression: NULL pointer dereference after NFS_V4_2_READ_PLUS (commit 7fd461c47)
From: Anna Schumaker
Date: Thu Jun 15 2023 - 15:39:10 EST
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