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

[Jeff Layton <jlayton@kernel.org>]

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?

> +		__entry->xid = rqstp ? be32_to_cpu(rqstp->rq_xid) :
> 0;
>  		__entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
>  		__entry->inode = d_inode(fhp->fh_dentry);
>  		__entry->type = type;
> @@ -232,13 +235,14 @@ TRACE_EVENT(nfsd_fh_verify,
>  
>  TRACE_EVENT_CONDITION(nfsd_fh_verify_err,
>  	TP_PROTO(
> +		const struct net *net,
>  		const struct svc_rqst *rqstp,
>  		const struct svc_fh *fhp,
>  		umode_t type,
>  		int access,
>  		__be32 error
>  	),
> -	TP_ARGS(rqstp, fhp, type, access, error),
> +	TP_ARGS(net, rqstp, fhp, type, access, error),
>  	TP_CONDITION(error),
>  	TP_STRUCT__entry(
>  		__field(unsigned int, netns_ino)
> @@ -252,12 +256,14 @@ TRACE_EVENT_CONDITION(nfsd_fh_verify_err,
>  		__field(int, error)
>  	),
>  	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);
> +		}
> +		__entry->xid = rqstp ? be32_to_cpu(rqstp->rq_xid) :
> 0;
>  		__entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
>  		if (fhp->fh_dentry)
>  			__entry->inode = d_inode(fhp->fh_dentry);
> @@ -286,7 +292,7 @@ DECLARE_EVENT_CLASS(nfsd_fh_err_class,
>  		__field(int, status)
>  	),
>  	TP_fast_assign(
> -		__entry->xid = be32_to_cpu(rqstp->rq_xid);
> +		__entry->xid = rqstp ? be32_to_cpu(rqstp->rq_xid) :
> 0;
>  		__entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
>  		__entry->status = status;
>  	),

-- 
Jeff Layton <jlayton@kernel.org>