Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230717205239.921002-1-anna@kernel.org> <20230717205239.921002-5-anna@kernel.org>
 <ZLabtlXdGw4p4Q4Z@bazille.1015granger.net>
In-Reply-To: <ZLabtlXdGw4p4Q4Z@bazille.1015granger.net>
From:   Anna Schumaker <anna@kernel.org>
Date:   Tue, 18 Jul 2023 10:22:23 -0400
Message-ID: <CAFX2JfmZ2JFRzdzLTTVWDYnwveQdeX0kWVY327z20V41zVTo1g@mail.gmail.com>
Subject: Re: [PATCH v5 4/5] SUNRPC: kmap() the xdr pages during decode
To:     Chuck Lever <cel@kernel.org>
Cc:     linux-nfs@vger.kernel.org, trond.myklebust@hammerspace.com,
        krzysztof.kozlowski@linaro.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jul 18, 2023 at 10:03=E2=80=AFAM 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 o=
f
> > 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 **page=
s,
> >               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 *bu=
f,
> > 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 =3D rpc_exit_task;
> >               task->tk_status =3D rpcauth_unwrap_resp(task, &xdr);
> > +             xdr_stream_unmap_current_page(&xdr);
> >               return;
> >       case -EAGAIN:
> >               task->tk_status =3D 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 =3D 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 !=3D 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 =3D 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 xd=
r_stream *xdr,
> >       if (len > maxlen)
> >               len =3D maxlen;
> >
> > +     xdr_stream_unmap_current_page(xdr);
> >       xdr_stream_page_set_pos(xdr, base);
> >       base +=3D xdr->buf->page_base;
> >
> >       pgnr =3D base >> PAGE_SHIFT;
> >       xdr->page_ptr =3D &xdr->buf->pages[pgnr];
> > -     kaddr =3D page_address(*xdr->page_ptr);
> > +
> > +     if (PageHighMem(*xdr->page_ptr)) {
> > +             xdr->page_kaddr =3D kmap_local_page(*xdr->page_ptr);
> > +             kaddr =3D xdr->page_kaddr;
> > +     } else
> > +             kaddr =3D page_address(*xdr->page_ptr);
> >
> >       pgoff =3D base & ~PAGE_MASK;
> >       xdr->p =3D (__be32*)(kaddr + pgoff);
> > @@ -1382,6 +1396,7 @@ void xdr_init_decode(struct xdr_stream *xdr, stru=
ct xdr_buf *buf, __be32 *p,
> >                    struct rpc_rqst *rqst)
> >  {
> >       xdr->buf =3D buf;
> > +     xdr->page_kaddr =3D NULL;
> >       xdr_reset_scratch_buffer(xdr);
> >       xdr->nwords =3D XDR_QUADLEN(buf->len);
> >       if (xdr_set_iov(xdr, buf->head, 0, buf->len) =3D=3D 0 &&
> > --
> > 2.41.0
> >