diff mbox series

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

Message ID 20220329113208.2466000-3-chenxiaosong2@huawei.com (mailing list archive)
State New
Headers show
Series fix nfsv4 bugs of opening with O_ACCMODE flag | expand

Commit Message

ChenXiaoSong March 29, 2022, 11:32 a.m. UTC
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(-)

Comments

Trond Myklebust March 29, 2022, 1:05 p.m. UTC | #1
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;
ChenXiaoSong March 29, 2022, 1:44 p.m. UTC | #2
在 2022/3/29 21:05, Trond Myklebust 写道:
> 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.

Reproducer:
```
   1. mount -t nfs -o vers=4.2 $server_ip:/ /mnt/
   2. fd = open("/mnt/file", O_ACCMODE|O_DIRECT|O_CREAT) = 3
   3. close(fd)
   4. fd = open("/mnt/file", O_ACCMODE|O_DIRECT) = -1
```

When firstly open with O_ACCMODE|O_DIRECT flags:
```c
   path_openat
     open_last_lookups
       lookup_open
         atomic_open
           nfs_atomic_open
             create_nfs_open_context
               f_mode = flags_to_mode
               alloc_nfs_open_context(..., f_mode, ...)
                 ctx->mode = f_mode // FMODE_READ|FMODE_WRITE
```

When secondly open with O_ACCMODE|O_DIRECT flags:
```c
   path_openat
     do_open
       vfs_open
         do_dentry_open
           nfs4_file_open
             f_mode = filp->f_mode | flags_to_mode(openflags)
             alloc_nfs_open_context(..., f_mode, ...)
               ctx->mode = f_mode // FMODE_READ|FMODE_WRITE
```

Before merging this patch, when firstly open, we does not set FMODE_READ 
and FMODE_WRITE to file mode of client, FMODE_READ and FMODE_WRITE just 
be set to context mode.

After merging this patch, when secondly open, I just do the same thing, 
file mode of client will not have FMODE_READ and FMODE_WRITE bits, file 
descriptor can't be used for reading or writing.
Trond Myklebust March 29, 2022, 1:56 p.m. UTC | #3
On Tue, 2022-03-29 at 21:44 +0800, chenxiaosong (A) wrote:
> 在 2022/3/29 21:05, Trond Myklebust 写道:
> > 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.
> 
> Reproducer:
> ```
>    1. mount -t nfs -o vers=4.2 $server_ip:/ /mnt/
>    2. fd = open("/mnt/file", O_ACCMODE|O_DIRECT|O_CREAT) = 3
>    3. close(fd)
>    4. fd = open("/mnt/file", O_ACCMODE|O_DIRECT) = -1
> ```
> 
> When firstly open with O_ACCMODE|O_DIRECT flags:
> ```c
>    path_openat
>      open_last_lookups
>        lookup_open
>          atomic_open
>            nfs_atomic_open
>              create_nfs_open_context
>                f_mode = flags_to_mode
>                alloc_nfs_open_context(..., f_mode, ...)
>                  ctx->mode = f_mode // FMODE_READ|FMODE_WRITE
> ```
> 
> When secondly open with O_ACCMODE|O_DIRECT flags:
> ```c
>    path_openat
>      do_open
>        vfs_open
>          do_dentry_open
>            nfs4_file_open
>              f_mode = filp->f_mode | flags_to_mode(openflags)
>              alloc_nfs_open_context(..., f_mode, ...)
>                ctx->mode = f_mode // FMODE_READ|FMODE_WRITE
> ```
> 
> Before merging this patch, when firstly open, we does not set
> FMODE_READ 
> and FMODE_WRITE to file mode of client, FMODE_READ and FMODE_WRITE
> just 
> be set to context mode.
> 
> After merging this patch, when secondly open, I just do the same
> thing, 
> file mode of client will not have FMODE_READ and FMODE_WRITE bits,
> file 
> descriptor can't be used for reading or writing.

I see. OK, I'll probably not apply this for the merge window (since I'm
pretty much queued up to send the pull request at this point), but it
might go in as a bug fix in rc1.
diff mbox series

Patch

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);
 
 	/* 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;