On Tue, Jul 18, 2023 at 10:03 AM Chuck Lever <cel@xxxxxxxxxx> wrote:
>
> On Mon, Jul 17, 2023 at 04:52:38PM -0400, Anna Schumaker wrote:
> > From: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
> >
> > 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.
> >
> > The downside to this is that we need an extra cleanup step at the end of
> > decode to kunmap() the last page.
> >
> > Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > Signed-off-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
> > ---
> > v5: Also clean up kmapped pages on the server
> > ---
> > include/linux/sunrpc/xdr.h | 2 ++
> > net/sunrpc/clnt.c | 1 +
> > net/sunrpc/svc.c | 2 ++
> > net/sunrpc/xdr.c | 17 ++++++++++++++++-
> > 4 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> > index 6bffd10b7a33..60ddad33b49b 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 */
> > @@ -253,6 +254,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);
>
> xdr_stream_unmap_current_page() is now effectively a matching
> book-end for xdr_init_decode(). I think it would be more clear
> (for human readers) if the name matched that organization rather
> than being about the one specific thing it happens to be doing
> now.
>
> Something like xdr_finish_decode() ?
I like xdr_finish_decode() much better! I'll rename that
Thanks,
Anna
>
>
> > 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 d7c697af3762..8080a1830ff3 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -2602,6 +2602,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/svc.c b/net/sunrpc/svc.c
> > index 587811a002c9..5f32817579db 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -1370,6 +1370,8 @@ svc_process_common(struct svc_rqst *rqstp)
> > rc = process.dispatch(rqstp);
> > if (procp->pc_release)
> > procp->pc_release(rqstp);
> > + xdr_stream_unmap_current_page(xdr);
> > +
> > if (!rc)
> > goto dropit;
> > if (rqstp->rq_auth_stat != rpc_auth_ok)
> > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > index 94bddd1dd1d7..2b972954327f 100644
> > --- a/net/sunrpc/xdr.c
> > +++ b/net/sunrpc/xdr.c
> > @@ -1306,6 +1306,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)
> > {
> > @@ -1323,12 +1331,18 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr,
> > if (len > maxlen)
> > len = maxlen;
> >
> > + xdr_stream_unmap_current_page(xdr);
> > xdr_stream_page_set_pos(xdr, base);
> > base += xdr->buf->page_base;
> >
> > pgnr = base >> PAGE_SHIFT;
> > xdr->page_ptr = &xdr->buf->pages[pgnr];
> > - kaddr = page_address(*xdr->page_ptr);
> > +
> > + if (PageHighMem(*xdr->page_ptr)) {
> > + xdr->page_kaddr = kmap_local_page(*xdr->page_ptr);
> > + kaddr = xdr->page_kaddr;
> > + } else
> > + kaddr = page_address(*xdr->page_ptr);
> >
> > pgoff = base & ~PAGE_MASK;
> > xdr->p = (__be32*)(kaddr + pgoff);
> > @@ -1382,6 +1396,7 @@ void xdr_init_decode(struct xdr_stream *xdr, struct xdr_buf *buf, __be32 *p,
> > struct rpc_rqst *rqst)
> > {
> > xdr->buf = buf;
> > + xdr->page_kaddr = NULL;
> > xdr_reset_scratch_buffer(xdr);
> > xdr->nwords = XDR_QUADLEN(buf->len);
> > if (xdr_set_iov(xdr, buf->head, 0, buf->len) == 0 &&
> > --
> > 2.41.0
> >
