From: Anna Schumaker <[email protected]>
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 <[email protected]>
Fixes: 913eca1aea87 ("NFS: Fallocate should use the nfs4_fattr_bitmap")
Signed-off-by: Anna Schumaker <[email protected]>
---
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);
+ }
+
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)
{
--
2.37.2
On Thu, 2022-09-01 at 14:05 -0400, Anna Schumaker wrote:
> From: Anna Schumaker <[email protected]>
>
> 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 <[email protected]>
> Fixes: 913eca1aea87 ("NFS: Fallocate should use the
> nfs4_fattr_bitmap")
> Signed-off-by: Anna Schumaker <[email protected]>
> ---
> 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.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Fixes: 913eca1aea87 ("NFS: Fallocate should use the
> > nfs4_fattr_bitmap")
> > Signed-off-by: Anna Schumaker <[email protected]>
> > ---
> > 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.
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Thu, Sep 1, 2022 at 2:47 PM Trond Myklebust <[email protected]> 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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Fixes: 913eca1aea87 ("NFS: Fallocate should use the
> > > nfs4_fattr_bitmap")
> > > Signed-off-by: Anna Schumaker <[email protected]>
> > > ---
> > > 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
> [email protected]
>
>