On Tue, 2022-03-29 at 19:32 +0800, ChenXiaoSong wrote:
> open() with O_ACCMODE|O_DIRECT flags secondly will fail.
>
> Reproducer:
> 1. mount -t nfs -o vers=4.2 $server_ip:/ /mnt/
> 2. fd = open("/mnt/file", O_ACCMODE|O_DIRECT|O_CREAT)
> 3. close(fd)
> 4. fd = open("/mnt/file", O_ACCMODE|O_DIRECT)
>
> Server nfsd4_decode_share_access() will fail with error
> nfserr_bad_xdr when
> client use incorrect share access mode of 0.
>
> Fix this by using NFS4_SHARE_ACCESS_BOTH share access mode in client,
> just like firstly opening.
>
> Fixes: ce4ef7c0a8a05 ("NFS: Split out NFS v4 file operations")
> Signed-off-by: ChenXiaoSong <chenxiaosong2@xxxxxxxxxx>
> ---
> fs/nfs/dir.c | 10 ----------
> fs/nfs/internal.h | 10 ++++++++++
> fs/nfs/nfs4file.c | 6 ++++--
> 3 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 75cb1cbe4cde..911bdb35eb08 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1853,16 +1853,6 @@ const struct dentry_operations
> nfs4_dentry_operations = {
> };
> EXPORT_SYMBOL_GPL(nfs4_dentry_operations);
>
> -static fmode_t flags_to_mode(int flags)
> -{
> - fmode_t res = (__force fmode_t)flags & FMODE_EXEC;
> - if ((flags & O_ACCMODE) != O_WRONLY)
> - res |= FMODE_READ;
> - if ((flags & O_ACCMODE) != O_RDONLY)
> - res |= FMODE_WRITE;
> - return res;
> -}
> -
> static struct nfs_open_context *create_nfs_open_context(struct
> dentry *dentry, int open_flags, struct file *filp)
> {
> return alloc_nfs_open_context(dentry,
> flags_to_mode(open_flags), filp);
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 2de7c56a1fbe..58e618a5d88e 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -42,6 +42,16 @@ static inline bool
> nfs_lookup_is_soft_revalidate(const struct dentry *dentry)
> return true;
> }
>
> +static inline fmode_t flags_to_mode(int flags)
> +{
> + fmode_t res = (__force fmode_t)flags & FMODE_EXEC;
> + if ((flags & O_ACCMODE) != O_WRONLY)
> + res |= FMODE_READ;
> + if ((flags & O_ACCMODE) != O_RDONLY)
> + res |= FMODE_WRITE;
> + return res;
> +}
> +
> /*
> * Note: RFC 1813 doesn't limit the number of auth flavors that
> * a server can return, so make something up.
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index c178db86a6e8..e34af48fb4f4 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -32,6 +32,7 @@ nfs4_file_open(struct inode *inode, struct file
> *filp)
> struct dentry *parent = NULL;
> struct inode *dir;
> unsigned openflags = filp->f_flags;
> + fmode_t f_mode;
> struct iattr attr;
> int err;
>
> @@ -50,8 +51,9 @@ nfs4_file_open(struct inode *inode, struct file
> *filp)
> if (err)
> return err;
>
> + f_mode = filp->f_mode;
> if ((openflags & O_ACCMODE) == 3)
> - openflags--;
> + f_mode |= flags_to_mode(openflags);
>
No. This will not fit the definition of open(2) in the manpage.
Linux reserves the special, nonstandard access mode 3 (binary 11) in
flags to mean: check for read and write permission on the file and re‐
turn a file descriptor that can't be used for reading or writing. This
nonstandard access mode is used by some Linux drivers to return a file
descriptor that is to be used only for device-specific ioctl(2) opera‐
tions.
Your patch will now cause FMODE_READ and FMODE_WRITE to be set on the
file, allowing the file descriptor to be usable for I/O.
> /* We can't create new files here */
> openflags &= ~(O_CREAT|O_EXCL);
> @@ -59,7 +61,7 @@ nfs4_file_open(struct inode *inode, struct file
> *filp)
> parent = dget_parent(dentry);
> dir = d_inode(parent);
>
> - ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode,
> filp);
> + ctx = alloc_nfs_open_context(file_dentry(filp), f_mode,
> filp);
> err = PTR_ERR(ctx);
> if (IS_ERR(ctx))
> goto out;
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
