Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220901180503.1347290-1-anna@kernel.org> <c38f77a54257fcf8c59b6e628cc13679fb3fb48d.camel@hammerspace.com>
 <50a371e51b76469671ede8d47201eb8d01dd5720.camel@hammerspace.com>
In-Reply-To: <50a371e51b76469671ede8d47201eb8d01dd5720.camel@hammerspace.com>
From:   Anna Schumaker <anna@kernel.org>
Date:   Thu, 1 Sep 2022 14:53:15 -0400
Message-ID: <CAFX2Jfn7Yuihiy8XOaRnrfeosRgR0536hZ9Tps8AtL78tj+Yjw@mail.gmail.com>
Subject: Re: [PATCH v2] NFSv4.2: Update mode bits after ALLOCATE and DEALLOCATE
To:     Trond Myklebust <trondmy@hammerspace.com>
Cc:     "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "cuiyue-fnst@fujitsu.com" <cuiyue-fnst@fujitsu.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>
>