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

[Chuck Lever <cel@kernel.org>]

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.


> Did you consider
>    if (u->read.rd_fhp)
>        trace .....
>    if (u->read.rd_nf)
>        nfsd_file_put....
> 
> ??

I can't think of a legitimate case where fhp is not NULL but nf is
NULL. Or vice versa.


> Or maybe
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -444,7 +444,7 @@ DECLARE_EVENT_CLASS(nfsd_io_class,
>  	),
>  	TP_fast_assign(
>  		__entry->xid = be32_to_cpu(rqstp->rq_xid);
> -		__entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
> +		__entry->fh_hash = fhp ? 0 : knfsd_fh_hash(&fhp->fh_handle);
>  		__entry->offset = offset;
>  		__entry->len = len;
>  	),
> 
> 
> That would make all IO event trace points robust.
> I'm almost tempted to suggest knfsd_fh_hash() be changed to receive a
> svc_fh and test for NULL, but there are 2 places where there isn't a
> svc_fh handy.
> 
> NeilBrown
> 


-- 
Chuck Lever