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