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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 ?

-- 
Chuck Lever




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux