[PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Neil Horman]

[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]

Hey all-
	This problem was reported to me recently that when a process is attempting
to change ownership (chown) on a file on a NFS share, it is possible that
permissions bits will be reverted to a previous state on the server, even if an
exclusive lock is held on the file.  I verified this, and came to find that this
occurs if the SUID bit is set on the file.  Basically when a file is chown-ed,
the chown_call forcibly strips the SUID/SGID bits from the mode (ostensibly for
security purposes).  The VFS then implements this stripping (if the SUID/SGID
bits are currently set on the inode in question) by updating the iattr structure
in notify_change to not only change the UID/GID of the inode, but also the MODE,
using the mode bits in the inode.  This works fine on locally mounted file
systems, in which the inodes in ram are only modified by one system, and are in
sync with disk contents.  However on NFS, its possible for the a files
attributes to be out of sync with the cached inode on a given system sharing the
NFS mount.  As such, if system A changes the mode bits on a file on the server,
system B preforming a chown on an inode cached locally can inadvertantly revert
permissions on the inode to whatever was most recently cached.

I don't think this hole can be 100% closed, as we can guarantee the order of
arival of packets to the server between nodes, but we can be sure that the
filesystem bits aren't reverted if the accessing programs participate in a
locking protocol.  The patch below refreshes the inode and strips the SUID/SGID
bits from the latest server side inode->i_mode bits.  This has been tested and
prevents the described problem from occuring.

An alternate solution (and one that generates less traffic) may be to simply
disallow the setting of ATTR_MODE and (ATTR_UID|ATTR_GID) at the same time in
nfs_setattr, but I haven't thought through what all the implications are there.

Thanks & Regards
Neil 

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 inode.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+)


diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -845,6 +845,23 @@ nfs_setattr(struct dentry *dentry, struc
 	if (attr->ia_valid == 0)
 		return 0;
 
+	/*
+	 * If we are changing UID/GID, lets be sure that we
+	 * have the latest mode attribute.  Conducting a chown,
+	 * causes the suid bits to get dropped, and that in turn 
+	 * can accidentally revert file permissions.
+	 */
+	if ((attr->ia_valid & (ATTR_UID|ATTR_GID)) &&
+	    (attr->ia_valid & ATTR_MODE)) {
+		/*
+		 * We're changing U/GIDS and the MODE.  lets be sure
+		 * we have the most up-to-date mode from the server
+		 * minus the SUID/SGID bits
+		 */
+		__nfs_revalidate_inode(NFS_SERVER(inode),inode);
+		attr->ia_mode = inode->i_mode & ~(S_ISUID|S_ISGID);
+	}
+
 	lock_kernel();
 	nfs_begin_data_update(inode);
 	/* Write all dirty data if we're changing file permissions or size */
-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Trond Myklebust]

On Thu, 2005-12-15 at 08:09 -0500, Neil Horman wrote:
> Hey all-
> 	This problem was reported to me recently that when a process is attempting
> to change ownership (chown) on a file on a NFS share, it is possible that
> permissions bits will be reverted to a previous state on the server, even if an
> exclusive lock is held on the file.  I verified this, and came to find that this
> occurs if the SUID bit is set on the file.  Basically when a file is chown-ed,
> the chown_call forcibly strips the SUID/SGID bits from the mode (ostensibly for
> security purposes).  The VFS then implements this stripping (if the SUID/SGID
> bits are currently set on the inode in question) by updating the iattr structure
> in notify_change to not only change the UID/GID of the inode, but also the MODE,
> using the mode bits in the inode.  This works fine on locally mounted file
> systems, in which the inodes in ram are only modified by one system, and are in
> sync with disk contents.  However on NFS, its possible for the a files
> attributes to be out of sync with the cached inode on a given system sharing the
> NFS mount.  As such, if system A changes the mode bits on a file on the server,
> system B preforming a chown on an inode cached locally can inadvertantly revert
> permissions on the inode to whatever was most recently cached.
> 
> I don't think this hole can be 100% closed, as we can guarantee the order of
> arival of packets to the server between nodes, but we can be sure that the
> filesystem bits aren't reverted if the accessing programs participate in a
> locking protocol.  The patch below refreshes the inode and strips the SUID/SGID
> bits from the latest server side inode->i_mode bits.  This has been tested and
> prevents the described problem from occuring.
> 
> An alternate solution (and one that generates less traffic) may be to simply
> disallow the setting of ATTR_MODE and (ATTR_UID|ATTR_GID) at the same time in
> nfs_setattr, but I haven't thought through what all the implications are there.

Third alternative: Fix the VFS to not do that. That makes the whole
thing race-free.

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Neil Horman]

On Thu, Dec 15, 2005 at 09:14:47AM -0500, Trond Myklebust wrote:
> On Thu, 2005-12-15 at 08:09 -0500, Neil Horman wrote:
> > Hey all-
> > 	This problem was reported to me recently that when a process is attempting
> > to change ownership (chown) on a file on a NFS share, it is possible that
> > permissions bits will be reverted to a previous state on the server, even if an
> > exclusive lock is held on the file.  I verified this, and came to find that this
> > occurs if the SUID bit is set on the file.  Basically when a file is chown-ed,
> > the chown_call forcibly strips the SUID/SGID bits from the mode (ostensibly for
> > security purposes).  The VFS then implements this stripping (if the SUID/SGID
> > bits are currently set on the inode in question) by updating the iattr structure
> > in notify_change to not only change the UID/GID of the inode, but also the MODE,
> > using the mode bits in the inode.  This works fine on locally mounted file
> > systems, in which the inodes in ram are only modified by one system, and are in
> > sync with disk contents.  However on NFS, its possible for the a files
> > attributes to be out of sync with the cached inode on a given system sharing the
> > NFS mount.  As such, if system A changes the mode bits on a file on the server,
> > system B preforming a chown on an inode cached locally can inadvertantly revert
> > permissions on the inode to whatever was most recently cached.
> > 
> > I don't think this hole can be 100% closed, as we can guarantee the order of
> > arival of packets to the server between nodes, but we can be sure that the
> > filesystem bits aren't reverted if the accessing programs participate in a
> > locking protocol.  The patch below refreshes the inode and strips the SUID/SGID
> > bits from the latest server side inode->i_mode bits.  This has been tested and
> > prevents the described problem from occuring.
> > 
> > An alternate solution (and one that generates less traffic) may be to simply
> > disallow the setting of ATTR_MODE and (ATTR_UID|ATTR_GID) at the same time in
> > nfs_setattr, but I haven't thought through what all the implications are there.
> 
> Third alternative: Fix the VFS to not do that. That makes the whole
> thing race-free.
> 
I considered this, but I'm not sure thats the right way to go. It seems that the
attribute structure can only set the mode bit, it has no way to represent a
masking of bits.  So to set the mode, the VFS needs to know what the current
mode flags are.  Since the sys_chown code path has no authoritative info on what
the mode bits should be (like chmod does, where the user explicitly specifies
the mode bits), notify_change has to rely on the current mode bits in
inode->i_mode.  We could modify notify_change to not alter the mode bits at all
and leave the ATTR_KILL_SUID flag in the ia_valid field for individual
filesystems to deal with, but I'm not sure thats the better solution, as it
would still leave NFS (and all other filesystems) with the responsibility of
turning the s[u|g]id bits off.

Thanks & Regards
Neil

> Cheers,
>   Trond

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Trond Myklebust]

On Thu, 2005-12-15 at 09:49 -0500, Neil Horman wrote:

> I considered this, but I'm not sure thats the right way to go. It seems that the
> attribute structure can only set the mode bit, it has no way to represent a
> masking of bits.  So to set the mode, the VFS needs to know what the current
> mode flags are.  Since the sys_chown code path has no authoritative info on what
> the mode bits should be (like chmod does, where the user explicitly specifies
> the mode bits), notify_change has to rely on the current mode bits in
> inode->i_mode.  We could modify notify_change to not alter the mode bits at all
> and leave the ATTR_KILL_SUID flag in the ia_valid field for individual
> filesystems to deal with, but I'm not sure thats the better solution, as it
> would still leave NFS (and all other filesystems) with the responsibility of
> turning the s[u|g]id bits off.

As far as an NFS client is concerned, we don't need to clear the
suid/sgid bits at all since the server will do it for us anyway. If the
VFS were to just send us the ATTR_UID/ATTR_GID fields, then we'd be
fine.

As for local filesystems, they can be given a helper function that fixes
up the struct sattr to do the KILL_SUID thing.

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Neil Horman]

On Thu, Dec 15, 2005 at 09:57:24AM -0500, Trond Myklebust wrote:
> On Thu, 2005-12-15 at 09:49 -0500, Neil Horman wrote:
> 
> > I considered this, but I'm not sure thats the right way to go. It seems that the
> > attribute structure can only set the mode bit, it has no way to represent a
> > masking of bits.  So to set the mode, the VFS needs to know what the current
> > mode flags are.  Since the sys_chown code path has no authoritative info on what
> > the mode bits should be (like chmod does, where the user explicitly specifies
> > the mode bits), notify_change has to rely on the current mode bits in
> > inode->i_mode.  We could modify notify_change to not alter the mode bits at all
> > and leave the ATTR_KILL_SUID flag in the ia_valid field for individual
> > filesystems to deal with, but I'm not sure thats the better solution, as it
> > would still leave NFS (and all other filesystems) with the responsibility of
> > turning the s[u|g]id bits off.
> 
> As far as an NFS client is concerned, we don't need to clear the
> suid/sgid bits at all since the server will do it for us anyway. If the
> VFS were to just send us the ATTR_UID/ATTR_GID fields, then we'd be
> fine.
> 
> As for local filesystems, they can be given a helper function that fixes
> up the struct sattr to do the KILL_SUID thing.
> 
> Cheers,
>   Trond
> 
> 
> 
> -------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
> for problems?  Stop!  Download the new AJAX search engine that makes
> searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
> http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> _______________________________________________
> NFS maillist  -  NFS@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs

Then this patch should work just as well (and save the overhead of both adding
the extra getaddr call and the need to add a helper function to all the other
supported file systems to kill the SUID bits that already happens in the VFS.

Thanks & Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 inode.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+)


diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -845,6 +845,22 @@ nfs_setattr(struct dentry *dentry, struc
 	if (attr->ia_valid == 0)
 		return 0;
 
+	/*
+	 * If we are changing UID/GID, lets be sure that we
+	 * have the latest mode attribute.  Conducting a chown,
+	 * causes the suid bits to get dropped, and that in turn 
+	 * can accidentally revert file permissions.
+	 */
+	if ((attr->ia_valid & (ATTR_UID|ATTR_GID)) &&
+	    (attr->ia_valid & ATTR_MODE)) {
+		/*
+		 * We fix this by simply dropping the mode change
+		 * as the NFS server will conditionally drop the 
+		 * the [s|g]uid bits at the server anyway
+		 */
+		attr->ia_valid &= ~ATTR_MODE;
+	}
+
 	lock_kernel();
 	nfs_begin_data_update(inode);
 	/* Write all dirty data if we're changing file permissions or size */
-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Peter Staubach]

Neil Horman wrote:

>On Thu, Dec 15, 2005 at 09:57:24AM -0500, Trond Myklebust wrote:
>  
>
>>On Thu, 2005-12-15 at 09:49 -0500, Neil Horman wrote:
>>
>>    
>>
>>>I considered this, but I'm not sure thats the right way to go. It seems that the
>>>attribute structure can only set the mode bit, it has no way to represent a
>>>masking of bits.  So to set the mode, the VFS needs to know what the current
>>>mode flags are.  Since the sys_chown code path has no authoritative info on what
>>>the mode bits should be (like chmod does, where the user explicitly specifies
>>>the mode bits), notify_change has to rely on the current mode bits in
>>>inode->i_mode.  We could modify notify_change to not alter the mode bits at all
>>>and leave the ATTR_KILL_SUID flag in the ia_valid field for individual
>>>filesystems to deal with, but I'm not sure thats the better solution, as it
>>>would still leave NFS (and all other filesystems) with the responsibility of
>>>turning the s[u|g]id bits off.
>>>      
>>>
>>As far as an NFS client is concerned, we don't need to clear the
>>suid/sgid bits at all since the server will do it for us anyway. If the
>>VFS were to just send us the ATTR_UID/ATTR_GID fields, then we'd be
>>fine.
>>
>>As for local filesystems, they can be given a helper function that fixes
>>up the struct sattr to do the KILL_SUID thing.
>>
>>Cheers,
>>  Trond
>>
>>
>>
>>-------------------------------------------------------
>>This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
>>for problems?  Stop!  Download the new AJAX search engine that makes
>>searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
>>http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
>>_______________________________________________
>>NFS maillist  -  NFS@lists.sourceforge.net
>>https://lists.sourceforge.net/lists/listinfo/nfs
>>    
>>
>
>Then this patch should work just as well (and save the overhead of both adding
>the extra getaddr call and the need to add a helper function to all the other
>supported file systems to kill the SUID bits that already happens in the VFS.
>

I don't think that it can be quite this easy.  I don't think that the 
protocol
requires that the server have this behavior, so, if the client requires it,
then it will need to check to see whether the right thing happened and 
if not,
then make it happen.

Perhaps the right answer is a conditional second SETATTR operation to 
correct
the mode if it does not get set right when the uid/gid is changed.

    Thanx...

       ps


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Trond Myklebust]

On Thu, 2005-12-15 at 11:48 -0500, Peter Staubach wrote:

> I don't think that it can be quite this easy.  I don't think that the 
> protocol
> requires that the server have this behavior, so, if the client requires it,
> then it will need to check to see whether the right thing happened and 
> if not,
> then make it happen.
> 
> Perhaps the right answer is a conditional second SETATTR operation to 
> correct
> the mode if it does not get set right when the uid/gid is changed.

That still allows for dangerous races in which client applications may
suddenly find themselves authorised to suid/sgid to the new uid/gid. Any
server that does this should probably be mounted using the nosuid mount
option and simply not be used for setuid/setgid applications.

As I see it, there are 2 possible options here:

 1) Rely on cached attributes and explicitly clear the suid/sgid mode
bit (which is what we do now).
 2) Rely on the server clearing the suid/sgid mode bit.

Trying to fix up the mode to be set inside the filesystem code is an
unacceptable practice since we cannot know who the caller is, or what
the intentions were.

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Neil Horman]

On Thu, Dec 15, 2005 at 12:32:00PM -0500, Trond Myklebust wrote:
> On Thu, 2005-12-15 at 11:48 -0500, Peter Staubach wrote:
> 
> > I don't think that it can be quite this easy.  I don't think that the 
> > protocol
> > requires that the server have this behavior, so, if the client requires it,
> > then it will need to check to see whether the right thing happened and 
> > if not,
> > then make it happen.
> > 
> > Perhaps the right answer is a conditional second SETATTR operation to 
> > correct
> > the mode if it does not get set right when the uid/gid is changed.
> 
> That still allows for dangerous races in which client applications may
> suddenly find themselves authorised to suid/sgid to the new uid/gid. Any
> server that does this should probably be mounted using the nosuid mount
> option and simply not be used for setuid/setgid applications.
> 
> As I see it, there are 2 possible options here:
> 
>  1) Rely on cached attributes and explicitly clear the suid/sgid mode
> bit (which is what we do now).
This isn't a good option.  It opens up an NFS share to inadvertently allow
access to a file that we shouldn't have rights to change.

>  2) Rely on the server clearing the suid/sgid mode bit.
> 
But you just Nacked my version of the patch which does exactly that, on the
basis that were trying to second guess the intent of the VFS, so it seems like
your trying to have it both ways here.  I suppose we could not mask off the
KILL_SUID flag in ia_valud up in notify_change, and supress the mode
modification in nfs_setattr based on that KILL_SUID flag (trusting that the
server will do it for us).  That way we could be clear on the intent of the VFS.
How does that sound?

Thanks & Regards
Neil

> Trying to fix up the mode to be set inside the filesystem code is an
> unacceptable practice since we cannot know who the caller is, or what
> the intentions were.
> 
> Cheers,
>   Trond
> 
> 
> 
> -------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
> for problems?  Stop!  Download the new AJAX search engine that makes
> searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
> http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> _______________________________________________
> NFS maillist  -  NFS@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Trond Myklebust]

On Thu, 2005-12-15 at 13:17 -0500, Neil Horman wrote:

> >  2) Rely on the server clearing the suid/sgid mode bit.
> > 
> But you just Nacked my version of the patch which does exactly that, on the
> basis that were trying to second guess the intent of the VFS, so it seems like
> your trying to have it both ways here.  I suppose we could not mask off the
> KILL_SUID flag in ia_valud up in notify_change, and supress the mode
> modification in nfs_setattr based on that KILL_SUID flag (trusting that the
> server will do it for us).  That way we could be clear on the intent of the VFS.
> How does that sound?

No. This needs to be fixed in the VFS.

A correct patch would move the gunk in notify_change() that "interprets"
KILL_SUID/KILL_SGID and sets ATTR_MODE into inode_setattr(), then fix up
the users that don't call inode_setattr() to do the right thing.

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Neil Horman]

On Thu, Dec 15, 2005 at 01:17:46PM -0500, Neil Horman wrote:
> On Thu, Dec 15, 2005 at 12:32:00PM -0500, Trond Myklebust wrote:
> > On Thu, 2005-12-15 at 11:48 -0500, Peter Staubach wrote:
> > 
> > > I don't think that it can be quite this easy.  I don't think that the 
> > > protocol
> > > requires that the server have this behavior, so, if the client requires it,
> > > then it will need to check to see whether the right thing happened and 
> > > if not,
> > > then make it happen.
> > > 
> > > Perhaps the right answer is a conditional second SETATTR operation to 
> > > correct
> > > the mode if it does not get set right when the uid/gid is changed.
> > 
> > That still allows for dangerous races in which client applications may
> > suddenly find themselves authorised to suid/sgid to the new uid/gid. Any
> > server that does this should probably be mounted using the nosuid mount
> > option and simply not be used for setuid/setgid applications.
> > 
> > As I see it, there are 2 possible options here:
> > 
> >  1) Rely on cached attributes and explicitly clear the suid/sgid mode
> > bit (which is what we do now).
> This isn't a good option.  It opens up an NFS share to inadvertently allow
> access to a file that we shouldn't have rights to change.
> 
> >  2) Rely on the server clearing the suid/sgid mode bit.
> > 
> But you just Nacked my version of the patch which does exactly that, on the
> basis that were trying to second guess the intent of the VFS, so it seems like
> your trying to have it both ways here.  I suppose we could not mask off the
> KILL_SUID flag in ia_valud up in notify_change, and supress the mode
> modification in nfs_setattr based on that KILL_SUID flag (trusting that the
> server will do it for us).  That way we could be clear on the intent of the VFS.
> How does that sound?
> 
> Thanks & Regards
> Neil
> 
> > Trying to fix up the mode to be set inside the filesystem code is an
> > unacceptable practice since we cannot know who the caller is, or what
> > the intentions were.
> > 
> > Cheers,
> >   Trond

Heres another alternative.  It removes the clearing of ATTR_KILL_SUID from
notify_change, so we can positively determine that the VFS intent in this case
was to issue a mode change for the purpose of clearing the SUID/SGID bits in the
file.  If we assume that the server will drop the s[g|u]id bits on our behalf,
then we can safely drop the mode modification in nfs_setattr, because we know
that was the only reason that the VFS was requesting that the mode be updated.

Thanks & Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 attr.c      |    1 -
 nfs/inode.c |   13 +++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)


diff --git a/fs/attr.c b/fs/attr.c
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -126,7 +126,6 @@ int notify_change(struct dentry * dentry
 	if (!(ia_valid & ATTR_MTIME_SET))
 		attr->ia_mtime = now;
 	if (ia_valid & ATTR_KILL_SUID) {
-		attr->ia_valid &= ~ATTR_KILL_SUID;
 		if (mode & S_ISUID) {
 			if (!(ia_valid & ATTR_MODE)) {
 				ia_valid = attr->ia_valid |= ATTR_MODE;
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -845,6 +845,19 @@ nfs_setattr(struct dentry *dentry, struc
 	if (attr->ia_valid == 0)
 		return 0;
 
+	/*
+	 * If we are modifying UID/GID, and the VFS is 
+	 * explicitly telling us to drop the SUID/SGID bits
+	 * (via the ATTR_KILL_SUID flag), we assume that the
+	 * NFS server is capable of dropping the suid bits
+	 * on our behalf, and so we just drop the mode update
+	 * to prevent inadvertent mode reverts.
+	 */
+	if ((attr->ia_valid & (ATTR_UID|ATTR_GID)) &&
+	    (attr->ia_valid & ATTR_KILL_SUID)) {
+		attr->ia_valid &= ~ATTR_MODE;
+	}
+
 	lock_kernel();
 	nfs_begin_data_update(inode);
 	/* Write all dirty data if we're changing file permissions or size */
-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Trond Myklebust]

On Thu, 2005-12-15 at 14:43 -0500, Neil Horman wrote:

> Heres another alternative.  It removes the clearing of ATTR_KILL_SUID from
> notify_change, so we can positively determine that the VFS intent in this case
> was to issue a mode change for the purpose of clearing the SUID/SGID bits in the
> file.  If we assume that the server will drop the s[g|u]id bits on our behalf,
> then we can safely drop the mode modification in nfs_setattr, because we know
> that was the only reason that the VFS was requesting that the mode be updated.

Why hack it like this instead of doing it right?

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Neil Horman]

On Thu, Dec 15, 2005 at 02:46:15PM -0500, Trond Myklebust wrote:
> On Thu, 2005-12-15 at 14:43 -0500, Neil Horman wrote:
> 
> > Heres another alternative.  It removes the clearing of ATTR_KILL_SUID from
> > notify_change, so we can positively determine that the VFS intent in this case
> > was to issue a mode change for the purpose of clearing the SUID/SGID bits in the
> > file.  If we assume that the server will drop the s[g|u]id bits on our behalf,
> > then we can safely drop the mode modification in nfs_setattr, because we know
> > that was the only reason that the VFS was requesting that the mode be updated.
> 
> Why hack it like this instead of doing it right?
> 
Primarily because I don't really see it as a hack, but secondarily because I was
posting this at the same time that you were posting your suggestion :)

I'll work up another patch that attempts your suggested fix.

Regards
Neil

> Cheers,
>   Trond
> 
> 
> 
> -------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
> for problems?  Stop!  Download the new AJAX search engine that makes
> searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
> http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> _______________________________________________
> NFS maillist  -  NFS@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Neil Horman]

On Thu, Dec 15, 2005 at 02:51:12PM -0500, Neil Horman wrote:
> On Thu, Dec 15, 2005 at 02:46:15PM -0500, Trond Myklebust wrote:
> > On Thu, 2005-12-15 at 14:43 -0500, Neil Horman wrote:
> > 
> > > Heres another alternative.  It removes the clearing of ATTR_KILL_SUID from
> > > notify_change, so we can positively determine that the VFS intent in this case
> > > was to issue a mode change for the purpose of clearing the SUID/SGID bits in the
> > > file.  If we assume that the server will drop the s[g|u]id bits on our behalf,
> > > then we can safely drop the mode modification in nfs_setattr, because we know
> > > that was the only reason that the VFS was requesting that the mode be updated.
> > 
> > Why hack it like this instead of doing it right?
> > 
> Primarily because I don't really see it as a hack, but secondarily because I was
> posting this at the same time that you were posting your suggestion :)
> 
> I'll work up another patch that attempts your suggested fix.
> 
> Regards
> Neil
> 
> > Cheers,
> >   Trond

As promised, heres my updated patch which fixes the permissions regression in
chown entirely within the VFS.  I've tested it, and confirmed that it fixes the
reported problem.  I made a pass through the kernel tree looking for filesystems
which may need to yet call inode_setattr, but it appears (to my somewhat
untrained eye), that at the moment everyone seems to use the call appropriately.
I thought for a moment that nfs_setattr itself might need to make use of the
call, but I believe that nfs_refresh_inode handles all the updates that we would
otherwise need inode_setattr for.  Anywho, tested and working, submitted for
your approval.

Thanks & Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 attr.c      |   41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)


diff --git a/fs/attr.c b/fs/attr.c
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -69,6 +69,27 @@ int inode_setattr(struct inode * inode, 
 	unsigned int ia_valid = attr->ia_valid;
 	int error = 0;
 
+	if (ia_valid & ATTR_KILL_SUID) {
+		attr->ia_valid &= ~ATTR_KILL_SUID;
+		if (inode->i_mode & S_ISUID) {
+			if (!(ia_valid & ATTR_MODE)) {
+				ia_valid = attr->ia_valid |= ATTR_MODE;
+				attr->ia_mode = inode->i_mode & ~S_ISUID;
+			}
+			attr->ia_mode &= ~S_ISUID;
+		}
+	}
+	if (ia_valid & ATTR_KILL_SGID) {
+		attr->ia_valid &= ~ ATTR_KILL_SGID;
+		if ((inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
+			if (!(ia_valid & ATTR_MODE)) {
+				ia_valid = attr->ia_valid |= ATTR_MODE;
+				attr->ia_mode = inode->i_mode & ~S_ISGID;
+			}
+			attr->ia_mode &= ~S_ISGID;
+		}
+	}
+
 	if (ia_valid & ATTR_SIZE) {
 		if (attr->ia_size != i_size_read(inode)) {
 			error = vmtruncate(inode, attr->ia_size);
@@ -125,26 +146,6 @@ int notify_change(struct dentry * dentry
 		attr->ia_atime = now;
 	if (!(ia_valid & ATTR_MTIME_SET))
 		attr->ia_mtime = now;
-	if (ia_valid & ATTR_KILL_SUID) {
-		attr->ia_valid &= ~ATTR_KILL_SUID;
-		if (mode & S_ISUID) {
-			if (!(ia_valid & ATTR_MODE)) {
-				ia_valid = attr->ia_valid |= ATTR_MODE;
-				attr->ia_mode = inode->i_mode;
-			}
-			attr->ia_mode &= ~S_ISUID;
-		}
-	}
-	if (ia_valid & ATTR_KILL_SGID) {
-		attr->ia_valid &= ~ ATTR_KILL_SGID;
-		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
-			if (!(ia_valid & ATTR_MODE)) {
-				ia_valid = attr->ia_valid |= ATTR_MODE;
-				attr->ia_mode = inode->i_mode;
-			}
-			attr->ia_mode &= ~S_ISGID;
-		}
-	}
 	if (!attr->ia_valid)
 		return 0;
 
-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Neil Horman]

On Fri, Dec 16, 2005 at 11:14:25AM -0500, Neil Horman wrote:
> On Thu, Dec 15, 2005 at 02:51:12PM -0500, Neil Horman wrote:
> > On Thu, Dec 15, 2005 at 02:46:15PM -0500, Trond Myklebust wrote:
> > > On Thu, 2005-12-15 at 14:43 -0500, Neil Horman wrote:
> > > 
> > > > Heres another alternative.  It removes the clearing of ATTR_KILL_SUID from
> > > > notify_change, so we can positively determine that the VFS intent in this case
> > > > was to issue a mode change for the purpose of clearing the SUID/SGID bits in the
> > > > file.  If we assume that the server will drop the s[g|u]id bits on our behalf,
> > > > then we can safely drop the mode modification in nfs_setattr, because we know
> > > > that was the only reason that the VFS was requesting that the mode be updated.
> > > 
> > > Why hack it like this instead of doing it right?
> > > 
> > Primarily because I don't really see it as a hack, but secondarily because I was
> > posting this at the same time that you were posting your suggestion :)
> > 
> > I'll work up another patch that attempts your suggested fix.
> > 
> > Regards
> > Neil
> > 
> > > Cheers,
> > >   Trond
> 
> As promised, heres my updated patch which fixes the permissions regression in
> chown entirely within the VFS.  I've tested it, and confirmed that it fixes the
> reported problem.  I made a pass through the kernel tree looking for filesystems
> which may need to yet call inode_setattr, but it appears (to my somewhat
> untrained eye), that at the moment everyone seems to use the call appropriately.
> I thought for a moment that nfs_setattr itself might need to make use of the
> call, but I believe that nfs_refresh_inode handles all the updates that we would
> otherwise need inode_setattr for.  Anywho, tested and working, submitted for
> your approval.
> 
> Thanks & Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
>  attr.c      |   41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> 
> diff --git a/fs/attr.c b/fs/attr.c
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -69,6 +69,27 @@ int inode_setattr(struct inode * inode, 
>  	unsigned int ia_valid = attr->ia_valid;
>  	int error = 0;
>  
> +	if (ia_valid & ATTR_KILL_SUID) {
> +		attr->ia_valid &= ~ATTR_KILL_SUID;
> +		if (inode->i_mode & S_ISUID) {
> +			if (!(ia_valid & ATTR_MODE)) {
> +				ia_valid = attr->ia_valid |= ATTR_MODE;
> +				attr->ia_mode = inode->i_mode & ~S_ISUID;
> +			}
> +			attr->ia_mode &= ~S_ISUID;
> +		}
> +	}
> +	if (ia_valid & ATTR_KILL_SGID) {
> +		attr->ia_valid &= ~ ATTR_KILL_SGID;
> +		if ((inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
> +			if (!(ia_valid & ATTR_MODE)) {
> +				ia_valid = attr->ia_valid |= ATTR_MODE;
> +				attr->ia_mode = inode->i_mode & ~S_ISGID;
> +			}
> +			attr->ia_mode &= ~S_ISGID;
> +		}
> +	}
> +
>  	if (ia_valid & ATTR_SIZE) {
>  		if (attr->ia_size != i_size_read(inode)) {
>  			error = vmtruncate(inode, attr->ia_size);
> @@ -125,26 +146,6 @@ int notify_change(struct dentry * dentry
>  		attr->ia_atime = now;
>  	if (!(ia_valid & ATTR_MTIME_SET))
>  		attr->ia_mtime = now;
> -	if (ia_valid & ATTR_KILL_SUID) {
> -		attr->ia_valid &= ~ATTR_KILL_SUID;
> -		if (mode & S_ISUID) {
> -			if (!(ia_valid & ATTR_MODE)) {
> -				ia_valid = attr->ia_valid |= ATTR_MODE;
> -				attr->ia_mode = inode->i_mode;
> -			}
> -			attr->ia_mode &= ~S_ISUID;
> -		}
> -	}
> -	if (ia_valid & ATTR_KILL_SGID) {
> -		attr->ia_valid &= ~ ATTR_KILL_SGID;
> -		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
> -			if (!(ia_valid & ATTR_MODE)) {
> -				ia_valid = attr->ia_valid |= ATTR_MODE;
> -				attr->ia_mode = inode->i_mode;
> -			}
> -			attr->ia_mode &= ~S_ISGID;
> -		}
> -	}
>  	if (!attr->ia_valid)
>  		return 0;
>  
> -- 
> /***************************************************
>  *Neil Horman
>  *Software Engineer
>  *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
>  ***************************************************/
> 
> 
> -------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
> for problems?  Stop!  Download the new AJAX search engine that makes
> searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
> http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> _______________________________________________
> NFS maillist  -  NFS@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs

Trond-
	Can you provide a status on this.  You had previously sent me a note
indicating that you would give this a spin in the NFS_ALL tree, but I've not
seen it show up there.  Have you found a problem with the patch?

Thanks & Regards
Neil

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
 ***************************************************/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Peter Staubach]

Trond Myklebust wrote:

>On Thu, 2005-12-15 at 11:48 -0500, Peter Staubach wrote:
>
>  
>
>>I don't think that it can be quite this easy.  I don't think that the 
>>protocol
>>requires that the server have this behavior, so, if the client requires it,
>>then it will need to check to see whether the right thing happened and 
>>if not,
>>then make it happen.
>>
>>Perhaps the right answer is a conditional second SETATTR operation to 
>>correct
>>the mode if it does not get set right when the uid/gid is changed.
>>    
>>
>
>That still allows for dangerous races in which client applications may
>suddenly find themselves authorised to suid/sgid to the new uid/gid. Any
>server that does this should probably be mounted using the nosuid mount
>option and simply not be used for setuid/setgid applications.
>
>As I see it, there are 2 possible options here:
>
> 1) Rely on cached attributes and explicitly clear the suid/sgid mode
>bit (which is what we do now).
> 2) Rely on the server clearing the suid/sgid mode bit.
>
>Trying to fix up the mode to be set inside the filesystem code is an
>unacceptable practice since we cannot know who the caller is, or what
>the intentions were.
>

It seems to me that there is a third option.  That would be to provide the
proper synchronization to prevent such races.  Any callers wishing to use
either the mode or owners of the file should have validated the attributes
first, and if there is such a setuid/setgid operation going on, then block
them until that operation has completed.  Perhaps something like the
revalidate hook can be used by the NFS client to synchronize things.

We already know that it is not safe to combine changing the mode and the
uid or gid in a single NFS SETATTR request.  The server may or may not allow
the owners to be changed and so, may or may not perform the mode change.  In
fact, it is not terribly safe to combine changing the uid or gid with any
other attributes in a single call.  This is an unfortunate fact of life
when attempting to be interoperable across a large variety of platforms.

    Thanx...

       ps


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Trond Myklebust]

On Thu, 2005-12-15 at 13:19 -0500, Peter Staubach wrote:
> It seems to me that there is a third option.  That would be to provide the
> proper synchronization to prevent such races.  Any callers wishing to use
> either the mode or owners of the file should have validated the attributes
> first, and if there is such a setuid/setgid operation going on, then block
> them until that operation has completed.  Perhaps something like the
> revalidate hook can be used by the NFS client to synchronize things.

That also requires the VFS change to move the KILL_SUID/KILL_SGID stuff
from notify_change() and into inode_setattr()/the filesystem code.

> We already know that it is not safe to combine changing the mode and the
> uid or gid in a single NFS SETATTR request.  The server may or may not allow
> the owners to be changed and so, may or may not perform the mode change.  In
> fact, it is not terribly safe to combine changing the uid or gid with any
> other attributes in a single call.  This is an unfortunate fact of life
> when attempting to be interoperable across a large variety of platforms.

Yes, but that is why I'd argue that a server claiming to support
suid/sgid mode bits had better have a _very_ good reason if it fails to
clear them when a client changes the uid/gid.

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Peter Staubach]

Trond Myklebust wrote:

>On Thu, 2005-12-15 at 13:19 -0500, Peter Staubach wrote:
>  
>
>>It seems to me that there is a third option.  That would be to provide the
>>proper synchronization to prevent such races.  Any callers wishing to use
>>either the mode or owners of the file should have validated the attributes
>>first, and if there is such a setuid/setgid operation going on, then block
>>them until that operation has completed.  Perhaps something like the
>>revalidate hook can be used by the NFS client to synchronize things.
>>    
>>
>
>That also requires the VFS change to move the KILL_SUID/KILL_SGID stuff
>from notify_change() and into inode_setattr()/the filesystem code.
>
>  
>

Well, that isn't exactly what I had in mind, but I would need to look at
the implementation more in order to describe a design better.

>>We already know that it is not safe to combine changing the mode and the
>>uid or gid in a single NFS SETATTR request.  The server may or may not allow
>>the owners to be changed and so, may or may not perform the mode change.  In
>>fact, it is not terribly safe to combine changing the uid or gid with any
>>other attributes in a single call.  This is an unfortunate fact of life
>>when attempting to be interoperable across a large variety of platforms.
>>    
>>
>
>Yes, but that is why I'd argue that a server claiming to support
>suid/sgid mode bits had better have a _very_ good reason if it fails to
>clear them when a client changes the uid/gid.
>

The most common reason that I have seen is that the server may not be a 
POSIX
based server and may not emulate all of these semantics correctly.  
There are
times when the client has to interoperate with a "broken" server and for 
those
instances, we do not very aesthetic things in the client to make up for it.

    Thanx...

       ps


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH]: NFS: fix inadvertent reversion of mode bits during chown

[Trond Myklebust]

On Thu, 2005-12-15 at 11:38 -0500, Neil Horman wrote:

> Then this patch should work just as well (and save the overhead of both adding
> the extra getaddr call and the need to add a helper function to all the other
> supported file systems to kill the SUID bits that already happens in the VFS.

NACK. The filesystem should _not_ try to second-guess what the VFS
intends it to do.

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs