Re: [PATCH v12 05/24] nfsd: fix nfsfh tracepoints to properly handle NULL rqstp

[Mike Snitzer <snitzer@kernel.org>]

On Wed, Aug 21, 2024 at 01:46:02PM -0400, Jeff Layton wrote:
> On Mon, 2024-08-19 at 14:17 -0400, Mike Snitzer wrote:
> > Fixes stop-gap used in previous commit where caller avoided using
> > tracepoint if rqstp is NULL.  Instead, have each tracepoint avoid
> > dereferencing NULL rqstp.
> > 
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfsd/nfsfh.c | 12 ++++--------
> >  fs/nfsd/trace.h | 36 +++++++++++++++++++++---------------
> >  2 files changed, 25 insertions(+), 23 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index 19e173187ab9..bae727e65214 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -195,8 +195,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst
> > *rqstp, struct net *net,
> >  
> >  	error = nfserr_stale;
> >  	if (IS_ERR(exp)) {
> > -		if (rqstp)
> > -			trace_nfsd_set_fh_dentry_badexport(rqstp,
> > fhp, PTR_ERR(exp));
> > +		trace_nfsd_set_fh_dentry_badexport(rqstp, fhp,
> > PTR_ERR(exp));
> >  
> >  		if (PTR_ERR(exp) == -ENOENT)
> >  			return error;
> > @@ -244,8 +243,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst
> > *rqstp, struct net *net,
> >  						data_left,
> > fileid_type, 0,
> >  						nfsd_acceptable,
> > exp);
> >  		if (IS_ERR_OR_NULL(dentry)) {
> > -			if (rqstp)
> > -
> > 				trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp,
> > +			trace_nfsd_set_fh_dentry_badhandle(rqstp,
> > fhp,
> >  					dentry ?  PTR_ERR(dentry) :
> > -ESTALE);
> >  			switch (PTR_ERR(dentry)) {
> >  			case -ENOMEM:
> > @@ -321,8 +319,7 @@ __fh_verify(struct svc_rqst *rqstp,
> >  	dentry = fhp->fh_dentry;
> >  	exp = fhp->fh_export;
> >  
> > -	if (rqstp)
> > -		trace_nfsd_fh_verify(rqstp, fhp, type, access);
> > +	trace_nfsd_fh_verify(net, rqstp, fhp, type, access);
> >  
> >  	/*
> >  	 * We still have to do all these permission checks, even
> > when
> > @@ -376,8 +373,7 @@ __fh_verify(struct svc_rqst *rqstp,
> >  	/* Finally, check access permissions. */
> >  	error = nfsd_permission(cred, exp, dentry, access);
> >  out:
> > -	if (rqstp)
> > -		trace_nfsd_fh_verify_err(rqstp, fhp, type, access,
> > error);
> > +	trace_nfsd_fh_verify_err(net, rqstp, fhp, type, access,
> > error);
> >  	if (error == nfserr_stale)
> >  		nfsd_stats_fh_stale_inc(nn, exp);
> >  	return error;
> > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > index 77bbd23aa150..d49b3c1e3ba9 100644
> > --- a/fs/nfsd/trace.h
> > +++ b/fs/nfsd/trace.h
> > @@ -195,12 +195,13 @@ TRACE_EVENT(nfsd_compound_encode_err,
> >  
> >  TRACE_EVENT(nfsd_fh_verify,
> >  	TP_PROTO(
> > +		const struct net *net,
> >  		const struct svc_rqst *rqstp,
> >  		const struct svc_fh *fhp,
> >  		umode_t type,
> >  		int access
> >  	),
> > -	TP_ARGS(rqstp, fhp, type, access),
> > +	TP_ARGS(net, rqstp, fhp, type, access),
> >  	TP_STRUCT__entry(
> >  		__field(unsigned int, netns_ino)
> >  		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
> > @@ -212,12 +213,14 @@ TRACE_EVENT(nfsd_fh_verify,
> >  		__field(unsigned long, access)
> >  	),
> >  	TP_fast_assign(
> > -		__entry->netns_ino = SVC_NET(rqstp)->ns.inum;
> > -		__assign_sockaddr(server, &rqstp->rq_xprt-
> > >xpt_local,
> > -		       rqstp->rq_xprt->xpt_locallen);
> > -		__assign_sockaddr(client, &rqstp->rq_xprt-
> > >xpt_remote,
> > -				  rqstp->rq_xprt->xpt_remotelen);
> > -		__entry->xid = be32_to_cpu(rqstp->rq_xid);
> > +		__entry->netns_ino = net->ns.inum;
> > +		if (rqstp) {
> > +			__assign_sockaddr(server, &rqstp->rq_xprt-
> > >xpt_local,
> > +					  rqstp->rq_xprt-
> > >xpt_locallen);
> > +			__assign_sockaddr(client, &rqstp->rq_xprt-
> > >xpt_remote,
> > +					  rqstp->rq_xprt-
> > >xpt_remotelen);
> > +		}
> 
> Does this need an else branch to set these values to something when
> rqstp is NULL, or are we guaranteed that they are already zeroed out
> when they aren't assigned?

I'm not sure.  It isn't immediately clear what is actually using these.

But I did just notice an inconsistency, these entry members are defined:

                __sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
                __sockaddr(client, rqstp->rq_xprt->xpt_remotelen)

Yet they go on to use rqstp->rq_xprt->xpt_locallen and
rqstp->rq_xprt->xpt_remotelen respectively.

Chuck, would welcome your feedback on how to properly fix these
tracepoints to handle rqstp being NULL.  And the inconsistency I just
noted is something extra.

Thanks,
Mike