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 4] xprtrdma: Fix XDRBUF_SPARSE_PAGES support
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <CAN-5tyFAmWL_Y73pgnaBEP3y_QeFAogRqXjtzPhsOpTvZt_o8w@mail.gmail.com>
Date:   Tue, 8 Dec 2020 14:57:17 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <841D209E-2FE1-47A9-BFBB-C7EDA6D73CD1@oracle.com>
References: <160719971621.32538.11224487886769737849.stgit@manet.1015granger.net>
 <CAN-5tyFAmWL_Y73pgnaBEP3y_QeFAogRqXjtzPhsOpTvZt_o8w@mail.gmail.com>
To:     Olga Kornievskaia <aglo@umich.edu>
Precedence: bulk


> On Dec 8, 2020, at 2:49 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> On Sat, Dec 5, 2020 at 3:24 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
> Ok so given that ACL never utilizes this, the only way to test this is
> to remove the xattr fixes and try this and run the xfstest generic/013
> would that be correct?

I'm simply hoisting the SPARSE_PAGES logic out of convert_iovs and into
marshal_req, to ensure that it is visited no matter what chunk type is
being encoded. NFSv3 GETACL would use this new code path instead of the
old one, so it would still get exercised.=20


> If so, then just applying this patch on top of pure say -rc4 produces =
problems.

I don't follow you. What problems would occur? On -rc4, the LISTXATTRS
crash you found should go away after this patch is applied.


> This patch on top of all the fixes (getxattr + listxattr patches),
> seems ok but I'm not sure if it gets exercised.
>>=20
>> Reported-by: Olga Kornievskaia <kolga@netapp.com>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>> net/sunrpc/xdr.c               |    1 +
>> net/sunrpc/xprtrdma/rpc_rdma.c |   41 =
+++++++++++++++++++++++++++++++---------
>> 2 files changed, 33 insertions(+), 9 deletions(-)
>>=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/xdr.c b/net/sunrpc/xdr.c
>> index 71e03b930b70..878f4c4fec1a 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -141,6 +141,7 @@ xdr_buf_pagecount(struct xdr_buf *buf)
>>                return 0;
>>        return (buf->page_base + buf->page_len + PAGE_SIZE - 1) >> =
PAGE_SHIFT;
>> }
>> +EXPORT_SYMBOL_GPL(xdr_buf_pagecount);
>>=20
>> int
>> xdr_alloc_bvec(struct xdr_buf *buf, gfp_t gfp)
>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c =
b/net/sunrpc/xprtrdma/rpc_rdma.c
>> index 0f5120c7668f..6c9a1810a70a 100644
>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>> @@ -179,6 +179,32 @@ 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 rpc_rqst *rqst)
>> +{
>> +       struct xdr_buf *xb =3D &rqst->rq_rcv_buf;
>> +       struct page **ppages;
>> +       int len;
>> +
>> +       len =3D xb->page_len;
>> +       ppages =3D xb->pages + xdr_buf_pagecount(xb);
>> +       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 +259,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 +884,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);
>> +               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