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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 8/24/23 3:26 PM, dai.ngo@xxxxxxxxxx wrote:

On 8/24/23 1:05 PM, Dai Ngo wrote:

On Aug 24, 2023, at 12:01 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:

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);
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@xxxxxxxxxxxxxxx





[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux