Hi Krzysztof,
On Wed, Jun 21, 2023 at 9:27 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 21/06/2023 14:49, Anna Schumaker wrote:
> > On Sat, Jun 17, 2023 at 6:09 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>
> >> On 15/06/2023 21:38, Anna Schumaker wrote:
> >>> 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..
> >>>
> >>
> >> Here you have - attached.
> >
> > This is good, thanks! I was finally able to figure out how to hit the
> > bug using a 32bit x86 VM, so hopefully the next thing you hear from me
> > is a patch fixing the bug!
I'm really hopeful that the attached patch finally fixes the issue.
Can you try it and let me know?
Thanks,
Anna
>
> QEMU also has 32-bit ARM and x86...
>
> Best regards,
> Krzysztof
>
From 88be621fc687d85891636ed4695ab993e1f2cb3b Mon Sep 17 00:00:00 2001
From: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
Date: Fri, 23 Jun 2023 11:43:14 -0400
Subject: [PATCH] SUNRPC: kmap() the xdr pages during decode
If the pages are in HIGHMEM then we need to make sure they're mapped
before trying to read data off of them, otherwise we could end up with a
NULL pointer dereference.
Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
Signed-off-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
---
include/linux/sunrpc/xdr.h | 2 ++
net/sunrpc/clnt.c | 1 +
net/sunrpc/xdr.c | 20 ++++++++++++++------
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index d917618a3058..f562aab468f5 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -228,6 +228,7 @@ struct xdr_stream {
struct kvec *iov; /* pointer to the current kvec */
struct kvec scratch; /* Scratch buffer */
struct page **page_ptr; /* pointer to the current page */
+ void *page_kaddr; /* kmapped address of the current page */
unsigned int nwords; /* Remaining decode buffer length */
struct rpc_rqst *rqst; /* For debugging */
@@ -254,6 +255,7 @@ extern void xdr_truncate_decode(struct xdr_stream *xdr, size_t len);
extern int xdr_restrict_buflen(struct xdr_stream *xdr, int newbuflen);
extern void xdr_write_pages(struct xdr_stream *xdr, struct page **pages,
unsigned int base, unsigned int len);
+extern void xdr_stream_unmap_current_page(struct xdr_stream *xdr);
extern unsigned int xdr_stream_pos(const struct xdr_stream *xdr);
extern unsigned int xdr_page_pos(const struct xdr_stream *xdr);
extern void xdr_init_decode(struct xdr_stream *xdr, struct xdr_buf *buf,
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index d2ee56634308..3b7e676d8935 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2590,6 +2590,7 @@ call_decode(struct rpc_task *task)
case 0:
task->tk_action = rpc_exit_task;
task->tk_status = rpcauth_unwrap_resp(task, &xdr);
+ xdr_stream_unmap_current_page(&xdr);
return;
case -EAGAIN:
task->tk_status = 0;
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 391b336d97de..b073f51e6018 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1308,6 +1308,14 @@ static unsigned int xdr_set_tail_base(struct xdr_stream *xdr,
return xdr_set_iov(xdr, buf->tail, base, len);
}
+void xdr_stream_unmap_current_page(struct xdr_stream *xdr)
+{
+ if (xdr->page_kaddr) {
+ kunmap_local(xdr->page_kaddr);
+ xdr->page_kaddr = NULL;
+ }
+}
+
static unsigned int xdr_set_page_base(struct xdr_stream *xdr,
unsigned int base, unsigned int len)
{
@@ -1315,7 +1323,6 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr,
unsigned int maxlen;
unsigned int pgoff;
unsigned int pgend;
- void *kaddr;
maxlen = xdr->buf->page_len;
if (base >= maxlen)
@@ -1330,15 +1337,15 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr,
pgnr = base >> PAGE_SHIFT;
xdr->page_ptr = &xdr->buf->pages[pgnr];
- kaddr = page_address(*xdr->page_ptr);
+ xdr->page_kaddr = kmap_local_page(*xdr->page_ptr);
pgoff = base & ~PAGE_MASK;
- xdr->p = (__be32*)(kaddr + pgoff);
+ xdr->p = (__be32*)(xdr->page_kaddr + pgoff);
pgend = pgoff + len;
if (pgend > PAGE_SIZE)
pgend = PAGE_SIZE;
- xdr->end = (__be32*)(kaddr + pgend);
+ xdr->end = (__be32*)(xdr->page_kaddr + pgend);
xdr->iov = NULL;
return len;
}
@@ -1366,9 +1373,10 @@ static void xdr_set_next_page(struct xdr_stream *xdr)
static bool xdr_set_next_buffer(struct xdr_stream *xdr)
{
- if (xdr->page_ptr != NULL)
+ if (xdr->page_ptr != NULL) {
+ xdr_stream_unmap_current_page(xdr);
xdr_set_next_page(xdr);
- else if (xdr->iov == xdr->buf->head)
+ } else if (xdr->iov == xdr->buf->head)
xdr_set_page(xdr, 0, xdr_stream_remaining(xdr));
return xdr->p != xdr->end;
}
--
2.41.0
