Re: [PATCH -next 2/2] NFSv4: fix open failure with O_ACCMODE flag

[Trond Myklebust <trondmy@hammerspace.com>]

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@huawei.com>
> ---
>  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@hammerspace.com