2023-11-20 01:17:23

by Chuck Lever

Subject: [PATCH v1] NFSD: Document lack of f_pos_lock in nfsd_readdir()

From: Chuck Lever <[email protected]>

Al Viro notes that normal system calls hold f_pos_lock when calling
->iterate_shared and ->llseek; however nfsd_readdir() does not take
that mutex when calling these methods.

It should be safe however because the struct file acquired by
nfsd_readdir() is not visible to other threads.

Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/vfs.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index fbbea7498f02..87a5953c6bf0 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -2109,9 +2109,23 @@ static __be32 nfsd_buffered_readdir(struct file *file, struct svc_fh *fhp,
return cdp->err;
}

-/*
- * Read entries from a directory.
- * The NFSv3/4 verifier we ignore for now.
+/**
+ * nfsd_readdir - Read entries from a directory
+ * @rqstp: RPC transaction context
+ * @fhp: NFS file handle of directory to be read
+ * @offsetp: OUT: seek offset of final entry that was read
+ * @cdp: OUT: an eof error value
+ * @func: entry filler actor
+ *
+ * This implementation ignores the NFSv3/4 verifier cookie.
+ *
+ * NB: normal system calls hold file->f_pos_lock when calling
+ * ->iterate_shared and ->llseek, but nfsd_readdir() does not.
+ * Because the struct file acquired here is not visible to other
+ * threads, it's internal state does not need mutex protection.
+ *
+ * Returns nfs_ok on success, otherwise an nfsstat code is
+ * returned.
*/
__be32
nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp,

2023-11-27 12:42:31

by Jeff Layton

Subject: Re: [PATCH v1] NFSD: Document lack of f_pos_lock in nfsd_readdir()

On Sun, 2023-11-19 at 20:17 -0500, Chuck Lever wrote:
> From: Chuck Lever <[email protected]>
>
> Al Viro notes that normal system calls hold f_pos_lock when calling
> ->iterate_shared and ->llseek; however nfsd_readdir() does not take
> that mutex when calling these methods.
>
> It should be safe however because the struct file acquired by
> nfsd_readdir() is not visible to other threads.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/vfs.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index fbbea7498f02..87a5953c6bf0 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -2109,9 +2109,23 @@ static __be32 nfsd_buffered_readdir(struct file *file, struct svc_fh *fhp,
> return cdp->err;
> }
>
> -/*
> - * Read entries from a directory.
> - * The NFSv3/4 verifier we ignore for now.
> +/**
> + * nfsd_readdir - Read entries from a directory
> + * @rqstp: RPC transaction context
> + * @fhp: NFS file handle of directory to be read
> + * @offsetp: OUT: seek offset of final entry that was read
> + * @cdp: OUT: an eof error value
> + * @func: entry filler actor
> + *
> + * This implementation ignores the NFSv3/4 verifier cookie.
> + *
> + * NB: normal system calls hold file->f_pos_lock when calling
> + * ->iterate_shared and ->llseek, but nfsd_readdir() does not.
> + * Because the struct file acquired here is not visible to other
> + * threads, it's internal state does not need mutex protection.
> + *
> + * Returns nfs_ok on success, otherwise an nfsstat code is
> + * returned.
> */
> __be32
> nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp,
>
>
Good thing to document

Reviewed-by: Jeff Layton <[email protected]>