Received-SPF: pass (google.com: domain of linux-nfs+bounces-925-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Benjamin Coddington <bcodding@redhat.com>
To: Trond Myklebust <trondmy@hammerspace.com>, anna@kernel.org
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH v3 2/2] NFSv4.1: Use the nfs_client's rpc timeouts for
 backchannel
Date: Thu, 04 Jan 2024 08:23:50 -0500
Message-ID: <1707B7F5-0D22-44BC-8170-1D119F71F9B1@redhat.com>
In-Reply-To: <47231988-6BE0-4A1C-93CE-F5D14600BA8F@redhat.com>
References: <e28038fba1243f00b0dd66b7c5296a1e181645ea.1702496910.git.bcodding@redhat.com>
 <90c9365ad91e1eb0058b170fb332ea70ad554d8b.1702496910.git.bcodding@redhat.com>
 <CAFX2Jf=CARs=2pPhi-Lj_ydZKyqpjA=kZQoWDNsRKM3gdp=CYw@mail.gmail.com>
 <1aa005d1c0b344a455ced93be866dff3c316e15e.camel@hammerspace.com>
 <25DCF24F-FB84-4A52-A66E-63A445197AB6@redhat.com>
 <47231988-6BE0-4A1C-93CE-F5D14600BA8F@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 4 Jan 2024, at 7:13, Benjamin Coddington wrote:

> On 4 Jan 2024, at 6:55, Benjamin Coddington wrote:
>
>> On 3 Jan 2024, at 18:00, Trond Myklebust wrote:
>>
>>> On Wed, 2024-01-03 at 16:45 -0500, Anna Schumaker wrote:
>>>> Hi Ben,
>>>>
>>>> On Wed, Dec 13, 2023 at 2:49 PM Benjamin Coddington
>>>> <bcodding@redhat.com> wrote:
>>>>>
>>>>> For backchannel requests that lookup the appropriate nfs_client,
>>>>> use the
>>>>> state-management rpc_clnt's rpc_timeout parameters for the
>>>>> backchannel's
>>>>> response.  When the nfs_client cannot be found, fall back to using
>>>>> the
>>>>> xprt's default timeout parameters.
>>>>
>>>> I'm seeing a use-after-free after applying this patch when using pNFS
>>>> and session trunking. Any idea what's going on? Here is the stack
>>>> trace I'm seeing:
>>>
>>> I'm going to guess that this is happening because nothing is clearing
>>> rqstp->bc_rpc_clnt after a call to svc_process_bc(). So if something
>>> later calls CB_NULL, then the resulting svc_process_bc() will free an
>>> extra reference.
>>
>> Doh!
>>
>>>>
>>>> I hit this while testing against ontap, if that helps.
>>>>
>>>> Thanks,
>>>> Anna
>>
>> Thank you for the test!!
>>
>>>>> --- a/fs/nfs/callback_xdr.c
>>>>> +++ b/fs/nfs/callback_xdr.c
>>>>> @@ -967,6 +967,9 @@ static __be32 nfs4_callback_compound(struct
>>>>> svc_rqst *rqstp)
>>>>>                 nops--;
>>>>>         }
>>>>>
>>>>> +       if (svc_is_backchannel(rqstp) && cps.clp)
>>>>> +               rqstp->bc_rpc_clnt = cps.clp->cl_rpcclient;
>>>
>>>
>>> You can re-create the clnt->cl_timeout in svc_process_bc() by just
>>> storing the values of to_initval and to_retrans here. Why store a
>>> reference to an entire rpc client structure that you don't need?
>>
>> Hmm, I think I started by thinking we could simply set tk_client, but then
>> didn't end up with that for other reasons and just kept passing the single
>> pointer.
>
> .. and the client being NULL was the signal to fallback to using the xprt
> timeouts.  That's lost with to_initval and to_retries because they can be
> set to zero deliberately.

I'm wrong about this ^^.  The mount option timeo must be > 0, so we can
check if to_initval is zero to see if we should fall back to the xprt
default.

Ben