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

[dai.ngo@oracle.com]

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);
  			spin_unlock(&inode->i_lock);
  		}
  		status = nfs_post_op_update_inode_force_wcc(inode,
-- 
2.9.5

looks like this patch causes more tests to fail:

[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 12:32:49.980272051 -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'll have to dig deeper to find out why unless you already
knew what the problem is.

-Dai

>
> -Dai
>
>>