NFSD: Checking whether kill_suid by should_remove_suid()

PATCH In review Kinglong Mee 3 participants 11 messages Apr 13, 2014 – May 16, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Kinglong Mee

author

2014-04-13 15:12 UTC #0 lore raw

PATCH 1/1 NFSD: Checking whether kill_suid by should_remove_suid()

As local filesystem, writing data to the file by non-owner will clears the SUID+SGID, owner will not.

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/vfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/nfsd/vfs.c +0 -0

index 16f0673..19c0931 100644
@@ -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) {

Signature · 1 line

1.9.0

J. Bruce Fields

2014-04-18 13:02 UTC #1 lore raw

Quoted 2 lines · Kinglong Mee

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.

Quoted 23 lines

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/vfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/nfsd/vfs.c +1 -1

index 16f0673..19c0931 100644
@@ -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) {

Signature · 1 line

1.9.0

Kinglong Mee

author

2014-04-18 13:52 UTC #2 lore raw

Quoted 5 lines · J. Bruce Fields

Quoted 2 lines · Kinglong Mee
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

Quoted 32 lines

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.
Quoted 23 lines
Signed-off-by Kinglong Mee <[email protected]>
---
  fs/nfsd/vfs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
fs/nfsd/vfs.c +1 -1
index 16f0673..19c0931 100644
@@ -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

Kinglong Mee

author

2014-04-18 16:18 UTC #3 lore raw

PATCH v2 1/1 NFSD: Don't clear SUID/SGID after root writing data

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 <[email protected]>

---
 fs/nfsd/vfs.c | 18 ------------------
 1 file changed, 18 deletions(-)

fs/nfsd/vfs.c +0 ~~-18~~

index 16f0673..6aaa305 100644
@@ -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);

Signature · 1 line

1.9.0

Kinglong Mee

author

2014-04-18 16:25 UTC #10 lore raw

In reply to J. Bruce Fields

Quoted 11 lines · J. Bruce Fields

Quoted 2 lines · Kinglong Mee
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

J. Bruce Fields

2014-05-08 16:12 UTC #4 lore raw

Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data

In reply to Kinglong Mee

Quoted 44 lines · Kinglong Mee

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: -

Quoted 4 lines

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.

Quoted 47 lines

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/vfs.c | 18 ------------------
 1 file changed, 18 deletions(-)

fs/nfsd/vfs.c +0 ~~-14~~

index 16f0673..6aaa305 100644
@@ -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

fs/nfsd/vfs.c +0 -4

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

Signature · 1 line

1.9.0

Kinglong Mee

author

2014-05-09 07:55 UTC #5 lore raw

Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data

Quoted 48 lines · J. Bruce Fields

Quoted 47 lines · Kinglong Mee
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.

Quoted 7 lines

Quoted 1 line
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

Quoted 51 lines

--b.

Quoted 47 lines

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/vfs.c | 18 ------------------
 1 file changed, 18 deletions(-)

fs/nfsd/vfs.c +0 ~~-14~~

index 16f0673..6aaa305 100644
@@ -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

fs/nfsd/vfs.c +0 -4

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

Signature · 1 line

1.9.0

J. Bruce Fields

2014-05-09 21:40 UTC #6 lore raw

Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data

Quoted 10 lines · Kinglong Mee

Quoted 5 lines · J. Bruce Fields
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 <[email protected]>
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 <[email protected]>
    Signed-off-by: J. Bruce Fields <[email protected]>

fs/nfsd/vfs.c +0 ~~-18~~

index 16f0673..6aaa305 100644
@@ -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);

Christoph Hellwig

2014-05-10 05:10 UTC #7 lore raw

Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data

Quoted 8 lines · J. Bruce Fields

Quoted 7 lines · Kinglong Mee
Quoted 5 lines · J. Bruce Fields
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?

Quoted 7 lines

Quoted 3 lines
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.

Kinglong Mee

author

2014-05-16 07:31 UTC #8 lore raw

Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data

Quoted 13 lines · Christoph Hellwig

Quoted 12 lines · J. Bruce Fields
Quoted 10 lines · Kinglong Mee
Quoted 5 lines · J. Bruce Fields
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.

Quoted 6 lines

Quoted 3 lines
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

Christoph Hellwig

2014-05-16 15:12 UTC #9 lore raw

Re: [PATCH v2] NFSD: Don't clear SUID/SGID after root writing data

Quoted 2 lines · Kinglong Mee

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.