Re: [Pvfs2-developers] kernel module copy attrs race

Tue, 25 Mar 2008 12:02:28 -0700
Attached patch adds the lock around the getattr for revalidate. It also includes some cleanup to the d_revalidate function for my sanity. 

-sam




Attachment:
fix-attr-race.patch

Description: Binary data





On Mar 25, 2008, at 12:19 PM, Pete Wyckoff wrote:

[EMAIL PROTECTED] wrote on Tue, 25 Mar 2008 12:10 -0500:

I've been debugging a problem with simul returning EACCES errors on  
open of
a PVFS file.  It turns out the bug was due to permissions being  
(re-)set on

an inode without a mutex locking the write.  This was causing the

permissions checks in other opens of the same file to fail because  
the mode

was wrong.

For each (subsequent) open, the kernel calls d_revalidate.  The PVFS

implementation of d_revalidate does a lookup to verify that the  
handle of

the inode and looked-up handle match, then it does a getattr on that

handle, and copies the attributes into the inode.  This is where  
the race

occurred, during copying of the attrs.  In pvfs2-utils.c:231, we set:

inode->i_mode = 0;


/* figure out the correct permissions for the file based on pvfs  
attrs */


if (attrs->perms & PVFS_O_EXECUTE)
           perm_mode |= S_IXOTH;
if (attrs->perms & PVFS_O_WRITE)
           perm_mode |= S_IWOTH;
....

inode->i_mode |= perm_mode;


That entire block is unprotected, so between setting the i_mode to  
0 and

then perm_mode, the i_mode can be accessed by other processes (in the
kernel vfs permissions checking code, for example).


I think the problem is exacerbated by the large block of code that  
converts

the pvfs attrs into the perm_mode field all while i_mode is 0.  Just

setting inode->i_mode = perm_mode directly made the bug disappear  
for my

test case, even though there's still a RMW race there.


I'm tempted to throw a big mutex_lock(inode->i_mutex) around the  
entire
code block that does copy_attrs_to_inode.  Any objections against  
doing

that?


Definitely wants locking, but I think it should be in the caller.
One of the two callers in that file already has the new inode,
locked, so it doesn't want another level of locking.  Ditto for the
caller in dir.c.  If it is true that that inode locking is
sufficient to keep other processes away.

                -- Pete






Attachment:
smime.p7s

Description: S/MIME cryptographic signature


_______________________________________________
Pvfs2-developers mailing list
[email protected]
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers






















					Reply via email to