* [PATCH] NFSD: Checking whether kill_suid by should_remove_suid() @ 2014-04-13 15:11 Kinglong Mee 2014-04-18 13:02 ` J. Bruce Fields 0 siblings, 1 reply; 11+ messages in thread From: Kinglong Mee @ 2014-04-13 15:11 UTC (permalink / raw) To: J. Bruce Fields, linux-nfs As local filesystem, writing data to the file by non-owner will clears the SUID+SGID, owner will not. Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> --- fs/nfsd/vfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 16f0673..19c0931 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -943,7 +943,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file, fsnotify_modify(file); /* clear setuid/setgid flag after write */ - if (inode->i_mode & (S_ISUID | S_ISGID)) + if (should_remove_suid(dentry)) kill_suid(dentry); if (stable) { -- 1.9.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] NFSD: Checking whether kill_suid by should_remove_suid() 2014-04-13 15:11 [PATCH] NFSD: Checking whether kill_suid by should_remove_suid() Kinglong Mee @ 2014-04-18 13:02 ` J. Bruce Fields 2014-04-18 13:51 ` Kinglong Mee 2014-04-18 16:25 ` [PATCH] NFSD: Checking whether kill_suid by should_remove_suid() Kinglong Mee 0 siblings, 2 replies; 11+ messages in thread From: J. Bruce Fields @ 2014-04-18 13:02 UTC (permalink / raw) To: Kinglong Mee; +Cc: linux-nfs On Sun, Apr 13, 2014 at 11:11:39PM +0800, Kinglong Mee wrote: > As local filesystem, writing data to the file by non-owner will > clears the SUID+SGID, owner will not. Are you sure about this? (Do you have a test case that fails?) I don't see an owner check in should_remove_suid. And I think that an nfsd thread will always have CAP_FSETID set (see cap_raise_nfsd_set and the definition of CAP_NFSD_SET), so that should_remove_suid() will always return 0. --b. > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > --- > fs/nfsd/vfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 16f0673..19c0931 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -943,7 +943,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh > *fhp, struct file *file, > fsnotify_modify(file); > > /* clear setuid/setgid flag after write */ > - if (inode->i_mode & (S_ISUID | S_ISGID)) > + if (should_remove_suid(dentry)) > kill_suid(dentry); > > if (stable) { > -- > 1.9.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] NFSD: Checking whether kill_suid by should_remove_suid() 2014-04-18 13:02 ` J. Bruce Fields @ 2014-04-18 13:51 ` Kinglong Mee 2014-04-18 16:17 ` [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data Kinglong Mee 2014-04-18 16:25 ` [PATCH] NFSD: Checking whether kill_suid by should_remove_suid() Kinglong Mee 1 sibling, 1 reply; 11+ messages in thread From: Kinglong Mee @ 2014-04-18 13:51 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On 2014/4/18 21:02, J. Bruce Fields wrote: > On Sun, Apr 13, 2014 at 11:11:39PM +0800, Kinglong Mee wrote: >> As local filesystem, writing data to the file by non-owner will >> clears the SUID+SGID, owner will not. > > Are you sure about this? (Do you have a test case that fails?) Sorry, maybe there are some fault of the comment, and also, there are some problems needs rechecking. Please ignore this patch. I test it using command line with (root, local ext4), touch test; chmod 4777 test; stat test; echo 1234 > test; stat test; File: ‘test’ Size: 0 Blocks: 0 IO Block: 4096 regular empty file Device: 803h/2051d Inode: 1200137 Links: 1 Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) Context: unconfined_u:object_r:admin_home_t:s0 Access: 2014-04-18 21:36:31.016029014 +0800 Modify: 2014-04-18 21:36:31.016029014 +0800 Change: 2014-04-18 21:36:31.026030285 +0800 Birth: - File: ‘test’ Size: 5 Blocks: 8 IO Block: 4096 regular file Device: 803h/2051d Inode: 1200137 Links: 1 Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) Context: unconfined_u:object_r:admin_home_t:s0 Access: 2014-04-18 21:36:31.016029014 +0800 Modify: 2014-04-18 21:36:31.040032065 +0800 Change: 2014-04-18 21:36:31.040032065 +0800 Birth: - With no_root_squash, (root, remote ext4), should not change the sgid/suid, but got File: ‘test’ Size: 0 Blocks: 0 IO Block: 262144 regular empty file Device: 24h/36d Inode: 786439 Links: 1 Access: (4777/-rwsrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) Context: system_u:object_r:nfs_t:s0 Access: 2014-04-18 21:45:32.155805097 +0800 Modify: 2014-04-18 21:45:32.155805097 +0800 Change: 2014-04-18 21:45:32.168806749 +0800 Birth: - File: ‘test’ Size: 5 Blocks: 8 IO Block: 262144 regular file Device: 24h/36d Inode: 786439 Links: 1 Access: (0777/-rwxrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) Context: system_u:object_r:nfs_t:s0 Access: 2014-04-18 21:45:32.155805097 +0800 Modify: 2014-04-18 21:45:32.184808783 +0800 Change: 2014-04-18 21:45:32.184808783 +0800 Birth: - thanks, Kinglong Mee > I don't see an owner check in should_remove_suid. > > And I think that an nfsd thread will always have CAP_FSETID set (see > cap_raise_nfsd_set and the definition of CAP_NFSD_SET), so that > should_remove_suid() will always return 0. > > --b. > >> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> >> --- >> fs/nfsd/vfs.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >> index 16f0673..19c0931 100644 >> --- a/fs/nfsd/vfs.c >> +++ b/fs/nfsd/vfs.c >> @@ -943,7 +943,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh >> *fhp, struct file *file, >> fsnotify_modify(file); >> >> /* clear setuid/setgid flag after write */ >> - if (inode->i_mode & (S_ISUID | S_ISGID)) >> + if (should_remove_suid(dentry)) >> kill_suid(dentry); >> >> if (stable) { >> -- >> 1.9.0 >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data 2014-04-18 13:51 ` Kinglong Mee @ 2014-04-18 16:17 ` Kinglong Mee 2014-05-08 16:12 ` J. Bruce Fields 0 siblings, 1 reply; 11+ messages in thread From: Kinglong Mee @ 2014-04-18 16:17 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs SUID/SGID is not cleared after writing data with (root, local ext4), #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test; File: ‘test’ Size: 0 Blocks: 0 IO Block: 4096 regular empty file Device: 803h/2051d Inode: 1200137 Links: 1 Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) Context: unconfined_u:object_r:admin_home_t:s0 Access: 2014-04-18 21:36:31.016029014 +0800 Modify: 2014-04-18 21:36:31.016029014 +0800 Change: 2014-04-18 21:36:31.026030285 +0800 Birth: - File: ‘test’ Size: 5 Blocks: 8 IO Block: 4096 regular file Device: 803h/2051d Inode: 1200137 Links: 1 Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) Context: unconfined_u:object_r:admin_home_t:s0 Access: 2014-04-18 21:36:31.016029014 +0800 Modify: 2014-04-18 21:36:31.040032065 +0800 Change: 2014-04-18 21:36:31.040032065 +0800 Birth: - With no_root_squash, (root, remote ext4), SUID/SGID are cleared, #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test; File: ‘test’ Size: 0 Blocks: 0 IO Block: 262144 regular empty file Device: 24h/36d Inode: 786439 Links: 1 Access: (4777/-rwsrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) Context: system_u:object_r:nfs_t:s0 Access: 2014-04-18 21:45:32.155805097 +0800 Modify: 2014-04-18 21:45:32.155805097 +0800 Change: 2014-04-18 21:45:32.168806749 +0800 Birth: - File: ‘test’ Size: 5 Blocks: 8 IO Block: 262144 regular file Device: 24h/36d Inode: 786439 Links: 1 Access: (0777/-rwxrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) Context: system_u:object_r:nfs_t:s0 Access: 2014-04-18 21:45:32.155805097 +0800 Modify: 2014-04-18 21:45:32.184808783 +0800 Change: 2014-04-18 21:45:32.184808783 +0800 Birth: - This patch drops codes that kills SGID/SUID visibly, because vfs_writev() can kills it if necessary. With this patch, the above problem will not exist. Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> --- fs/nfsd/vfs.c | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 16f0673..6aaa305 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -857,20 +857,6 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file, return err; } -static void kill_suid(struct dentry *dentry) -{ - struct iattr ia; - ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV; - - mutex_lock(&dentry->d_inode->i_mutex); - /* - * Note we call this on write, so notify_change will not - * encounter any conflicting delegations: - */ - notify_change(dentry, &ia, NULL); - mutex_unlock(&dentry->d_inode->i_mutex); -} - /* * Gathered writes: If another process is currently writing to the file, * there's a high chance this is another nfsd (triggered by a bulk write @@ -942,10 +928,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file, nfsdstats.io_write += host_err; fsnotify_modify(file); - /* clear setuid/setgid flag after write */ - if (inode->i_mode & (S_ISUID | S_ISGID)) - kill_suid(dentry); - if (stable) { if (use_wgather) host_err = wait_for_concurrent_writes(file); -- 1.9.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data 2014-04-18 16:17 ` [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data Kinglong Mee @ 2014-05-08 16:12 ` J. Bruce Fields 2014-05-09 7:55 ` Kinglong Mee 0 siblings, 1 reply; 11+ messages in thread From: J. Bruce Fields @ 2014-05-08 16:12 UTC (permalink / raw) To: Kinglong Mee; +Cc: linux-nfs On Sat, Apr 19, 2014 at 12:17:31AM +0800, Kinglong Mee wrote: > SUID/SGID is not cleared after writing data with (root, local ext4), > #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test; > File: ‘test’ > Size: 0 Blocks: 0 IO Block: 4096 regular > empty file > Device: 803h/2051d Inode: 1200137 Links: 1 > Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) > Context: unconfined_u:object_r:admin_home_t:s0 > Access: 2014-04-18 21:36:31.016029014 +0800 > Modify: 2014-04-18 21:36:31.016029014 +0800 > Change: 2014-04-18 21:36:31.026030285 +0800 > Birth: - > File: ‘test’ > Size: 5 Blocks: 8 IO Block: 4096 regular file > Device: 803h/2051d Inode: 1200137 Links: 1 > Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) > Context: unconfined_u:object_r:admin_home_t:s0 > Access: 2014-04-18 21:36:31.016029014 +0800 > Modify: 2014-04-18 21:36:31.040032065 +0800 > Change: 2014-04-18 21:36:31.040032065 +0800 > Birth: - > > With no_root_squash, (root, remote ext4), SUID/SGID are cleared, > #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test; > File: ‘test’ > Size: 0 Blocks: 0 IO Block: 262144 regular > empty file > Device: 24h/36d Inode: 786439 Links: 1 > Access: (4777/-rwsrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) > Context: system_u:object_r:nfs_t:s0 > Access: 2014-04-18 21:45:32.155805097 +0800 > Modify: 2014-04-18 21:45:32.155805097 +0800 > Change: 2014-04-18 21:45:32.168806749 +0800 > Birth: - > File: ‘test’ > Size: 5 Blocks: 8 IO Block: 262144 regular file > Device: 24h/36d Inode: 786439 Links: 1 > Access: (0777/-rwxrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) > Context: system_u:object_r:nfs_t:s0 > Access: 2014-04-18 21:45:32.155805097 +0800 > Modify: 2014-04-18 21:45:32.184808783 +0800 > Change: 2014-04-18 21:45:32.184808783 +0800 > Birth: - > > This patch drops codes that kills SGID/SUID visibly, > because vfs_writev() can kills it if necessary. > > With this patch, the above problem will not exist. I'd like to apply this if only to remove the redundant code. I'd like to understand, though, whether this is something that caused an actual practical problem for someone, or if you just happened to notice the inconsistency between nfs and ext4 behavior? --b. > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > --- > fs/nfsd/vfs.c | 18 ------------------ > 1 file changed, 18 deletions(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 16f0673..6aaa305 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -857,20 +857,6 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct > svc_fh *fhp, struct file *file, > return err; > } > > -static void kill_suid(struct dentry *dentry) > -{ > - struct iattr ia; > - ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV; > - > - mutex_lock(&dentry->d_inode->i_mutex); > - /* > - * Note we call this on write, so notify_change will not > - * encounter any conflicting delegations: > - */ > - notify_change(dentry, &ia, NULL); > - mutex_unlock(&dentry->d_inode->i_mutex); > -} > - > /* > * Gathered writes: If another process is currently writing to the file, > * there's a high chance this is another nfsd (triggered by a bulk write > @@ -942,10 +928,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct > svc_fh *fhp, struct file *file, > nfsdstats.io_write += host_err; > fsnotify_modify(file); > > - /* clear setuid/setgid flag after write */ > - if (inode->i_mode & (S_ISUID | S_ISGID)) > - kill_suid(dentry); > - > if (stable) { > if (use_wgather) > host_err = wait_for_concurrent_writes(file); > -- > 1.9.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data 2014-05-08 16:12 ` J. Bruce Fields @ 2014-05-09 7:55 ` Kinglong Mee 2014-05-09 21:40 ` J. Bruce Fields 0 siblings, 1 reply; 11+ messages in thread From: Kinglong Mee @ 2014-05-09 7:55 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On 5/9/2014 00:12, J. Bruce Fields wrote: > On Sat, Apr 19, 2014 at 12:17:31AM +0800, Kinglong Mee wrote: >> SUID/SGID is not cleared after writing data with (root, local ext4), >> #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test; >> File: ‘test’ >> Size: 0 Blocks: 0 IO Block: 4096 regular >> empty file >> Device: 803h/2051d Inode: 1200137 Links: 1 >> Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) >> Context: unconfined_u:object_r:admin_home_t:s0 >> Access: 2014-04-18 21:36:31.016029014 +0800 >> Modify: 2014-04-18 21:36:31.016029014 +0800 >> Change: 2014-04-18 21:36:31.026030285 +0800 >> Birth: - >> File: ‘test’ >> Size: 5 Blocks: 8 IO Block: 4096 regular file >> Device: 803h/2051d Inode: 1200137 Links: 1 >> Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) >> Context: unconfined_u:object_r:admin_home_t:s0 >> Access: 2014-04-18 21:36:31.016029014 +0800 >> Modify: 2014-04-18 21:36:31.040032065 +0800 >> Change: 2014-04-18 21:36:31.040032065 +0800 >> Birth: - >> >> With no_root_squash, (root, remote ext4), SUID/SGID are cleared, >> #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test; >> File: ‘test’ >> Size: 0 Blocks: 0 IO Block: 262144 regular >> empty file >> Device: 24h/36d Inode: 786439 Links: 1 >> Access: (4777/-rwsrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) >> Context: system_u:object_r:nfs_t:s0 >> Access: 2014-04-18 21:45:32.155805097 +0800 >> Modify: 2014-04-18 21:45:32.155805097 +0800 >> Change: 2014-04-18 21:45:32.168806749 +0800 >> Birth: - >> File: ‘test’ >> Size: 5 Blocks: 8 IO Block: 262144 regular file >> Device: 24h/36d Inode: 786439 Links: 1 >> Access: (0777/-rwxrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) >> Context: system_u:object_r:nfs_t:s0 >> Access: 2014-04-18 21:45:32.155805097 +0800 >> Modify: 2014-04-18 21:45:32.184808783 +0800 >> Change: 2014-04-18 21:45:32.184808783 +0800 >> Birth: - >> >> This patch drops codes that kills SGID/SUID visibly, >> because vfs_writev() can kills it if necessary. >> >> With this patch, the above problem will not exist. > > I'd like to apply this if only to remove the redundant code. > > I'd like to understand, though, whether this is something that caused an > actual practical problem for someone, or if you just happened to notice > the inconsistency between nfs and ext4 behavior? I test it with ext2,ext3,btrfs,xfs. Test result is same as ext4. So, we needs remove the redundant killing of suid/sgid. thanks, Kinglong Mee > > --b. > >> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> >> --- >> fs/nfsd/vfs.c | 18 ------------------ >> 1 file changed, 18 deletions(-) >> >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >> index 16f0673..6aaa305 100644 >> --- a/fs/nfsd/vfs.c >> +++ b/fs/nfsd/vfs.c >> @@ -857,20 +857,6 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct >> svc_fh *fhp, struct file *file, >> return err; >> } >> >> -static void kill_suid(struct dentry *dentry) >> -{ >> - struct iattr ia; >> - ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV; >> - >> - mutex_lock(&dentry->d_inode->i_mutex); >> - /* >> - * Note we call this on write, so notify_change will not >> - * encounter any conflicting delegations: >> - */ >> - notify_change(dentry, &ia, NULL); >> - mutex_unlock(&dentry->d_inode->i_mutex); >> -} >> - >> /* >> * Gathered writes: If another process is currently writing to the file, >> * there's a high chance this is another nfsd (triggered by a bulk write >> @@ -942,10 +928,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct >> svc_fh *fhp, struct file *file, >> nfsdstats.io_write += host_err; >> fsnotify_modify(file); >> >> - /* clear setuid/setgid flag after write */ >> - if (inode->i_mode & (S_ISUID | S_ISGID)) >> - kill_suid(dentry); >> - >> if (stable) { >> if (use_wgather) >> host_err = wait_for_concurrent_writes(file); >> -- >> 1.9.0 >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data 2014-05-09 7:55 ` Kinglong Mee @ 2014-05-09 21:40 ` J. Bruce Fields 2014-05-10 5:10 ` Christoph Hellwig 0 siblings, 1 reply; 11+ messages in thread From: J. Bruce Fields @ 2014-05-09 21:40 UTC (permalink / raw) To: Kinglong Mee; +Cc: linux-nfs On Fri, May 09, 2014 at 03:55:03PM +0800, Kinglong Mee wrote: > On 5/9/2014 00:12, J. Bruce Fields wrote: > > I'd like to apply this if only to remove the redundant code. > > > > I'd like to understand, though, whether this is something that caused an > > actual practical problem for someone, or if you just happened to notice > > the inconsistency between nfs and ext4 behavior? > > I test it with ext2,ext3,btrfs,xfs. > Test result is same as ext4. > So, we needs remove the redundant killing of suid/sgid. Understood that this would make the behavior consistent with filesystems. But, you don't know of any cases of the current behavior is actually causing a problem for anyone? Anyway, I intend to apply with a slightly longer changelog, as below. --b. commit 0dcee85226291950adde74338a2972ac7f3c9410 Author: Kinglong Mee <kinglongmee@gmail.com> Date: Sat Apr 19 00:17:31 2014 +0800 NFSD: Don't clear SUID/SGID after root writing data We're clearing the SUID/SGID bits on write by hand in nfsd_vfs_write, even though the subsequent vfs_writev() call will end up doing this for us (through file system write methods eventually calling file_remove_suid(), e.g., from __generic_file_aio_write). So, remove the redundant nfsd code. The only change in behavior is when the write is by root, in which case we previously cleared SUID/SGID, but will now leave it alone. The new behavior is the behavior of every filesystem we've checked. It seems better to be consistent with local filesystem behavior. And the security advantage seems limited as root could always restore these bits by hand if it wanted. SUID/SGID is not cleared after writing data with (root, local ext4), File: ‘test’ Size: 0 Blocks: 0 IO Block: 4096 regular empty file Device: 803h/2051d Inode: 1200137 Links: 1 Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) Context: unconfined_u:object_r:admin_home_t:s0 Access: 2014-04-18 21:36:31.016029014 +0800 Modify: 2014-04-18 21:36:31.016029014 +0800 Change: 2014-04-18 21:36:31.026030285 +0800 Birth: - File: ‘test’ Size: 5 Blocks: 8 IO Block: 4096 regular file Device: 803h/2051d Inode: 1200137 Links: 1 Access: (4777/-rwsrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root) Context: unconfined_u:object_r:admin_home_t:s0 Access: 2014-04-18 21:36:31.016029014 +0800 Modify: 2014-04-18 21:36:31.040032065 +0800 Change: 2014-04-18 21:36:31.040032065 +0800 Birth: - With no_root_squash, (root, remote ext4), SUID/SGID are cleared, File: ‘test’ Size: 0 Blocks: 0 IO Block: 262144 regular empty file Device: 24h/36d Inode: 786439 Links: 1 Access: (4777/-rwsrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) Context: system_u:object_r:nfs_t:s0 Access: 2014-04-18 21:45:32.155805097 +0800 Modify: 2014-04-18 21:45:32.155805097 +0800 Change: 2014-04-18 21:45:32.168806749 +0800 Birth: - File: ‘test’ Size: 5 Blocks: 8 IO Block: 262144 regular file Device: 24h/36d Inode: 786439 Links: 1 Access: (0777/-rwxrwxrwx) Uid: ( 1000/ test) Gid: ( 1000/ test) Context: system_u:object_r:nfs_t:s0 Access: 2014-04-18 21:45:32.155805097 +0800 Modify: 2014-04-18 21:45:32.184808783 +0800 Change: 2014-04-18 21:45:32.184808783 +0800 Birth: - Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> Signed-off-by: J. Bruce Fields <bfields@redhat.com> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 16f0673..6aaa305 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -857,20 +857,6 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file, return err; } -static void kill_suid(struct dentry *dentry) -{ - struct iattr ia; - ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV; - - mutex_lock(&dentry->d_inode->i_mutex); - /* - * Note we call this on write, so notify_change will not - * encounter any conflicting delegations: - */ - notify_change(dentry, &ia, NULL); - mutex_unlock(&dentry->d_inode->i_mutex); -} - /* * Gathered writes: If another process is currently writing to the file, * there's a high chance this is another nfsd (triggered by a bulk write @@ -942,10 +928,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file, nfsdstats.io_write += host_err; fsnotify_modify(file); - /* clear setuid/setgid flag after write */ - if (inode->i_mode & (S_ISUID | S_ISGID)) - kill_suid(dentry); - if (stable) { if (use_wgather) host_err = wait_for_concurrent_writes(file); ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data 2014-05-09 21:40 ` J. Bruce Fields @ 2014-05-10 5:10 ` Christoph Hellwig 2014-05-16 7:31 ` Kinglong Mee 0 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2014-05-10 5:10 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Kinglong Mee, linux-nfs On Fri, May 09, 2014 at 05:40:57PM -0400, J. Bruce Fields wrote: > On Fri, May 09, 2014 at 03:55:03PM +0800, Kinglong Mee wrote: > > On 5/9/2014 00:12, J. Bruce Fields wrote: > > > I'd like to apply this if only to remove the redundant code. > > > > > > I'd like to understand, though, whether this is something that caused an > > > actual practical problem for someone, or if you just happened to notice > > > the inconsistency between nfs and ext4 behavior? > > > > I test it with ext2,ext3,btrfs,xfs. > > Test result is same as ext4. > > So, we needs remove the redundant killing of suid/sgid. > > Understood that this would make the behavior consistent with > filesystems. But, you don't know of any cases of the current behavior > is actually causing a problem for anyone? I thin this also is the root cause for xfstests generic/193 failing on NFS, but I haven't verified it yet. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data 2014-05-10 5:10 ` Christoph Hellwig @ 2014-05-16 7:31 ` Kinglong Mee 2014-05-16 15:12 ` Christoph Hellwig 0 siblings, 1 reply; 11+ messages in thread From: Kinglong Mee @ 2014-05-16 7:31 UTC (permalink / raw) To: Christoph Hellwig, J. Bruce Fields; +Cc: linux-nfs On 5/10/2014 13:10, Christoph Hellwig wrote: > On Fri, May 09, 2014 at 05:40:57PM -0400, J. Bruce Fields wrote: >> On Fri, May 09, 2014 at 03:55:03PM +0800, Kinglong Mee wrote: >>> On 5/9/2014 00:12, J. Bruce Fields wrote: >>>> I'd like to apply this if only to remove the redundant code. >>>> >>>> I'd like to understand, though, whether this is something that caused an >>>> actual practical problem for someone, or if you just happened to notice >>>> the inconsistency between nfs and ext4 behavior? >>> >>> I test it with ext2,ext3,btrfs,xfs. >>> Test result is same as ext4. >>> So, we needs remove the redundant killing of suid/sgid. >> >> Understood that this would make the behavior consistent with >> filesystems. But, you don't know of any cases of the current behavior >> is actually causing a problem for anyone? > > I thin this also is the root cause for xfstests generic/193 failing on > NFS, but I haven't verified it yet. xfstests generic/193 only tests non-root user truncating file with root setting SGID/SUID mode. generic/193 will not fail. 236 _create_files 237 # Now test out the clear of suid/sgid for truncate 238 # 239 echo "check that suid/sgid bits are cleared after successful truncate..." 240 241 echo "with no exec perm" 242 echo frobnozzle >> $test_user 243 chmod ug+s $test_user 244 echo -n "before: "; stat -c '%A' $test_user 245 su ${qa_user} -c "echo > $test_user" 246 echo -n "after: "; stat -c '%A' $test_user 247 248 echo "with user exec perm" 249 echo frobnozzle >> $test_user 250 chmod ug+s $test_user 251 chmod u+x $test_user 252 echo -n "before: "; stat -c '%A' $test_user 253 su ${qa_user} -c "echo > $test_user" 254 echo -n "after: "; stat -c '%A' $test_user 255 256 echo "with group exec perm" 257 echo frobnozzle >> $test_user 258 chmod ug+s $test_user 259 chmod g+x $test_user 260 chmod u-x $test_user 261 echo -n "before: "; stat -c '%A' $test_user 262 su ${qa_user} -c "echo > $test_user" 263 echo -n "after: "; stat -c '%A' $test_user 264 265 echo "with user+group exec perm" 266 echo frobnozzle >> $test_user 267 chmod ug+s $test_user 268 chmod ug+x $test_user 269 echo -n "before: "; stat -c '%A' $test_user 270 su ${qa_user} -c "echo > $test_user" 271 echo -n "after: "; stat -c '%A' $test_user thanks, Kinglong Mee ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data 2014-05-16 7:31 ` Kinglong Mee @ 2014-05-16 15:12 ` Christoph Hellwig 0 siblings, 0 replies; 11+ messages in thread From: Christoph Hellwig @ 2014-05-16 15:12 UTC (permalink / raw) To: Kinglong Mee; +Cc: J. Bruce Fields, linux-nfs On Fri, May 16, 2014 at 03:31:09PM +0800, Kinglong Mee wrote: > xfstests generic/193 only tests non-root user truncating file > with root setting SGID/SUID mode. generic/193 will not fail. generic/193 reproducibly fails for me. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] NFSD: Checking whether kill_suid by should_remove_suid() 2014-04-18 13:02 ` J. Bruce Fields 2014-04-18 13:51 ` Kinglong Mee @ 2014-04-18 16:25 ` Kinglong Mee 1 sibling, 0 replies; 11+ messages in thread From: Kinglong Mee @ 2014-04-18 16:25 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On 2014/4/18 21:02, J. Bruce Fields wrote: > On Sun, Apr 13, 2014 at 11:11:39PM +0800, Kinglong Mee wrote: >> As local filesystem, writing data to the file by non-owner will >> clears the SUID+SGID, owner will not. > > Are you sure about this? (Do you have a test case that fails?) > > I don't see an owner check in should_remove_suid. > > And I think that an nfsd thread will always have CAP_FSETID set (see > cap_raise_nfsd_set and the definition of CAP_NFSD_SET), so that > should_remove_suid() will always return 0. You are right, should_remove_suid always return 0, nfsd will never call kill_suid(). Coincidentally, that's the fix for bug of root clears the SUID/SGID after writing data. The right fix should drops the kill_suid(), because vfs_writev() have do it correctly. I have push a new patch. thanks, Kinglong Mee ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-05-16 15:12 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-13 15:11 [PATCH] NFSD: Checking whether kill_suid by should_remove_suid() Kinglong Mee 2014-04-18 13:02 ` J. Bruce Fields 2014-04-18 13:51 ` Kinglong Mee 2014-04-18 16:17 ` [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data Kinglong Mee 2014-05-08 16:12 ` J. Bruce Fields 2014-05-09 7:55 ` Kinglong Mee 2014-05-09 21:40 ` J. Bruce Fields 2014-05-10 5:10 ` Christoph Hellwig 2014-05-16 7:31 ` Kinglong Mee 2014-05-16 15:12 ` Christoph Hellwig 2014-04-18 16:25 ` [PATCH] NFSD: Checking whether kill_suid by should_remove_suid() Kinglong Mee
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).