* [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op @ 2023-08-24 15:53 Dai Ngo 2023-08-24 16:01 ` Trond Myklebust 0 siblings, 1 reply; 10+ messages in thread From: Dai Ngo @ 2023-08-24 15:53 UTC (permalink / raw) To: trondmy, anna; +Cc: linux-nfs 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) -- 2.39.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op 2023-08-24 15:53 [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op Dai Ngo @ 2023-08-24 16:01 ` Trond Myklebust 2023-08-24 16:12 ` dai.ngo 0 siblings, 1 reply; 10+ messages in thread From: Trond Myklebust @ 2023-08-24 16:01 UTC (permalink / raw) To: anna@kernel.org, dai.ngo@oracle.com; +Cc: linux-nfs@vger.kernel.org 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. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op 2023-08-24 16:01 ` Trond Myklebust @ 2023-08-24 16:12 ` dai.ngo 2023-08-24 16:34 ` Trond Myklebust 0 siblings, 1 reply; 10+ messages in thread From: dai.ngo @ 2023-08-24 16:12 UTC (permalink / raw) To: Trond Myklebust, anna@kernel.org; +Cc: linux-nfs@vger.kernel.org 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. -Dai > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op 2023-08-24 16:12 ` dai.ngo @ 2023-08-24 16:34 ` Trond Myklebust 2023-08-24 16:38 ` dai.ngo 0 siblings, 1 reply; 10+ messages in thread From: Trond Myklebust @ 2023-08-24 16:34 UTC (permalink / raw) To: anna@kernel.org, dai.ngo@oracle.com; +Cc: linux-nfs@vger.kernel.org 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. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op 2023-08-24 16:34 ` Trond Myklebust @ 2023-08-24 16:38 ` dai.ngo 2023-08-24 18:42 ` dai.ngo 0 siblings, 1 reply; 10+ messages in thread From: dai.ngo @ 2023-08-24 16:38 UTC (permalink / raw) To: Trond Myklebust, anna@kernel.org; +Cc: linux-nfs@vger.kernel.org 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. -Dai > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op 2023-08-24 16:38 ` dai.ngo @ 2023-08-24 18:42 ` dai.ngo 2023-08-24 19:01 ` Trond Myklebust 0 siblings, 1 reply; 10+ messages in thread From: dai.ngo @ 2023-08-24 18:42 UTC (permalink / raw) To: Trond Myklebust, anna@kernel.org; +Cc: linux-nfs@vger.kernel.org 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 > >> ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op 2023-08-24 18:42 ` dai.ngo @ 2023-08-24 19:01 ` Trond Myklebust 2023-08-24 20:05 ` Dai Ngo 0 siblings, 1 reply; 10+ messages in thread From: Trond Myklebust @ 2023-08-24 19:01 UTC (permalink / raw) To: anna@kernel.org, dai.ngo@oracle.com; +Cc: linux-nfs@vger.kernel.org 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); > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op 2023-08-24 19:01 ` Trond Myklebust @ 2023-08-24 20:05 ` Dai Ngo 2023-08-24 22:26 ` dai.ngo 0 siblings, 1 reply; 10+ messages in thread From: Dai Ngo @ 2023-08-24 20:05 UTC (permalink / raw) To: Trond Myklebust; +Cc: anna@kernel.org, linux-nfs@vger.kernel.org > 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. 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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op 2023-08-24 20:05 ` Dai Ngo @ 2023-08-24 22:26 ` dai.ngo 2023-08-24 23:01 ` dai.ngo 0 siblings, 1 reply; 10+ messages in thread From: dai.ngo @ 2023-08-24 22:26 UTC (permalink / raw) To: Trond Myklebust; +Cc: anna@kernel.org, linux-nfs@vger.kernel.org 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. -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 >> >> ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op 2023-08-24 22:26 ` dai.ngo @ 2023-08-24 23:01 ` dai.ngo 0 siblings, 0 replies; 10+ messages in thread From: dai.ngo @ 2023-08-24 23:01 UTC (permalink / raw) To: Trond Myklebust; +Cc: anna@kernel.org, linux-nfs@vger.kernel.org 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 >>> >>> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-08-24 23:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-24 15:53 [PATCH v2 1/1] nfs42: client needs to update file mode after ALLOCATE op Dai Ngo 2023-08-24 16:01 ` Trond Myklebust 2023-08-24 16:12 ` dai.ngo 2023-08-24 16:34 ` Trond Myklebust 2023-08-24 16:38 ` dai.ngo 2023-08-24 18:42 ` dai.ngo 2023-08-24 19:01 ` Trond Myklebust 2023-08-24 20:05 ` Dai Ngo 2023-08-24 22:26 ` dai.ngo 2023-08-24 23:01 ` dai.ngo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).