Re: [PATCH v1] NFSD: Fix crash in nfsd4_read_release()

[Chuck Lever <cel@kernel.org>]

On 10/3/25 3:08 AM, NeilBrown wrote:
> On Wed, 01 Oct 2025, Chuck Lever wrote:
>> On 9/30/25 8:53 PM, NeilBrown wrote:
>>> On Wed, 01 Oct 2025, Chuck Lever wrote:
>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>
>>>> When tracing is enabled, the trace_nfsd_read_done trace point
>>>> crashes during the pynfs read.testNoFh test.
>>>>
>>>> Fixes: 87c5942e8fae ("nfsd: Add I/O trace points in the NFSv4 read proc")
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>  fs/nfsd/nfs4proc.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>> index e466cf52d7d7..f9aeefc0da73 100644
>>>> --- a/fs/nfsd/nfs4proc.c
>>>> +++ b/fs/nfsd/nfs4proc.c
>>>> @@ -988,10 +988,11 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>  static void
>>>>  nfsd4_read_release(union nfsd4_op_u *u)
>>>>  {
>>>> -	if (u->read.rd_nf)
>>>> +	if (u->read.rd_nf) {
>>>> +		trace_nfsd_read_done(u->read.rd_rqstp, u->read.rd_fhp,
>>>> +				     u->read.rd_offset, u->read.rd_length);
>>>>  		nfsd_file_put(u->read.rd_nf);
>>>> -	trace_nfsd_read_done(u->read.rd_rqstp, u->read.rd_fhp,
>>>> -			     u->read.rd_offset, u->read.rd_length);
>>>> +	}
>>>>  }
>>>
>>> I must say this looks a bit weird.  rd_nf isn't used in the trace but if
>>> it isn't set, you say the trace crashes...
>>>
>>> That is because rd_fhp being NULL (because there is no current_fh) is
>>> one thing that results in rd_nf being NULL.  Seems a bit indirect.
>>
>> When rd_nf is NULL, no file or file handle is present. That's really all
>> there is to it. You can read it as "when rd_nf is NULL, no processing is
>> needed".
>>
>> The other read trace points are already skipped in this case as well, so
>> there is no need to protect them.
> 
> They are skipped because ALLOWED_WITHOUT_FH is not set so ->op_func
> isn't called.  But ->op_release *is* called.  That seems inconsistent.
> I wonder if we could move the ->op_release call out of
> nfsd4_encode_operation() and only call it in cases were ->op_func
> was called.
> 
> Something like the following?  It isn't as elegant as I would have
> liked, but I think it is better than hiding the ->op_release cxall in
> nfsd4_encode_replay().
> 
> Thanks,
> NeilBrown
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 71b428efcbb5..4ed823d1e6be 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2837,6 +2837,8 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  
>  	trace_nfsd_compound(rqstp, args->tag, args->taglen, args->opcnt);
>  	while (!status && resp->opcnt < args->opcnt) {
> +		bool called_func = false;
> +

Or call it "release_needed" ?

This change is not something I'd care to have backported into stable
kernels... So maybe keep the modification of nfsd4_read_release for
backport, and then subsequently do this change too?

I was reminded of a release-related change that Jeff did recently, maybe
commit 15a8b55dbb1b ("nfsd: call op_release, even when op_func returns
an error")... might be relevant, might not.


>  		op = &args->ops[resp->opcnt++];
>  
>  		/*
> @@ -2886,6 +2888,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  		if (op->opdesc->op_get_currentstateid)
>  			op->opdesc->op_get_currentstateid(cstate, &op->u);
>  		op->status = op->opdesc->op_func(rqstp, cstate, &op->u);
> +		called_func = true;
>  		trace_nfsd_compound_op_err(rqstp, op->opnum, op->status);
>  
>  		/* Only from SEQUENCE */
> @@ -2914,6 +2917,9 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  			nfsd4_encode_operation(resp, op);
>  			status = op->status;
>  		}
> +		if (called_func && op->opdesc->op_release)
> +			op->opdesc->op_release(&op->u);
> +
>  
>  		trace_nfsd_compound_status(args->opcnt, resp->opcnt,
>  					   status, nfsd4_op_name(op->opnum));
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index ea91bad4eee2..e7b4363aadb0 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5885,10 +5885,10 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>  	nfsd4_enc encoder;
>  
>  	if (xdr_stream_encode_u32(xdr, op->opnum) != XDR_UNIT)
> -		goto release;
> +		goto out;
>  	op_status_offset = xdr->buf->len;
>  	if (!xdr_reserve_space(xdr, XDR_UNIT))
> -		goto release;
> +		goto out;
>  
>  	if (op->opnum == OP_ILLEGAL)
>  		goto status;
> @@ -5944,10 +5944,7 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>  				      resp->cstate.minorversion);
>  	write_bytes_to_xdr_buf(xdr->buf, op_status_offset,
>  			       &op->status, XDR_UNIT);
> -release:
> -	if (opdesc && opdesc->op_release)
> -		opdesc->op_release(&op->u);
> -
> +out:
>  	/*
>  	 * Account for pages consumed while encoding this operation.
>  	 * The xdr_stream primitives don't manage rq_next_page.
> 
> 


-- 
Chuck Lever