On Thu, 2023-08-24 at 11:42 -0700, dai.ngo@xxxxxxxxxx wrote:
>
> On 8/24/23 9:38 AM, dai.ngo@xxxxxxxxxx wrote:
> >
> > On 8/24/23 9:34 AM, Trond Myklebust wrote:
> > > On Thu, 2023-08-24 at 09:12 -0700, dai.ngo@xxxxxxxxxx wrote:
> > > > On 8/24/23 9:01 AM, Trond Myklebust wrote:
> > > > > On Thu, 2023-08-24 at 08:53 -0700, Dai Ngo wrote:
> > > > > > The Linux NFS server strips the SUID and SGID from the file
> > > > > > mode
> > > > > > on ALLOCATE op. The GETATTR op in the ALLOCATE compound
> > > > > > needs to
> > > > > > request the file mode from the server to update its file
> > > > > > mode in
> > > > > > case the SUID/SGUI bit were stripped.
> > > > > >
> > > > > > Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
> > > > > > ---
> > > > > > fs/nfs/nfs42proc.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > > > > > index 63802d195556..d3d050171822 100644
> > > > > > --- a/fs/nfs/nfs42proc.c
> > > > > > +++ b/fs/nfs/nfs42proc.c
> > > > > > @@ -70,7 +70,7 @@ static int _nfs42_proc_fallocate(struct
> > > > > > rpc_message
> > > > > > *msg, struct file *filep,
> > > > > > }
> > > > > > nfs4_bitmask_set(bitmask, server-
> > > > > > > cache_consistency_bitmask,
> > > > > > inode,
> > > > > > - NFS_INO_INVALID_BLOCKS);
> > > > > > + NFS_INO_INVALID_BLOCKS |
> > > > > > NFS_INO_INVALID_MODE);
> > > > > > res.falloc_fattr = nfs_alloc_fattr();
> > > > > > if (!res.falloc_fattr)
> > > > > Actually... Wait... Why isn't the existing code sufficient?
> > > > >
> > > > > status = nfs4_call_sync(server->client, server,
> > > > > msg,
> > > > > &args.seq_args,
> > > > > &res.seq_res, 0);
> > > > > if (status == 0) {
> > > > > if (nfs_should_remove_suid(inode)) {
> > > > > spin_lock(&inode->i_lock);
> > > > > nfs_set_cache_invalid(inode,
> > > > > NFS_INO_INVALID_MODE);
> > > > > spin_unlock(&inode->i_lock);
> > > > > }
> > > > > status =
> > > > > nfs_post_op_update_inode_force_wcc(inode,
> > > > > res.falloc_fattr);
> > > > > }
> > > > >
> > > > > We explicitly check for SUID bits, and invalidate the mode if
> > > > > they
> > > > > are
> > > > > set.
> > > > nfs_set_cache_invalid checks for delegation and clears the
> > > > NFS_INO_INVALID_MODE.
> > > >
> > > Oh. That just means we need to add NFS_INO_REVAL_FORCED, so let's
> > > rather do that.
> >
> > ok, I'll create a new patch and test it.
>
> This is the new patch:
>
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 63802d195556..ea1991e393e2 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -81,7 +81,7 @@ static int _nfs42_proc_fallocate(struct rpc_message
> *msg, struct file *filep,
> if (status == 0) {
> if (nfs_should_remove_suid(inode)) {
> spin_lock(&inode->i_lock);
> - nfs_set_cache_invalid(inode,
> NFS_INO_INVALID_MODE);
> + nfs_set_cache_invalid(inode,
> NFS_INO_REVAL_FORCED);
No. The above needs to add NFS_INO_REVAL_FORCED.
IOW:
nfs_set_cache_invalid(inode, NFS_INO_INVALID_MODE | NFS_INO_REVAL_FORCED);
> spin_unlock(&inode->i_lock);
> }
> status = nfs_post_op_update_inode_force_wcc(inode,
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
