Re: [PATCH 5/8] SUNRPC: Don't truncate tail in xdr_inline_pages()

From: Chuck Lever <chuck.lever@oracle.com>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "trondmy@kernel.org" <trondmy@kernel.org>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 5/8] SUNRPC: Don't truncate tail in xdr_inline_pages()
Date: Mon, 23 Nov 2020 09:52:53 -0500	[thread overview]
Message-ID: <4C120984-5A7B-4245-9B04-8E44C4370BC1@oracle.com> (raw)
In-Reply-To: <614498c69c40421f8581fd8b25633e8668959581.camel@hammerspace.com>

> On Nov 22, 2020, at 11:29 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Sun, 2020-11-22 at 20:24 -0500, Chuck Lever wrote:
>> 
>> 
>>> On Nov 22, 2020, at 3:52 PM, trondmy@kernel.org wrote:
>>> 
>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> 
>>> True that if the length of the pages[] array is not 4-byte aligned,
>>> then
>>> we will need to store the padding in the tail, but there is no need
>>> to
>>> truncate the total buffer length here.
>> 
>> This description confuses me. The existing code reduces the length of
>> the tail, not the "total buffer length." And what the removed logic
>> is
>> doing is taking out the length of the XDR pad for the pages array
>> when
>> it is not expected to be used.
> 
> Why are we bothering to do that? There is nothing problematic with just
> ignoring this test and leaving the tail length as it is, nor is there
> anything to be gained by applying it.

You are correct that leaving the buffer a little long is not going
to harm normal operation. After all, we lived with a wildly over-
estimated slack length for years.

The purpose of this code path is to prepare the receive buffer with
the memory resources and expected length of the Reply. The series
of patches that introduced this particular change was all about
ensuring that the estimated length of the reply message was exact.

If the reply message size is overestimated, that moves the end-of-
message sentinel that is later set by xdr_init_decode(). We then
miss subtle problems like our fixed size estimates are incorrect
or a man-in-the-middle is extending the RPC message or the server
is malfunctioning.

<scratches chin>

After moving the ->pages pad into ->pages, I'm wondering if you
should revert 02ef04e432ba ("NFS: Account for XDR pad of buf->pages") --
the maxsz macros don't need to account for the XDR pad of ->pages
any more. Then the below hunk makes sense. The patch description
still doesn't, though ;-)

And then you should confirm that we are still getting the receive
buffer size estimate right for krb5i and krb5p.

>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> ---
>>> net/sunrpc/xdr.c | 3 ---
>>> 1 file changed, 3 deletions(-)
>>> 
>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>>> index 3ce0a5daa9eb..5a450055469f 100644
>>> --- a/net/sunrpc/xdr.c
>>> +++ b/net/sunrpc/xdr.c
>>> @@ -193,9 +193,6 @@ xdr_inline_pages(struct xdr_buf *xdr, unsigned
>>> int offset,
>>> 
>>>         tail->iov_base = buf + offset;
>>>         tail->iov_len = buflen - offset;
>>> -       if ((xdr->page_len & 3) == 0)
>>> -               tail->iov_len -= sizeof(__be32);
>>> -
>>>         xdr->buflen += len;
>>> }
>>> EXPORT_SYMBOL_GPL(xdr_inline_pages);
>>> -- 
>>> 2.28.0
>>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com

--
Chuck Lever