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 v1] NFS: Fix rpcrdma_inline_fixup() crash with new
 LISTXATTRS operation
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20201124200640.GA2476@dev-dsk-fllinden-2c-c1893d73.us-west-2.amazon.com>
Date:   Tue, 24 Nov 2020 15:18:55 -0500
Cc:     Olga Kornievskaia <kolga@netapp.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <10EFE633-374C-4BB1-8877-424579564C55@oracle.com>
References: <160623862874.1534.4471924380357882531.stgit@manet.1015granger.net>
 <20201124200640.GA2476@dev-dsk-fllinden-2c-c1893d73.us-west-2.amazon.com>
To:     Frank van der Linden <fllinden@amazon.com>
Precedence: bulk


> On Nov 24, 2020, at 3:06 PM, Frank van der Linden =
<fllinden@amazon.com> wrote:
>=20
> On Tue, Nov 24, 2020 at 12:26:32PM -0500, Chuck Lever wrote:
>> CAUTION: This email originated from outside of the organization. Do =
not click links or open attachments unless you can confirm the sender =
and know the content is safe.
>>=20
>>=20
>>=20
>> By switching to an XFS-backed export, I am able to reproduce the
>> ibcomp worker crash on my client with xfstests generic/013.
>>=20
>> For the failing LISTXATTRS operation, xdr_inline_pages() is called
>> with page_len=3D12 and buflen=3D128. Then:
>>=20
>> - Because buflen is small, rpcrdma_marshal_req will not set up a
>>  Reply chunk and the rpcrdma's XDRBUF_SPARSE_PAGES logic does not
>>  get invoked at all.
>>=20
>> - Because page_len is non-zero, rpcrdma_inline_fixup() tries to
>>  copy received data into rq_rcv_buf->pages, but they're missing.
>>=20
>> The result is that the ibcomp worker faults and dies. Sometimes that
>> causes a visible crash, and sometimes it results in a transport
>> hang without other symptoms.
>>=20
>> RPC/RDMA's XDRBUF_SPARSE_PAGES support is not entirely correct, and
>> should eventually be fixed or replaced. However, my preference is
>> that upper-layer operations should explicitly allocate their receive
>> buffers (using GFP_KERNEL) when possible, rather than relying on
>> XDRBUF_SPARSE_PAGES.
>>=20
>> Reported-by: Olga kornievskaia <kolga@netapp.com>
>> Suggested-by: Olga kornievskaia <kolga@netapp.com>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfs/nfs42proc.c |   17 ++++++++++-------
>> fs/nfs/nfs42xdr.c  |    1 -
>> 2 files changed, 10 insertions(+), 8 deletions(-)
>>=20
>> Hi-
>>=20
>> I like Olga's proposed approach. What do you think of this patch?
>>=20
>>=20
>> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
>> index 2b2211d1234e..24810305ec1c 100644
>> --- a/fs/nfs/nfs42proc.c
>> +++ b/fs/nfs/nfs42proc.c
>> @@ -1241,7 +1241,7 @@ static ssize_t _nfs42_proc_listxattrs(struct =
inode *inode, void *buf,
>>                .rpc_resp       =3D &res,
>>        };
>>        u32 xdrlen;
>> -       int ret, np;
>> +       int ret, np, i;
>>=20
>>=20
>>        res.scratch =3D alloc_page(GFP_KERNEL);
>> @@ -1253,10 +1253,14 @@ static ssize_t _nfs42_proc_listxattrs(struct =
inode *inode, void *buf,
>>                xdrlen =3D server->lxasize;
>>        np =3D xdrlen / PAGE_SIZE + 1;
>>=20
>> +       ret =3D -ENOMEM;
>>        pages =3D kcalloc(np, sizeof(struct page *), GFP_KERNEL);
>> -       if (pages =3D=3D NULL) {
>> -               __free_page(res.scratch);
>> -               return -ENOMEM;
>> +       if (pages =3D=3D NULL)
>> +               goto out_free;
>> +       for (i =3D 0; i < np; i++) {
>> +               pages[i] =3D alloc_page(GFP_KERNEL);
>> +               if (!pages[i])
>> +                       goto out_free;
>>        }
>>=20
>>        arg.xattr_pages =3D pages;
>> @@ -1271,14 +1275,13 @@ static ssize_t _nfs42_proc_listxattrs(struct =
inode *inode, void *buf,
>>                *eofp =3D res.eof;
>>        }
>>=20
>> +out_free:
>>        while (--np >=3D 0) {
>>                if (pages[np])
>>                        __free_page(pages[np]);
>>        }
>> -
>> -       __free_page(res.scratch);
>>        kfree(pages);
>> -
>> +       __free_page(res.scratch);
>>        return ret;
>>=20
>> }
>> diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
>> index 6e060a88f98c..8432bd6b95f0 100644
>> --- a/fs/nfs/nfs42xdr.c
>> +++ b/fs/nfs/nfs42xdr.c
>> @@ -1528,7 +1528,6 @@ static void nfs4_xdr_enc_listxattrs(struct =
rpc_rqst *req,
>>=20
>>        rpc_prepare_reply_pages(req, args->xattr_pages, 0, =
args->count,
>>            hdr.replen);
>> -       req->rq_rcv_buf.flags |=3D XDRBUF_SPARSE_PAGES;
>>=20
>>        encode_nops(&hdr);
>> }
>>=20
>>=20
>=20
> I can see why this is the simplest and most pragmatic solution, so =
it's
> fine with me.

Thanks. I've added Olga's Reviewed/Tested-by to my local copy.
May I add a Reviewed-by from you?


> Why doesn't this happen with getxattr? Do we need to convert that too?

If a GETXATTR request can be generated such that buflen is less than
a page, but page_len > 0, then yes, it will happen there too. As Olga
says, NFS is unaware of the transport's inline threshold setting, so
there will always be some buflen value under which bad things happen.

Either way, I would prefer to see GETXATTR converted to avoid using
XDRBUF_SPARSE_PAGES. Does NFSv4 GETACL provide a good example?


> The basic issue here is that the RPC code does not deal with inlined =
data
> that exceeds PAGE_SIZE. That can only be done with raw pages.
>=20
> Since the upper layer has already allocated a buffer in the case of =
listxattr
> and getxattr, I would love to be able to just XDR code in to that =
buffer,
> and void the whole alloc+copy situation.

Do you mean like the READDIR entry encoders and decoders?


> But sadly, it might be > PAGE_SIZE,
> so the XDR code doesn't allow it. It's not all bad, having to use =
pages
> allows them to be directly hooked in to the cache in the case of =
getxattr,
> but for listxattr, decoding directly in to the provided buffer would =
be nice.
>=20
> Hm, I wonder if that restriction actually holds for listxattr - the =
invidual
> XDR items (xattr names) should never exceed PAGE_SIZE..

You'll have to worry about XDR data items crossing page boundaries.
Our XDR code uses a scratch buffer for that, so it can be handled
transparently.


--
Chuck Lever