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

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Aug 22, 2024 at 01:07:29PM -0400, Jeff Layton wrote:
> 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@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.
> > > 
> > > 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.

Right, wasn't suggesting to always display loopback interface, was
saying ideal to display reality.  There isn't anything special in the
rpc_clnt address.  The localio RPC client is connecting to the server
using the same rpcclient as NFS (cloned from that used for NFS).

> 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).

I agree.  But yeah, I don't have a sense for how to do this cleanly.

Pass rpc_clnt in and only use it for address if mayflags has
NFSD_MAY_LOCALIO set _and_ rqstop is NULL?