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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Fixes: 913eca1aea87 ("NFS: Fallocate should use the
> > nfs4_fattr_bitmap")
> > Signed-off-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
> > ---
> > 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
trond.myklebust@xxxxxxxxxxxxxxx
