Received-SPF: pass (google.com: domain of linux-nfs+bounces-922-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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 06:55:51 -0500
Message-ID: <25DCF24F-FB84-4A52-A66E-63A445197AB6@redhat.com>
In-Reply-To: <1aa005d1c0b344a455ced93be866dff3c316e15e.camel@hammerspace.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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.

I will send a v4 with your suggestion.

Ben