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 12.4 \(3445.104.11\))
Subject: Re: GSS unwrapping breaks the DRC
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200415215823.GB6466@fieldses.org>
Date:   Wed, 15 Apr 2020 18:23:57 -0400
Cc:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <39815C35-EAD8-4B2E-B48F-88F3D5B10C57@oracle.com>
References: <DAED9EC8-7461-48FF-AD6C-C85FB968F8A6@oracle.com>
 <20200415192542.GA6466@fieldses.org>
 <0775FBE7-C2DD-4ED6-955D-22B944F302E0@oracle.com>
 <20200415215823.GB6466@fieldses.org>
To:     Bruce Fields <bfields@fieldses.org>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Apr 15, 2020, at 5:58 PM, Bruce Fields <bfields@fieldses.org> =
wrote:
>=20
> On Wed, Apr 15, 2020 at 04:06:17PM -0400, Chuck Lever wrote:
>>=20
>>=20
>>> On Apr 15, 2020, at 3:25 PM, Bruce Fields <bfields@fieldses.org> =
wrote:
>>>=20
>>> On Wed, Apr 15, 2020 at 01:05:11PM -0400, Chuck Lever wrote:
>>>> Hi Bruce and Jeff:
>>>>=20
>>>> Testing intensive workloads with NFSv3 and NFSv4.0 on NFS/RDMA with =
krb5i
>>>> or krb5p results in a pretty quick workload failure. Closer =
examination
>>>> shows that the client is able to overrun the GSS sequence window =
with
>>>> some regularity. When that happens, the server drops the =
connection.
>>>>=20
>>>> However, when the client retransmits requests with lost replies, =
they
>>>> never hit in the DRC, and that results in unexpected failures of =
non-
>>>> idempotent requests.
>>>>=20
>>>> The retransmitted XIDs are found in the DRC, but the retransmitted =
request
>>>> has a different checksum than the original. We're hitting the =
"mismatch"
>>>> case in nfsd_cache_key_cmp for these requests.
>>>>=20
>>>> I tracked down the problem to the way the DRC computes the length =
of the
>>>> part of the buffer it wants to checksum. nfsd_cache_csum uses
>>>>=20
>>>> head.iov_len + page_len
>>>>=20
>>>> and then caps that at RC_CSUMLEN.
>>>>=20
>>>> That works fine for krb5 and sys, but the GSS unwrap functions
>>>> (integ_unwrap_data and priv_unwrap_data) don't appear to update =
head.iov_len
>>>> properly. So nfsd_cache_csum's length computation is significantly =
larger
>>>> than the clear-text message, and that allows stale parts of the =
xdr_buf
>>>> to be included in the checksum.
>>>>=20
>>>> Using xdr_buf_subsegment() at the end of integ_unwrap_data sets the =
xdr_buf
>>>> lengths properly and fixes the situation for krb5i.
>>>>=20
>>>> I don't see a similar solution for priv_unwrap_data: there's no MIC =
len
>>>> available, and priv_len is not the actual length of the clear-text =
message.
>>>>=20
>>>> Moreover, the comment in fix_priv_head() is disturbing. I don't see =
anywhere
>>>> where the relationship between the buf's head/len and how svc_defer =
works is
>>>> authoritatively documented. It's not clear exactly how =
priv_unwrap_data is
>>>> supposed to accommodate svc_defer, or whether integ_unwrap_data =
also needs
>>>> to accommodate it.
>>>>=20
>>>> So I can't tell if the GSS unwrap functions are wrong or if there's =
a more
>>>> accurate way to compute the message length in nfsd_cache_csum. I =
suspect
>>>> both could use some improvement, but I'm not certain exactly what =
that
>>>> might be.
>>>=20
>>> I don't know, I tried looking through that code and didn't get any
>>> further than you.  The gss unwrap code does look suspect to me.  It
>>> needs some kind of proper design, as it stands it's just an =
accumulation
>>> of fixes.
>>=20
>> Having recently completed overhauling the client-side equivalents, I
>> agree with you there.
>>=20
>> I've now convinced myself that because nfsd_cache_csum might need to
>> advance into the first page of the Call message, rq_arg.head.iov_len
>> must contain an accurate length so that csum_partial is applied
>> correctly to the head buffer.
>>=20
>> Therefore it is the preceding code that needs to set up =
rq_arg.head.iov_len
>> correctly. The GSS unwrap functions have to do this, and therefore =
these
>> must be fixed. I would theorize that svc_defer also depends on =
head.iov_len
>> being set correctly.
>>=20
>> As far as how rq_arg.len needs to be set for svc_defer, I think I =
need
>> to have some kind of test case to examine how that path is triggered. =
Any
>> advice appreciated.
>=20
> It's triggered on upcalls, so for example if you flush the export =
caches
> with exports -f and then send an rpc with a filehandle, it should call
> svc_defer on that request.

/me puts a brown paper bag over his head

Reverting 241b1f419f0e ("SUNRPC: Remove xdr_buf_trim()") seems to fix =
both
krb5i and krb5p.

I'll post an official patch once I've done a little more testing. I =
promise
to get the Fixes: tag right :-)


--
Chuck Lever