Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op

[dai.ngo@oracle.com]

On 8/24/23 3:26 PM, dai.ngo@oracle.com wrote:
>
> On 8/24/23 1:05 PM, Dai Ngo wrote:
>>
>>> On Aug 24, 2023, at 12:01 PM, Trond Myklebust 
>>> <trondmy@hammerspace.com> wrote:
>>>
>>> On Thu, 2023-08-24 at 11:42 -0700, dai.ngo@oracle.com wrote:
>>>>> On 8/24/23 9:38 AM, dai.ngo@oracle.com wrote:
>>>>>
>>>>> On 8/24/23 9:34 AM, Trond Myklebust wrote:
>>>>>> On Thu, 2023-08-24 at 09:12 -0700, dai.ngo@oracle.com 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@oracle.com>
>>>>>>>>> ---
>>>>>>>>>     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);
>> Ok, I’ll try again.
>
> I tried again with this patch:
>
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 63802d195556..5c6f15961a9b 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -80,8 +80,10 @@ static int _nfs42_proc_fallocate(struct rpc_message 
> *msg, struct file *filep,
>                                 &args.seq_args, &res.seq_res, 0);
>         if (status == 0) {
>                 if (nfs_should_remove_suid(inode)) {
> +                       printk("%s: FORCE nfs_set_cache_invalid with 
> NFS_INO_REVAL_FORCE\n", __func__);  <<== for testing
>                         spin_lock(&inode->i_lock);
> -                       nfs_set_cache_invalid(inode, 
> NFS_INO_INVALID_MODE);
> +                       nfs_set_cache_invalid(inode,
> +                               NFS_INO_REVAL_FORCED | 
> NFS_INO_INVALID_MODE);
>                         spin_unlock(&inode->i_lock);
>                 }
>                 status = nfs_post_op_update_inode_force_wcc(inode,
> [dngo@nfsdev linux]$
>
> and the xfstest's generic/683 still fail as with previous patch:
>
> [root@nfsvmd08 xfstests-dev]# diff -u 
> /root/xfstests-dev/tests/generic/683.out 
> /root/xfstests-dev/results//generic/683.out.bad
> --- /root/xfstests-dev/tests/generic/683.out    2023-08-17 
> 23:59:09.621604998 -0600
> +++ /root/xfstests-dev/results//generic/683.out.bad    2023-08-24 
> 15:47:40.684240872 -0600
> @@ -1,19 +1,19 @@
>  QA output created by 683
>  Test 1 - qa_user, non-exec file falloc
>  6666 -rwSrwSrw- TEST_DIR/683/a
> -666 -rw-rw-rw- TEST_DIR/683/a
> +6666 -rwSrwSrw- TEST_DIR/683/a
>
>  Test 2 - qa_user, group-exec file falloc
>  6676 -rwSrwsrw- TEST_DIR/683/a
> -676 -rw-rwxrw- TEST_DIR/683/a
> +6676 -rwSrwsrw- TEST_DIR/683/a
>
>  Test 3 - qa_user, user-exec file falloc
>  6766 -rwsrwSrw- TEST_DIR/683/a
> -766 -rwxrw-rw- TEST_DIR/683/a
> +6766 -rwsrwSrw- TEST_DIR/683/a
>
>  Test 4 - qa_user, all-exec file falloc
>  6777 -rwsrwsrwx TEST_DIR/683/a
> -777 -rwxrwxrwx TEST_DIR/683/a
> +6777 -rwsrwsrwx TEST_DIR/683/a
>
>  Test 5 - root, non-exec file falloc
>  6666 -rwSrwSrw- TEST_DIR/683/a
> @@ -33,9 +33,9 @@
>
>  Test 9 - qa_user, group-exec file falloc, only sgid
>  2676 -rw-rwsrw- TEST_DIR/683/a
> -676 -rw-rwxrw- TEST_DIR/683/a
> +2676 -rw-rwsrw- TEST_DIR/683/a
>
>  Test 10 - qa_user, all-exec file falloc, only sgid
>  2777 -rwxrwsrwx TEST_DIR/683/a
> -777 -rwxrwxrwx TEST_DIR/683/a
> +2777 -rwxrwsrwx TEST_DIR/683/a
>
> [root@nfsvmd08 xfstests-dev]#
>
> I don't think adding NFS_INO_REVAL_FORCED will fix the problem
> because nfs_post_op_update_inode_force_wcc(inode, res.falloc_fattr)
> will only update the file attributes with the attributes returned
> from the GETATTR in the ALLOCATE compound which currently does not
> ask for the file's mode attribute.

ops! sorry I loaded the wrong image.

With NFS_INO_REVAL_FORCED the xfstest generic/683 now passes.
I will submit the v2 patch.

-Dai

>
> -Dai
>
>>
>> Thanks,
>> -Dai
>>>> spin_unlock(&inode->i_lock);
>>>>                  }
>>>>                  status = nfs_post_op_update_inode_force_wcc(inode,
>>> -- 
>>> Trond Myklebust
>>> Linux NFS client maintainer, Hammerspace
>>> trond.myklebust@hammerspace.com
>>>
>>>