On Thu, 2024-08-22 at 12:04 -0400, Mike Snitzer wrote:
> On Thu, Aug 22, 2024 at 11:07:47AM -0400, Chuck Lever wrote:
> > On Wed, Aug 21, 2024 at 05:23:56PM -0400, Mike Snitzer wrote:
> > > 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.
> >
> > First, a comment about patch ordering: I think you can preserve
> > attribution but make these a little easier to digest if you reverse
> > 4/ and 5/. Fix the problem before it becomes a problem, as it were.
> >
> > As a general remark, I would prefer to retain the trace points and
> > even the address information in the local I/O case: the client
> > address is an important part of the decision to permit or deny
> > access to the FH in question. The issue is how to make that
> > happen...
> >
> > The __sockaddr() macros I think will trigger an oops if
> > rqstp == NULL. The second argument determines the size of a
> > variable-length trace field IIRC. One way to avoid that is to use a
> > fixed size field for the addresses (big enough to store an IPv6
> > address? or an abstract address? those can get pretty big)
> >
> > I need to study 4/ more closely; perhaps it is doing too much in a
> > single patch. (ie, the code ends up in a better place, but the
> > details of the transition are obscured by being lumped together into
> > one patch).
> >
> > So, can you or Neil answer: what would appear as the client address
> > for local I/O ?
>
> Before when there was the "fake" svc_rqst it was initialized with:
>
> /* Note: we're connecting to ourself, so source addr == peer addr */
> rqstp->rq_addrlen = rpc_peeraddr(rpc_clnt,
> (struct sockaddr *)&rqstp->rq_addr,
> sizeof(rqstp->rq_addr));
>
> Anyway, as the code is also now: the rpc_clnt passed to
> nfsd_open_local_fh() will reflect the same address as the server.
>
> My thinking was that for localio there doesn't need to be any explicit
> listing of the address info in the tracepoints (but that'd be more
> convincing if we at least logged localio by looking for and logging
> NFSD_MAY_LOCALIO in mayflags passed to nfsd_file_acquire_local).
>
> But I agree it'd be nice to have tracepoints log matching 127.0.0.1 or
> ::1, etc -- just don't think it strictly necessary.
>
> Open to whatever you think best.
>
The client is likely to be coming from a different container in many
cases and so won't be coming in via the loopback interface. Presenting
a loopback address in that case seems wrong.
What would be ideal IMO would be to still display the addresses from
the rpc_clnt and just display a flag or something that shows that this
is a localio request. Having to pass that as an additional arg to
__fh_verify would be pretty ugly though (and may be a layering
violation).
--
Jeff Layton <jlayton@xxxxxxxxxx>
