diff mbox series

[v5,4/5] SUNRPC: kmap() the xdr pages during decode

Message ID 20230717205239.921002-5-anna@kernel.org (mailing list archive)
State New, archived
Headers show
Series NFSv4.2: Various READ_PLUS fixes | expand

Commit Message

Anna Schumaker July 17, 2023, 8:52 p.m. UTC 
From: Anna Schumaker <Anna.Schumaker@Netapp.com>

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@linaro.org>
Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
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(-)

Comments

Chuck Lever July 18, 2023, 2:03 p.m. UTC | #1 
On Mon, Jul 17, 2023 at 04:52:38PM -0400, Anna Schumaker wrote:
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> 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@linaro.org>
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
> 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() ?


>  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
>
Anna Schumaker July 18, 2023, 2:22 p.m. UTC | #2 
On Tue, Jul 18, 2023 at 10:03 AM Chuck Lever <cel@kernel.org> wrote:
>
> On Mon, Jul 17, 2023 at 04:52:38PM -0400, Anna Schumaker wrote:
> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >
> > 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@linaro.org>
> > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > ---
> > 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
> >
diff mbox series

Patch

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);
 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 &&