Re: [RFC PATCH] fix krb5p mount not providing large enough buffer in rq_rcvsize

[Chuck Lever <chuck.lever@oracle.com>]

> On Mar 10, 2020, at 5:07 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> 
> Hi Chuck,
> 
> On Tue, Mar 10, 2020 at 3:57 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> Hi Olga-
>> 
>>> On Mar 10, 2020, at 2:58 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>> 
>>> Ever since commit 2c94b8eca1a26 "SUNRPC: Use au_rslack when computing
>>> reply buffer size". It changed how "req->rq_rcvsize" is calculated. It
>>> used to use au_cslack value which was nice and large and changed it to
>>> au_rslack value which turns out to be too small.
>>> 
>>> Since 5.1, v3 mount with sec=krb5p fails against an Ontap server
>>> because client's receive buffer it too small.
>> 
>> Can you be more specific? For instance, why is 100 bytes adequate for
>> Linux servers, but not OnTAP?
> 
> I don't know why Ontap sends more data than Linux server.

Let's be sure we are fixing the right problem. Yes, au_rslack is
smaller in v5.1, and that results in a behavioral regression. But
exactly which part of the new calculation is incorrect is not yet
clear. Simply bumping GSS_VERF_SLACK could very well plaster over
the real problem.


> The opaque_len is just a lot larger. For the first message Linux
> opaque_len is 120bytes and Ontap it's 206. So it could be for instance
> for FSINFO that sends the file handle, for Netapp the file handle is
> 44bytes and for Linux it's only 28bytes.

The maximum filehandle size should already be accounted for in the
maxsize macro for FSINFO.

Is this problem evident only with NFSv3 plus krb5p?


>> Is this explanation for the current value not correct?
>> 
>>  51 /* length of a krb5 verifier (48), plus data added before arguments when
>>  52  * using integrity (two 4-byte integers): */
> 
> I'm not sure what it is suppose to be. Isn't "data before arguments"
> can vary in length and thus explain why linux and onto sizes are
> different?
> Looking at the network trace the krb5 verifier I see is 36bytes.

GSS_VERF_SLACK is only for the extra length added by GSS data. The
length of the RPC message itself is handled separately, see above.

Can you post a Wireshark dissection of the problematic FSINFO reply?
(Having a working reply from Linux and a failing reply from OnTAP
would be even better).


>>> For GSS, au_rslack is calculated from GSS_VERF_SLACK value which is
>>> currently 100. And it's not enough. Changing it to 104 works and then
>>> au_rslack is recalculated based on actual received mic.len and not
>>> just the default buffer size.

What are the computed au_ralign and au_rslack values after the first
successful operation?


>>> I would like to propose to change it to something a little larger than
>>> 104, like 120 to give room if some other server might reply with
>>> something even larger.
>> 
>> Why does it need to be larger than 104?
> 
> I don't know why 100 was chosen and given that I think arguments are
> taken into the account and arguments can change. I think NetApp has
> changed their file handle sizes (at some point, not in the near past
> but i think so?). Perhaps they might want to do that again so the size
> will change again.
> 
> Honestly, I would have like for 100 to be 200 to be safe.

To be safe, I would like to have a good understanding of the details,
rather than guessing at an arbitrary maximum value. Let's choose a
rational maximum and include a descriptive comment about why that value
is the best choice.


>>> Thoughts? Will send an actual patch if no objections to this one.
>>> 
>>> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
>>> index 24ca861..44ae6bc 100644
>>> --- a/net/sunrpc/auth_gss/auth_gss.c
>>> +++ b/net/sunrpc/auth_gss/auth_gss.c
>>> @@ -50,7 +50,7 @@
>>> #define GSS_CRED_SLACK         (RPC_MAX_AUTH_SIZE * 2)
>>> /* length of a krb5 verifier (48), plus data added before arguments when
>>> * using integrity (two 4-byte integers): */
>>> -#define GSS_VERF_SLACK         100
>>> +#define GSS_VERF_SLACK         120
>>> 
>>> static DEFINE_HASHTABLE(gss_auth_hash_table, 4);
>>> static DEFINE_SPINLOCK(gss_auth_hash_lock);
>> 
>> --
>> Chuck Lever

--
Chuck Lever