Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [PATCH v5] xprtrdma: Fix XDRBUF_SPARSE_PAGES support
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <CAN-5tyGPzJUSSb3SGixo0L+CcFV=A12Er5+R=egc3orA9rz8Aw@mail.gmail.com>
Date:   Wed, 9 Dec 2020 11:58:36 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <E59C0E34-2172-4DBE-AD51-E57129FAE9E1@oracle.com>
References: <160746979784.1926.1490061321200284214.stgit@manet.1015granger.net>
 <CAN-5tyGPzJUSSb3SGixo0L+CcFV=A12Er5+R=egc3orA9rz8Aw@mail.gmail.com>
To:     Olga Kornievskaia <aglo@umich.edu>
Precedence: bulk


> On Dec 9, 2020, at 11:47 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> On Tue, Dec 8, 2020 at 6:31 PM Chuck Lever <chuck.lever@oracle.com> =
wrote:
>>=20
>> Olga K. observed that rpcrdma_marsh_req() allocates sparse pages
>> only when it has determined that a Reply chunk is necessary. There
>> are plenty of cases where no Reply chunk is needed, but the
>> XDRBUF_SPARSE_PAGES flag is set. The result would be a crash in
>> rpcrdma_inline_fixup() when it tries to copy parts of the received
>> Reply into a missing page.
>>=20
>> To avoid crashing, handle sparse page allocation up front.
>>=20
>> Until XATTR support was added, this issue did not appear often
>> because the only SPARSE_PAGES consumer always expected a reply large
>> enough to always require a Reply chunk.
>>=20
>> Reported-by: Olga Kornievskaia <kolga@netapp.com>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>> net/sunrpc/xprtrdma/rpc_rdma.c |   40 =
+++++++++++++++++++++++++++++++---------
>> 1 file changed, 31 insertions(+), 9 deletions(-)
>>=20
>> Hi-
>>=20
>> v4 had a bug, which I've fixed. This version has been tested.
>=20
> This version on top of the same commit  (rc4) passes generic/013
> without oopsing for me too.

Excellent, thanks for confirming!


>> In kernels before 5.10-rc5, there are still problems with the way
>> LISTXATTRS and GETXATTR deal with the tail / XDR pad for the page
>> content that this patch does not address. So backporting this fix
>> alone is not enough to get those working again -- more surgery would
>> be required.
>>=20
>> Since none of the other SPARSE_PAGES users have a problem, let's
>> leave this one on the cutting room floor. It's here in the mail
>> archive if anyone needs it.
>>=20
>>=20
>> Changes since v4:
>> - xdr_buf_pagecount() was simply the wrong thing to use.
>>=20
>> Changes since v3:
>> - I swear I am not drunk. I forgot to commit the change before =
mailing it.
>>=20
>> Changes since v2:
>> - Actually fix the xdr_buf_pagecount() problem
>>=20
>> Changes since RFC:
>> - Ensure xdr_buf_pagecount() is defined in rpc_rdma.c
>> - noinline the sparse page allocator -- it's an uncommon path
>>=20
>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c =
b/net/sunrpc/xprtrdma/rpc_rdma.c
>> index 0f5120c7668f..c48536f2121f 100644
>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>> @@ -179,6 +179,31 @@ rpcrdma_nonpayload_inline(const struct =
rpcrdma_xprt *r_xprt,
>>                r_xprt->rx_ep->re_max_inline_recv;
>> }
>>=20
>> +/* ACL likes to be lazy in allocating pages. For TCP, these
>> + * pages can be allocated during receive processing. Not true
>> + * for RDMA, which must always provision receive buffers
>> + * up front.
>> + */
>> +static noinline int
>> +rpcrdma_alloc_sparse_pages(struct xdr_buf *buf)
>> +{
>> +       struct page **ppages;
>> +       int len;
>> +
>> +       len =3D buf->page_len;
>> +       ppages =3D buf->pages + (buf->page_base >> PAGE_SHIFT);
>> +       while (len > 0) {
>> +               if (!*ppages)
>> +                       *ppages =3D alloc_page(GFP_NOWAIT | =
__GFP_NOWARN);
>> +               if (!*ppages)
>> +                       return -ENOBUFS;
>> +               ppages++;
>> +               len -=3D PAGE_SIZE;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> /* Split @vec on page boundaries into SGEs. FMR registers pages, not
>>  * a byte range. Other modes coalesce these SGEs into a single MR
>>  * when they can.
>> @@ -233,15 +258,6 @@ rpcrdma_convert_iovs(struct rpcrdma_xprt =
*r_xprt, struct xdr_buf *xdrbuf,
>>        ppages =3D xdrbuf->pages + (xdrbuf->page_base >> PAGE_SHIFT);
>>        page_base =3D offset_in_page(xdrbuf->page_base);
>>        while (len) {
>> -               /* ACL likes to be lazy in allocating pages - ACLs
>> -                * are small by default but can get huge.
>> -                */
>> -               if (unlikely(xdrbuf->flags & XDRBUF_SPARSE_PAGES)) {
>> -                       if (!*ppages)
>> -                               *ppages =3D alloc_page(GFP_NOWAIT | =
__GFP_NOWARN);
>> -                       if (!*ppages)
>> -                               return -ENOBUFS;
>> -               }
>>                seg->mr_page =3D *ppages;
>>                seg->mr_offset =3D (char *)page_base;
>>                seg->mr_len =3D min_t(u32, PAGE_SIZE - page_base, =
len);
>> @@ -867,6 +883,12 @@ rpcrdma_marshal_req(struct rpcrdma_xprt *r_xprt, =
struct rpc_rqst *rqst)
>>        __be32 *p;
>>        int ret;
>>=20
>> +       if (unlikely(rqst->rq_rcv_buf.flags & XDRBUF_SPARSE_PAGES)) {
>> +               ret =3D =
rpcrdma_alloc_sparse_pages(&rqst->rq_rcv_buf);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>>        rpcrdma_set_xdrlen(&req->rl_hdrbuf, 0);
>>        xdr_init_encode(xdr, &req->rl_hdrbuf, =
rdmab_data(req->rl_rdmabuf),
>>                        rqst);

--
Chuck Lever