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

Mike