| Message ID | 20220901180503.1347290-1-anna@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] NFSv4.2: Update mode bits after ALLOCATE and DEALLOCATE | expand |
On Thu, 2022-09-01 at 14:05 -0400, Anna Schumaker wrote: > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > The fallocate call invalidates suid and sgid bits as part of normal > operation. We need to mark the mode bits as invalid when using > fallocate > with an suid so these will be updated the next time the user looks at > them. > > This fixes xfstests generic/683 and generic/684. > > Reported-by: Yue Cui <cuiyue-fnst@fujitsu.com> > Fixes: 913eca1aea87 ("NFS: Fallocate should use the > nfs4_fattr_bitmap") > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > --- > fs/nfs/internal.h | 25 +++++++++++++++++++++++++ > fs/nfs/nfs42proc.c | 13 ++++++++++++- > fs/nfs/write.c | 25 ------------------------- > 3 files changed, 37 insertions(+), 26 deletions(-) > > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h > index 27c720d71b4e..898dd95bc7a7 100644 > --- a/fs/nfs/internal.h > +++ b/fs/nfs/internal.h > @@ -606,6 +606,31 @@ static inline gfp_t nfs_io_gfp_mask(void) > return GFP_KERNEL; > } > > +/* > + * Special version of should_remove_suid() that ignores > capabilities. > + */ > +static inline int nfs_should_remove_suid(const struct inode *inode) > +{ > + umode_t mode = inode->i_mode; > + int kill = 0; > + > + /* suid always must be killed */ > + if (unlikely(mode & S_ISUID)) > + kill = ATTR_KILL_SUID; > + > + /* > + * sgid without any exec bits is just a mandatory locking > mark; leave > + * it alone. If some exec bits are set, it's a real sgid; > kill it. > + */ > + if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) > + kill |= ATTR_KILL_SGID; > + > + if (unlikely(kill && S_ISREG(mode))) > + return kill; > + > + return 0; > +} > + > /* unlink.c */ > extern struct rpc_task * > nfs_async_rename(struct inode *old_dir, struct inode *new_dir, > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c > index 068c45b3bc1a..23023ddf75d1 100644 > --- a/fs/nfs/nfs42proc.c > +++ b/fs/nfs/nfs42proc.c > @@ -56,6 +56,7 @@ static int _nfs42_proc_fallocate(struct rpc_message > *msg, struct file *filep, > struct nfs42_falloc_res res = { > .falloc_server = server, > }; > + unsigned int invalid = 0; > int status; > > msg->rpc_argp = &args; > @@ -78,10 +79,20 @@ static int _nfs42_proc_fallocate(struct > rpc_message *msg, struct file *filep, > > status = nfs4_call_sync(server->client, server, msg, > &args.seq_args, &res.seq_res, 0); > + > + if (!res.falloc_fattr->valid) > + invalid |= NFS_INO_INVALID_ATTR; > + if (nfs_should_remove_suid(inode)) > + invalid |= NFS_INO_INVALID_MODE; > + if (invalid) { > + spin_lock(&inode->i_lock); > + nfs_set_cache_invalid(inode, NFS_INO_INVALID_MODE); > + spin_unlock(&inode->i_lock); > + } This all really wants to go into the 'if (status == 0)' below. > + > if (status == 0) > status = nfs_post_op_update_inode_force_wcc(inode, > > res.falloc_fattr); > - > if (msg->rpc_proc == > &nfs4_procedures[NFSPROC4_CLNT_ALLOCATE]) > trace_nfs4_fallocate(inode, &args, status); > else > diff --git a/fs/nfs/write.c b/fs/nfs/write.c > index 1843fa235d9b..f41d24b54fd1 100644 > --- a/fs/nfs/write.c > +++ b/fs/nfs/write.c > @@ -1496,31 +1496,6 @@ void nfs_commit_prepare(struct rpc_task *task, > void *calldata) > NFS_PROTO(data->inode)->commit_rpc_prepare(task, data); > } > > -/* > - * Special version of should_remove_suid() that ignores > capabilities. > - */ > -static int nfs_should_remove_suid(const struct inode *inode) > -{ > - umode_t mode = inode->i_mode; > - int kill = 0; > - > - /* suid always must be killed */ > - if (unlikely(mode & S_ISUID)) > - kill = ATTR_KILL_SUID; > - > - /* > - * sgid without any exec bits is just a mandatory locking > mark; leave > - * it alone. If some exec bits are set, it's a real sgid; > kill it. > - */ > - if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) > - kill |= ATTR_KILL_SGID; > - > - if (unlikely(kill && S_ISREG(mode))) > - return kill; > - > - return 0; > -} > - > static void nfs_writeback_check_extend(struct nfs_pgio_header *hdr, > struct nfs_fattr *fattr) > { Otherwise, looks OK.
On Thu, 2022-09-01 at 14:22 -0400, Trond Myklebust wrote: > On Thu, 2022-09-01 at 14:05 -0400, Anna Schumaker wrote: > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > The fallocate call invalidates suid and sgid bits as part of normal > > operation. We need to mark the mode bits as invalid when using > > fallocate > > with an suid so these will be updated the next time the user looks > > at > > them. > > > > This fixes xfstests generic/683 and generic/684. > > > > Reported-by: Yue Cui <cuiyue-fnst@fujitsu.com> > > Fixes: 913eca1aea87 ("NFS: Fallocate should use the > > nfs4_fattr_bitmap") > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > --- > > fs/nfs/internal.h | 25 +++++++++++++++++++++++++ > > fs/nfs/nfs42proc.c | 13 ++++++++++++- > > fs/nfs/write.c | 25 ------------------------- > > 3 files changed, 37 insertions(+), 26 deletions(-) > > > > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h > > index 27c720d71b4e..898dd95bc7a7 100644 > > --- a/fs/nfs/internal.h > > +++ b/fs/nfs/internal.h > > @@ -606,6 +606,31 @@ static inline gfp_t nfs_io_gfp_mask(void) > > return GFP_KERNEL; > > } > > > > +/* > > + * Special version of should_remove_suid() that ignores > > capabilities. > > + */ > > +static inline int nfs_should_remove_suid(const struct inode > > *inode) > > +{ > > + umode_t mode = inode->i_mode; > > + int kill = 0; > > + > > + /* suid always must be killed */ > > + if (unlikely(mode & S_ISUID)) > > + kill = ATTR_KILL_SUID; > > + > > + /* > > + * sgid without any exec bits is just a mandatory locking > > mark; leave > > + * it alone. If some exec bits are set, it's a real sgid; > > kill it. > > + */ > > + if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) > > + kill |= ATTR_KILL_SGID; > > + > > + if (unlikely(kill && S_ISREG(mode))) > > + return kill; > > + > > + return 0; > > +} > > + > > /* unlink.c */ > > extern struct rpc_task * > > nfs_async_rename(struct inode *old_dir, struct inode *new_dir, > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c > > index 068c45b3bc1a..23023ddf75d1 100644 > > --- a/fs/nfs/nfs42proc.c > > +++ b/fs/nfs/nfs42proc.c > > @@ -56,6 +56,7 @@ static int _nfs42_proc_fallocate(struct > > rpc_message > > *msg, struct file *filep, > > struct nfs42_falloc_res res = { > > .falloc_server = server, > > }; > > + unsigned int invalid = 0; > > int status; > > > > msg->rpc_argp = &args; > > @@ -78,10 +79,20 @@ static int _nfs42_proc_fallocate(struct > > rpc_message *msg, struct file *filep, > > > > status = nfs4_call_sync(server->client, server, msg, > > &args.seq_args, &res.seq_res, 0); > > + > > + if (!res.falloc_fattr->valid) > > + invalid |= NFS_INO_INVALID_ATTR; Oh wait... We shouldn't need this.^^^^^^^^^ nfs_post_op_update_inode_force_wcc() will do the right thing for you here. > > + if (nfs_should_remove_suid(inode)) > > + invalid |= NFS_INO_INVALID_MODE; > > + if (invalid) { > > + spin_lock(&inode->i_lock); > > + nfs_set_cache_invalid(inode, NFS_INO_INVALID_MODE); > > + spin_unlock(&inode->i_lock); > > + } > > This all really wants to go into the 'if (status == 0)' below. > > + > > if (status == 0) > > status = nfs_post_op_update_inode_force_wcc(inode, > > > > res.falloc_fattr); > > - > > if (msg->rpc_proc == > > &nfs4_procedures[NFSPROC4_CLNT_ALLOCATE]) > > trace_nfs4_fallocate(inode, &args, status); > > else > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c > > index 1843fa235d9b..f41d24b54fd1 100644 > > --- a/fs/nfs/write.c > > +++ b/fs/nfs/write.c > > @@ -1496,31 +1496,6 @@ void nfs_commit_prepare(struct rpc_task > > *task, > > void *calldata) > > NFS_PROTO(data->inode)->commit_rpc_prepare(task, data); > > } > > > > -/* > > - * Special version of should_remove_suid() that ignores > > capabilities. > > - */ > > -static int nfs_should_remove_suid(const struct inode *inode) > > -{ > > - umode_t mode = inode->i_mode; > > - int kill = 0; > > - > > - /* suid always must be killed */ > > - if (unlikely(mode & S_ISUID)) > > - kill = ATTR_KILL_SUID; > > - > > - /* > > - * sgid without any exec bits is just a mandatory locking > > mark; leave > > - * it alone. If some exec bits are set, it's a real sgid; > > kill it. > > - */ > > - if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) > > - kill |= ATTR_KILL_SGID; > > - > > - if (unlikely(kill && S_ISREG(mode))) > > - return kill; > > - > > - return 0; > > -} > > - > > static void nfs_writeback_check_extend(struct nfs_pgio_header > > *hdr, > > struct nfs_fattr *fattr) > > { > > Otherwise, looks OK. >
On Thu, Sep 1, 2022 at 2:47 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Thu, 2022-09-01 at 14:22 -0400, Trond Myklebust wrote: > > On Thu, 2022-09-01 at 14:05 -0400, Anna Schumaker wrote: > > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > > > The fallocate call invalidates suid and sgid bits as part of normal > > > operation. We need to mark the mode bits as invalid when using > > > fallocate > > > with an suid so these will be updated the next time the user looks > > > at > > > them. > > > > > > This fixes xfstests generic/683 and generic/684. > > > > > > Reported-by: Yue Cui <cuiyue-fnst@fujitsu.com> > > > Fixes: 913eca1aea87 ("NFS: Fallocate should use the > > > nfs4_fattr_bitmap") > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > --- > > > fs/nfs/internal.h | 25 +++++++++++++++++++++++++ > > > fs/nfs/nfs42proc.c | 13 ++++++++++++- > > > fs/nfs/write.c | 25 ------------------------- > > > 3 files changed, 37 insertions(+), 26 deletions(-) > > > > > > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h > > > index 27c720d71b4e..898dd95bc7a7 100644 > > > --- a/fs/nfs/internal.h > > > +++ b/fs/nfs/internal.h > > > @@ -606,6 +606,31 @@ static inline gfp_t nfs_io_gfp_mask(void) > > > return GFP_KERNEL; > > > } > > > > > > +/* > > > + * Special version of should_remove_suid() that ignores > > > capabilities. > > > + */ > > > +static inline int nfs_should_remove_suid(const struct inode > > > *inode) > > > +{ > > > + umode_t mode = inode->i_mode; > > > + int kill = 0; > > > + > > > + /* suid always must be killed */ > > > + if (unlikely(mode & S_ISUID)) > > > + kill = ATTR_KILL_SUID; > > > + > > > + /* > > > + * sgid without any exec bits is just a mandatory locking > > > mark; leave > > > + * it alone. If some exec bits are set, it's a real sgid; > > > kill it. > > > + */ > > > + if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) > > > + kill |= ATTR_KILL_SGID; > > > + > > > + if (unlikely(kill && S_ISREG(mode))) > > > + return kill; > > > + > > > + return 0; > > > +} > > > + > > > /* unlink.c */ > > > extern struct rpc_task * > > > nfs_async_rename(struct inode *old_dir, struct inode *new_dir, > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c > > > index 068c45b3bc1a..23023ddf75d1 100644 > > > --- a/fs/nfs/nfs42proc.c > > > +++ b/fs/nfs/nfs42proc.c > > > @@ -56,6 +56,7 @@ static int _nfs42_proc_fallocate(struct > > > rpc_message > > > *msg, struct file *filep, > > > struct nfs42_falloc_res res = { > > > .falloc_server = server, > > > }; > > > + unsigned int invalid = 0; > > > int status; > > > > > > msg->rpc_argp = &args; > > > @@ -78,10 +79,20 @@ static int _nfs42_proc_fallocate(struct > > > rpc_message *msg, struct file *filep, > > > > > > status = nfs4_call_sync(server->client, server, msg, > > > &args.seq_args, &res.seq_res, 0); > > > + > > > + if (!res.falloc_fattr->valid) > > > + invalid |= NFS_INO_INVALID_ATTR; > > Oh wait... We shouldn't need this.^^^^^^^^^ > nfs_post_op_update_inode_force_wcc() will do the right thing for you > here. Sure. I can remove that and put everything under the "if (status == 0)". Anna > > > > + if (nfs_should_remove_suid(inode)) > > > + invalid |= NFS_INO_INVALID_MODE; > > > + if (invalid) { > > > + spin_lock(&inode->i_lock); > > > + nfs_set_cache_invalid(inode, NFS_INO_INVALID_MODE); > > > + spin_unlock(&inode->i_lock); > > > + } > > > > This all really wants to go into the 'if (status == 0)' below. > > > + > > > if (status == 0) > > > status = nfs_post_op_update_inode_force_wcc(inode, > > > > > > res.falloc_fattr); > > > - > > > if (msg->rpc_proc == > > > &nfs4_procedures[NFSPROC4_CLNT_ALLOCATE]) > > > trace_nfs4_fallocate(inode, &args, status); > > > else > > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c > > > index 1843fa235d9b..f41d24b54fd1 100644 > > > --- a/fs/nfs/write.c > > > +++ b/fs/nfs/write.c > > > @@ -1496,31 +1496,6 @@ void nfs_commit_prepare(struct rpc_task > > > *task, > > > void *calldata) > > > NFS_PROTO(data->inode)->commit_rpc_prepare(task, data); > > > } > > > > > > -/* > > > - * Special version of should_remove_suid() that ignores > > > capabilities. > > > - */ > > > -static int nfs_should_remove_suid(const struct inode *inode) > > > -{ > > > - umode_t mode = inode->i_mode; > > > - int kill = 0; > > > - > > > - /* suid always must be killed */ > > > - if (unlikely(mode & S_ISUID)) > > > - kill = ATTR_KILL_SUID; > > > - > > > - /* > > > - * sgid without any exec bits is just a mandatory locking > > > mark; leave > > > - * it alone. If some exec bits are set, it's a real sgid; > > > kill it. > > > - */ > > > - if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) > > > - kill |= ATTR_KILL_SGID; > > > - > > > - if (unlikely(kill && S_ISREG(mode))) > > > - return kill; > > > - > > > - return 0; > > > -} > > > - > > > static void nfs_writeback_check_extend(struct nfs_pgio_header > > > *hdr, > > > struct nfs_fattr *fattr) > > > { > > > > Otherwise, looks OK. > > > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h index 27c720d71b4e..898dd95bc7a7 100644 --- a/fs/nfs/internal.h +++ b/fs/nfs/internal.h @@ -606,6 +606,31 @@ static inline gfp_t nfs_io_gfp_mask(void) return GFP_KERNEL; } +/* + * Special version of should_remove_suid() that ignores capabilities. + */ +static inline int nfs_should_remove_suid(const struct inode *inode) +{ + umode_t mode = inode->i_mode; + int kill = 0; + + /* suid always must be killed */ + if (unlikely(mode & S_ISUID)) + kill = ATTR_KILL_SUID; + + /* + * sgid without any exec bits is just a mandatory locking mark; leave + * it alone. If some exec bits are set, it's a real sgid; kill it. + */ + if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) + kill |= ATTR_KILL_SGID; + + if (unlikely(kill && S_ISREG(mode))) + return kill; + + return 0; +} + /* unlink.c */ extern struct rpc_task * nfs_async_rename(struct inode *old_dir, struct inode *new_dir, diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c index 068c45b3bc1a..23023ddf75d1 100644 --- a/fs/nfs/nfs42proc.c +++ b/fs/nfs/nfs42proc.c @@ -56,6 +56,7 @@ static int _nfs42_proc_fallocate(struct rpc_message *msg, struct file *filep, struct nfs42_falloc_res res = { .falloc_server = server, }; + unsigned int invalid = 0; int status; msg->rpc_argp = &args; @@ -78,10 +79,20 @@ static int _nfs42_proc_fallocate(struct rpc_message *msg, struct file *filep, status = nfs4_call_sync(server->client, server, msg, &args.seq_args, &res.seq_res, 0); + + if (!res.falloc_fattr->valid) + invalid |= NFS_INO_INVALID_ATTR; + if (nfs_should_remove_suid(inode)) + invalid |= NFS_INO_INVALID_MODE; + if (invalid) { + spin_lock(&inode->i_lock); + nfs_set_cache_invalid(inode, NFS_INO_INVALID_MODE); + spin_unlock(&inode->i_lock); + } + if (status == 0) status = nfs_post_op_update_inode_force_wcc(inode, res.falloc_fattr); - if (msg->rpc_proc == &nfs4_procedures[NFSPROC4_CLNT_ALLOCATE]) trace_nfs4_fallocate(inode, &args, status); else diff --git a/fs/nfs/write.c b/fs/nfs/write.c index 1843fa235d9b..f41d24b54fd1 100644 --- a/fs/nfs/write.c +++ b/fs/nfs/write.c @@ -1496,31 +1496,6 @@ void nfs_commit_prepare(struct rpc_task *task, void *calldata) NFS_PROTO(data->inode)->commit_rpc_prepare(task, data); } -/* - * Special version of should_remove_suid() that ignores capabilities. - */ -static int nfs_should_remove_suid(const struct inode *inode) -{ - umode_t mode = inode->i_mode; - int kill = 0; - - /* suid always must be killed */ - if (unlikely(mode & S_ISUID)) - kill = ATTR_KILL_SUID; - - /* - * sgid without any exec bits is just a mandatory locking mark; leave - * it alone. If some exec bits are set, it's a real sgid; kill it. - */ - if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) - kill |= ATTR_KILL_SGID; - - if (unlikely(kill && S_ISREG(mode))) - return kill; - - return 0; -} - static void nfs_writeback_check_extend(struct nfs_pgio_header *hdr, struct nfs_fattr *fattr) {