| Message ID | 20220329113208.2466000-3-chenxiaosong2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix nfsv4 bugs of opening with O_ACCMODE flag | expand |
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;
在 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.
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 --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;
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(-)