On Thu, Sep 1, 2022 at 2:47 PM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> 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@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.
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@xxxxxxxxxxxxxxx
>
>
