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