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@xxxxxxxxxx> 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
