From: Anna Schumaker <[email protected]>
These patches are a handul of fixes I've done recently to the READ_PLUS
code. This includes fixing some smatch warnings, fixing the XDR reply
length calculation, improving scratch buffer handling, and having
xdr_inline_decode() kmap pages if we detect that they're HIGHMEM so we
don't oops during READ_PLUS xdr decoding.
Thoughts? Patch #4 probably needs the most review, and I'm open to other
approaches if something else makes more sense!
Thanks,
Anna
Anna Schumaker (4):
NFSv4.2: Fix READ_PLUS smatch warnings
NFSv4.2: Fix READ_PLUS size calculations
NFSv4.2: Rework scratch handling for READ_PLUS (again)
SUNRPC: kmap() the xdr pages during decode
fs/nfs/internal.h | 1 +
fs/nfs/nfs42.h | 1 +
fs/nfs/nfs42xdr.c | 17 +++++++++++------
fs/nfs/nfs4proc.c | 13 +------------
fs/nfs/read.c | 10 ++++++++++
include/linux/sunrpc/xdr.h | 2 ++
net/sunrpc/clnt.c | 1 +
net/sunrpc/xdr.c | 17 ++++++++++++++++-
8 files changed, 43 insertions(+), 19 deletions(-)
--
2.41.0
From: Anna Schumaker <[email protected]>
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 <[email protected]>
Signed-off-by: Anna Schumaker <[email protected]>
---
include/linux/sunrpc/xdr.h | 2 ++
net/sunrpc/clnt.c | 1 +
net/sunrpc/xdr.c | 17 ++++++++++++++++-
3 files changed, 19 insertions(+), 1 deletion(-)
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..fb5203337608 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)
{
@@ -1325,12 +1333,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);
@@ -1384,6 +1398,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
On Fri, Jun 30, 2023 at 04:42:40PM -0400, Anna Schumaker wrote:
> From: Anna Schumaker <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Anna Schumaker <[email protected]>
> ---
> include/linux/sunrpc/xdr.h | 2 ++
> net/sunrpc/clnt.c | 1 +
> net/sunrpc/xdr.c | 17 ++++++++++++++++-
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
> 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);
The server uses xdr_inline_decode() as well. Wouldn't it also need
some kind of clean up?
I'm curious why this issue hasn't been a problem until now.
> return;
> case -EAGAIN:
> task->tk_status = 0;
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 391b336d97de..fb5203337608 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)
> {
> @@ -1325,12 +1333,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);
> @@ -1384,6 +1398,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
>