linux-kernel - Re: [PATCH v2 07/12] nfsd: add tracepoints for symlink events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f38712c5-b2b1-48d8-915b-a0c9f9e72984@oracle.com>
Date: Wed, 9 Apr 2025 11:15:43 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>, Neil Brown <neilb@...e.de>,
        Olga Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>,
        Tom Talpey <tom@...pey.com>, Trond Myklebust <trondmy@...nel.org>,
        Anna Schumaker <anna@...nel.org>
Cc: Sargun Dillon <sargun@...gun.me>, linux-nfs@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 07/12] nfsd: add tracepoints for symlink events

On 4/9/25 10:32 AM, Jeff Layton wrote:
> ...and remove the legacy dprintks.
> 
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
>  fs/nfsd/nfs3proc.c |  8 +++-----
>  fs/nfsd/nfs4proc.c |  3 +++
>  fs/nfsd/nfsproc.c  |  7 +++----
>  fs/nfsd/trace.h    | 35 +++++++++++++++++++++++++++++++++++
>  4 files changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index ea1280970ea11b2a82f0de88ad0422eef7063d6d..587fc92597e7c77d078e871b8d12684c6b5efa2d 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -423,6 +423,9 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
>  		.na_iattr	= &argp->attrs,
>  	};
>  
> +	trace_nfsd3_proc_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
> +				 argp->tname, argp->tlen);
> +
>  	if (argp->tlen == 0) {
>  		resp->status = nfserr_inval;
>  		goto out;
> @@ -440,11 +443,6 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
>  		goto out;
>  	}
>  
> -	dprintk("nfsd: SYMLINK(3)  %s %.*s -> %.*s\n",
> -				SVCFH_fmt(&argp->ffh),
> -				argp->flen, argp->fname,
> -				argp->tlen, argp->tname);
> -
>  	fh_copy(&resp->dirfh, &argp->ffh);
>  	fh_init(&resp->fh, NFS3_FHSIZE);
>  	resp->status = nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 2c795103deaa4044596bd07d90db788169a32a0c..e22596a2e311861be1e4f595d77547be04634ce7 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -873,6 +873,9 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	current->fs->umask = create->cr_umask;
>  	switch (create->cr_type) {
>  	case NF4LNK:
> +		trace_nfsd4_symlink(rqstp, &cstate->current_fh,
> +				    create->cr_name, create->cr_namelen,
> +				    create->cr_data, create->cr_datalen);
>  		status = nfsd_symlink(rqstp, &cstate->current_fh,
>  				      create->cr_name, create->cr_namelen,
>  				      create->cr_data, &attrs, &resfh);
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 33d8cbf8785588d38d4ec5efd769c1d1d06c6a91..0674ed6b978f6caa1325a9271f2fde9b3ef60945 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -506,6 +506,9 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
>  	};
>  	struct svc_fh	newfh;
>  
> +	trace_nfsd_proc_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
> +				argp->tname, argp->tlen);
> +
>  	if (argp->tlen > NFS_MAXPATHLEN) {
>  		resp->status = nfserr_nametoolong;
>  		goto out;
> @@ -519,10 +522,6 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
>  		goto out;
>  	}
>  
> -	dprintk("nfsd: SYMLINK  %s %.*s -> %.*s\n",
> -		SVCFH_fmt(&argp->ffh), argp->flen, argp->fname,
> -		argp->tlen, argp->tname);
> -
>  	fh_init(&newfh, NFS_FHSIZE);
>  	resp->status = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
>  				    argp->tname, &attrs, &newfh);
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index c6aff23a845f06c87e701d57ec577c2c5c5a743c..850dbf1240b234b67dd7d75d6903c0f49dc01261 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -2430,6 +2430,41 @@ DEFINE_NFSD_VFS_CREATE_EVENT(nfsd3_proc_mknod);
>  DEFINE_NFSD_VFS_CREATE_EVENT(nfsd4_create);
>  DEFINE_NFSD_VFS_CREATE_EVENT(nfsd4_create_file);
>  
> +DECLARE_EVENT_CLASS(nfsd_vfs_symlink_class,
> +	TP_PROTO(struct svc_rqst *rqstp,
> +		 struct svc_fh *fhp,
> +		 const char *name,
> +		 unsigned int namelen,
> +		 const char *tgt,
> +		 unsigned int tgtlen),
> +	TP_ARGS(rqstp, fhp, name, namelen, tgt, tgtlen),
> +	TP_STRUCT__entry(
> +		SVC_RQST_ENDPOINT_FIELDS(rqstp)
> +		__field(u32, fh_hash)
> +		__string_len(name, name, namelen)
> +		__string_len(tgt, tgt, tgtlen)
> +	),
> +	TP_fast_assign(
> +		SVC_RQST_ENDPOINT_ASSIGNMENTS(rqstp);
> +		__entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
> +		__assign_str(name);
> +		__assign_str(tgt);
> +	),
> +	TP_printk("xid=0x%08x fh_hash=0x%08x name=%s target=%s",
> +		  __entry->xid, __entry->fh_hash,
> +		  __get_str(name), __get_str(tgt))
> +);
> +
> +#define DEFINE_NFSD_VFS_SYMLINK_EVENT(__name)					\
> +	DEFINE_EVENT(nfsd_vfs_symlink_class, __name,				\
> +		     TP_PROTO(struct svc_rqst *rqstp, struct svc_fh *fhp,	\
> +			      const char *name, unsigned int namelen,		\
> +			      const char *tgt, unsigned int tgtlen),		\
> +		     TP_ARGS(rqstp, fhp, name, namelen, tgt, tgtlen))
> +
> +DEFINE_NFSD_VFS_SYMLINK_EVENT(nfsd_proc_symlink);
> +DEFINE_NFSD_VFS_SYMLINK_EVENT(nfsd3_proc_symlink);
> +DEFINE_NFSD_VFS_SYMLINK_EVENT(nfsd4_symlink);

Likewise, maybe one tracepoint in nfsd_symlink() would be better? This
comment also applies to the remaining patches in this series.

If you're trying to capture the NFS version too, perhaps
SVC_RQST_ENDPOINT can be expanded to record the RPC program and version.

Also, name these new tracepoints "nfsd_vfs_yada" and then you can use a
glob to enable the VFS tracepoints in one go, like so:

  # trace-cmd record -e nfsd:nfsd_vfs_\*


>  #endif /* _NFSD_TRACE_H */
>  
>  #undef TRACE_INCLUDE_PATH
> 


-- 
Chuck Lever